Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function pointer bug fixes #45473

Merged
6 commits merged into from
Jun 27, 2020
Merged

Function pointer bug fixes #45473

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c614d2
Correctly support local functions in function pointer loads. Fixes ht…
333fred Jun 26, 2020
ab5e21a
Correctly bind function pointer arguments for error recovery. Fixes h…
333fred Jun 26, 2020
801f1ab
Correct baselines.
333fred Jun 26, 2020
e1133ad
PR Feedback:
333fred Jun 26, 2020
a670e7a
Don't emit ldftn when the result is unused.
333fred Jun 26, 2020
71d1c6f
VisitType in MethodToClassRewriter for function pointers.
333fred Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,6 +1671,7 @@ private BoundFunctionPointerInvocation BindFunctionPointerInvocation(SyntaxNode

if (!overloadResolutionResult.Succeeded)
{
ImmutableArray<FunctionPointerMethodSymbol> methods = methodsBuilder.ToImmutableAndFree();
overloadResolutionResult.ReportDiagnostics(
binder: this,
node.Location,
Expand All @@ -1680,15 +1681,15 @@ private BoundFunctionPointerInvocation BindFunctionPointerInvocation(SyntaxNode
boundExpression,
boundExpression.Syntax,
analyzedArguments,
methodsBuilder.ToImmutableAndFree(),
methods,
typeContainingConstructor: null,
delegateTypeBeingInvoked: null,
returnRefKind: funcPtr.Signature.RefKind);

return new BoundFunctionPointerInvocation(
node,
boundExpression,
analyzedArguments.Arguments.SelectAsArray((expr, args) => args.binder.BindToNaturalType(expr, args.diagnostics), (binder: this, diagnostics)),
BuildArgumentsForErrorRecovery(analyzedArguments, StaticCast<MethodSymbol>.From(methods)),
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
analyzedArguments.RefKinds.ToImmutableOrNull(),
LookupResultKind.OverloadResolutionFailure,
funcPtr.Signature.ReturnType,
Expand Down
18 changes: 17 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ private void PerformMemberOverloadResolution<TMember>(
if (isFunctionPointerResolution)
{
RemoveCallingConventionMismatches(results, callingConvention);
RemoveStaticInstanceMismatches(results, requireStatic: true);
RemoveMethodsNotDeclaredStatic(results);
}

// NB: As in dev12, we do this AFTER removing less derived members.
Expand Down Expand Up @@ -387,6 +387,22 @@ private static void RemoveStaticInstanceMismatches<TMember>(ArrayBuilder<MemberR
}
}

private static void RemoveMethodsNotDeclaredStatic<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results) where TMember : Symbol
{
// RemoveStaticInstanceMistmatches allows methods that do not need a reciever but are not declared static,
// such as a local function that is not declared static. This eliminates methods that are not actually
// declared as static
for (int f = 0; f < results.Count; f++)
{
var result = results[f];
TMember member = result.Member;
if (result.Result.IsValid && !member.IsStatic)
{
results[f] = new MemberResolutionResult<TMember>(member, result.LeastOverriddenMember, MemberAnalysisResult.StaticInstanceMismatch());
}
}
}

private void RemoveConstraintViolations<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results) where TMember : Symbol
{
// When the feature 'ImprovedOverloadCandidates' is enabled, we do not include methods for which the type arguments
Expand Down
12 changes: 8 additions & 4 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ private void EmitExpressionCore(BoundExpression expression, bool used)
break;

case BoundKind.FunctionPointerLoad:
EmitLoadFunction((BoundFunctionPointerLoad)expression);
EmitLoadFunction((BoundFunctionPointerLoad)expression, used);
break;

default:
Expand Down Expand Up @@ -3466,11 +3466,15 @@ private void EmitCallCleanup(SyntaxNode syntax, UseKind useKind, MethodSymbol me
}
}

private void EmitLoadFunction(BoundFunctionPointerLoad load)
private void EmitLoadFunction(BoundFunctionPointerLoad load, bool used)
{
Debug.Assert(load.Type is { TypeKind: TypeKind.FunctionPointer });
_builder.EmitOpCode(ILOpCode.Ldftn);
EmitSymbolToken(load.TargetMethod, load.Syntax, optArgList: null);

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

@AlekseyTs AlekseyTs Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
}
27 changes: 27 additions & 0 deletions src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,33 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
return base.VisitDelegateCreationExpression(node);
}

public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
if (node.TargetMethod.MethodKind == MethodKind.LocalFunction)
{
Debug.Assert(node.TargetMethod is { RequiresInstanceReceiver: false, IsStatic: true });
ImmutableArray<BoundExpression> arguments = default;
ImmutableArray<RefKind> argRefKinds = default;

RemapLocalFunction(
node.Syntax,
node.TargetMethod,
out BoundExpression receiver,
out MethodSymbol remappedMethod,
ref arguments,
ref argRefKinds);

Debug.Assert(arguments.IsDefault &&
argRefKinds.IsDefault &&
receiver.Kind == BoundKind.TypeExpression &&
remappedMethod is { RequiresInstanceReceiver: false, IsStatic: true });

return node.Update(remappedMethod, node.Type);
}

return base.VisitFunctionPointerLoad(node);
}

public override BoundNode VisitConversion(BoundConversion conversion)
{
// a conversion with a method should have been rewritten, e.g. to an invocation
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

It looks like we should also visit type. #Closed

}

public override BoundNode VisitLoweredConditionalAccess(BoundLoweredConditionalAccess node)
{
BoundExpression receiver = (BoundExpression)this.Visit(node.Receiver);
Expand Down
Loading