Function pointer bug fixes #45473

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

@@ -1303,6 +1303,30 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
            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

@@ -506,6 +506,11 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
            return node.Update(rewrittenArgument, method, node.IsExtensionMethod, type);
        }

+        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 AlekseyTs and a team 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.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function pointer bug fixes #45473

Function pointer bug fixes #45473

333fred commented Jun 26, 2020 •

edited

Loading

AlekseyTs Jun 26, 2020 •

edited

Loading

333fred Jun 26, 2020

AlekseyTs Jun 26, 2020 •

edited

Loading

333fred Jun 26, 2020

jaredpar Jun 26, 2020 •

edited by 333fred

Loading

AlekseyTs commented Jun 26, 2020 •

edited

Loading

333fred commented Jun 26, 2020

AlekseyTs Jun 26, 2020 •

edited

Loading

333fred Jun 26, 2020

jaredpar Jun 26, 2020

jaredpar Jun 26, 2020

AlekseyTs Jun 26, 2020 •

edited

Loading

AlekseyTs commented Jun 26, 2020 •

edited

Loading

333fred commented Jun 26, 2020

AlekseyTs left a comment

ghost left a comment

Function pointer bug fixes #45473

Function pointer bug fixes #45473

Conversation

333fred commented Jun 26, 2020 • edited Loading

AlekseyTs Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

333fred Jun 26, 2020

Choose a reason for hiding this comment

AlekseyTs Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

333fred Jun 26, 2020

Choose a reason for hiding this comment

jaredpar Jun 26, 2020 • edited by 333fred Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 26, 2020 • edited Loading

333fred commented Jun 26, 2020

AlekseyTs Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

333fred Jun 26, 2020

Choose a reason for hiding this comment

jaredpar Jun 26, 2020

Choose a reason for hiding this comment

jaredpar Jun 26, 2020

Choose a reason for hiding this comment

AlekseyTs Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 26, 2020 • edited Loading

333fred commented Jun 26, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

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