Function pointer bug fixes by 333fred · Pull Request #45473 · dotnet/roslyn

333fred · 2020-06-26T02:52:37Z

Fixes a couple of function pointer bugs:

Commit 1 ensures that we use translated local function symbols, not the original symbol, when emitting the ldftn instruction, and corrects handling of non-static local functions. This fixes Address of local function not working #45447.
Commit 2 ensures that we correctly bind arguments for error recovery in all cases. Fixes [16.7P3] VS crashes opening .cs file with incorrect function pointers usage #45418.
Commit 5 ensures that we don't emit an ldftn the result is not used.

@dotnet/roslyn-compiler @AlekseyTs for review.

…tnet#45447.

…otnet#45418.

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

AlekseyTs · 2020-06-26T04:01:40Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

+                    ref arguments,
+                    ref argRefKinds);
+
+                Debug.Assert(arguments.IsDefault && argRefKinds.IsDefault && receiver.Kind == BoundKind.TypeExpression);


receiver.Kind == BoundKind.TypeExpression [](start = 77, length = 41)

What is this type? #Closed

Made the var explicit and added some more verification of the remapped method.

In reply to: 445955503 [](ancestors = 445955503)

AlekseyTs · 2020-06-26T04:05:02Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs

            return base.VisitDelegateCreationExpression(node);
        }

+        public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node)


VisitFunctionPointerLoad [](start = 34, length = 24)

Did you check other places where we override VisitDelegateCreationExpression to confirm that we don't need to add VisitFunctionPointerLoad? We probably have several visitors that introduce virtual methods with names like that. #Closed

I've taken a look. I'll add an override in MethodToClassRewriter, but I can't find a case that it will actually change, as other than this it's only used for iterator and async method rewriting. Since we don't allow you to refer to the compiler-generated underlying methods of these from source, there's nothing that we can test. If we add a new thing that gets remapped in the future, though, it will function as expected.

In reply to: 445956108 [](ancestors = 445956108)

jaredpar · 2020-06-26T04:06:52Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs

+        delegate*<void> a = &local;
+        a();
+
+        static void local() => System.Console.Write(""local"");


Consider adding a test where the contents of this method body exist within a lambda expression. Verifying the rewriting works in a nested environment. #Resolved

AlekseyTs · 2020-06-26T04:09:08Z

Done with review pass (iteration 3) #Closed

* Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts.

333fred · 2020-06-26T21:33:40Z

@AlekseyTs addressed feedback.

This PR now addresses an additional issue I found when testing locally, where an ldftn would be left on the stack when the result was unused. @jaredpar, I'd like another review with that additional fix.

In reply to: 649949098 [](ancestors = 649949098)

AlekseyTs · 2020-06-26T21:36:28Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+            if (used)
+            {
+                _builder.EmitOpCode(ILOpCode.Ldftn);
+                EmitSymbolToken(load.TargetMethod, load.Syntax, optArgList: null);


EmitSymbolToken(load.TargetMethod, load.Syntax, optArgList: null); [](start = 16, length = 66)

Can this have a side effect, for example an exception? Is this conditional handling consistent with what we do for delegate creation? I.e. if a delegate instance is not used, are we not loading the address of a target method? #Closed

If I hadn't fixed the remapped local function bug, it would have an effect, but the only effect from this I could see now is if a tree-trimming algorithm decides the referenced method isn't used (which it isn't) and decides to remove it. However, when I implemented this I checked our handling for delegates:

static void M() { System.Action a = M; }

produces this IL in release mode:

.method private hidebysig static void M () cil managed { // Method begins at RVA 0x2050 // Code size 1 (0x1) .maxstack 8 IL_0000: ret } // end of method C::M

In reply to: 446421466 [](ancestors = 446421466)

My intuition was that the tree trimming aspect was fine, and likely desired. The fact that it lines up with delegate behavior makes me feel good about this outcome.

jaredpar · 2020-06-26T21:52:09Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+            if (used)
+            {
+                _builder.EmitOpCode(ILOpCode.Ldftn);
+                EmitSymbolToken(load.TargetMethod, load.Syntax, optArgList: null);


My intuition was that the tree trimming aspect was fine, and likely desired. The fact that it lines up with delegate behavior makes me feel good about this outcome.

AlekseyTs · 2020-06-26T23:23:53Z

src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs


+        public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node)
+        {
+            return node.Update(VisitMethodSymbol(node.TargetMethod), node.Type);


node.Type [](start = 69, length = 9)

It looks like we should also visit type. #Closed

AlekseyTs · 2020-06-26T23:27:18Z

Done with review pass (iteration 5) #Closed

333fred · 2020-06-26T23:33:16Z

@AlekseyTs addressed feedback.

In reply to: 650448547 [](ancestors = 650448547)

AlekseyTs

LGTM (iteration 6), assuming CI is passing

ghost

Auto-approval

333fred added 2 commits June 25, 2020 18:19

Correctly support local functions in function pointer loads. Fixes do…

3c614d2

…tnet#45447.

Correctly bind function pointer arguments for error recovery. Fixes d…

ab5e21a

…otnet#45418.

333fred requested review from a team and AlekseyTs June 26, 2020 02:52

jaredpar reviewed Jun 26, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs Show resolved Hide resolved

Correct baselines.

801f1ab

AlekseyTs reviewed Jun 26, 2020

View reviewed changes

jaredpar reviewed Jun 26, 2020

View reviewed changes

jaredpar approved these changes Jun 26, 2020

View reviewed changes

333fred added 2 commits June 26, 2020 14:30

PR Feedback:

e1133ad

* Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts.

Don't emit ldftn when the result is unused.

a670e7a

333fred requested a review from jaredpar June 26, 2020 21:33

AlekseyTs reviewed Jun 26, 2020

View reviewed changes

jaredpar approved these changes Jun 26, 2020

View reviewed changes

AlekseyTs reviewed Jun 26, 2020

View reviewed changes

VisitType in MethodToClassRewriter for function pointers.

71d1c6f

AlekseyTs approved these changes Jun 26, 2020

View reviewed changes

333fred added Area-Compilers auto-merge Feature - Function Pointers Adding Function Pointers labels Jun 27, 2020

ghost approved these changes Jun 27, 2020

View reviewed changes

ghost merged commit fa69a8c into dotnet:master Jun 27, 2020

333fred deleted the func-ptr-bugs branch June 27, 2020 02:08

This pull request was closed.

Conversation

333fred commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlekseyTs Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 26, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Jun 26, 2020

Uh oh!

AlekseyTs Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Jun 26, 2020

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

333fred commented Jun 26, 2020 •

edited

Loading

AlekseyTs Jun 26, 2020 •

edited

Loading

AlekseyTs Jun 26, 2020 •

edited

Loading

jaredpar Jun 26, 2020 •

edited by 333fred

Loading

AlekseyTs commented Jun 26, 2020 •

edited

Loading

AlekseyTs Jun 26, 2020 •

edited

Loading

AlekseyTs Jun 26, 2020 •

edited

Loading

AlekseyTs commented Jun 26, 2020 •

edited

Loading