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

Jump to bottom

Support ldftn #42488

Merged
8 commits merged into from
Mar 28, 2020
Merged

Support ldftn #42488

8 commits merged into from
Mar 28, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 17, 2020

This change enables an address-of operator (&) to target a method group,
and for that result to be converted a function pointer type. Much like a
method-group-to-delegate conversion, an address-of-method-group
has no natural type, and must be target-typed to something with a
compatible function pointer type signature. Compatible is defined in
our function pointer spec here:
https://github.com/dotnet/csharplang/blob/master/proposals/function-pointers.md#function-pointer-conversions
An address of a method group, if valid, will load the function with the
ldftm CIL instruction, and this token can then be calli'd. This PR
does not implement support for NativeCallableAttribute, that will
come in a future PR. When that does, it will change the calling
convention that the method can be converted to.

@AlekseyTs @dotnet/roslyn-compiler for review.

@333fred 333fred requested review from AlekseyTs and a team March 17, 2020 00:01
@gafter
Copy link
Member

gafter commented Mar 17, 2020

Completed review (Iteration 1)

gafter
gafter approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

Looks good, though please add a comment in the code where I said "You should not permit a variant conversion when there is a ref kind other than none." to indicate where that check is performed.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 18, 2020 •
edited
Loading 

                    if (resolution.MethodGroup.IsExtensionMethodGroup)

Are extension methods Ok for a function pointer conversion? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs:164 in de55633. [](commit_id = de556330aa2d97a36ed6d225429454574c84ae3c, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 18, 2020 •
edited
Loading

Done with review pass (iteration 2) #Closed

@333fred
Copy link
Member Author

333fred commented Mar 19, 2020

Are extension methods Ok for a function pointer conversion?

In fully-qualified form, they should be fine. In reduced form, I'm of the opinion that we shouldn't allow them (as we don't allow instance methods).

@333fred
Copy link
Member Author

333fred commented Mar 20, 2020

@AlekseyTs addressed feedback.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 24, 2020 •
edited
Loading

Done with review pass (iteration 5) #Closed

@333fred
Copy link
Member Author

333fred commented Mar 25, 2020 •
edited
Loading

@AlekseyTs addressed feedback. The failing tests are due to effectively a race condition in what methods are emitted first, so the token number changes (from 0x4 to 0x5 or the other way around). I'm working on getting the Visualizer update in so it will just print the delegate type and we'll be done with it.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 25, 2020 •
edited
Loading

The failing tests are due to effectively a race condition in what methods are emitted first, so the token number changes (from 0x4 to 0x5 or the other way around).

This indicates that we have a non-deterministic emit. This is likely a real problem. Is this related to function pointers in any way? #Closed

AlekseyTs
AlekseyTs reviewed Mar 25, 2020
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs
comp.VerifyDiagnostics(
// (10,35): error CS8757: No overload for 'M1' matches function pointer 'delegate*<C,void>'
// delegate*<C, void> ptr1 = &c.M1;
Diagnostic(ErrorCode.ERR_MethFuncPtrMismatch, "&c.M1").WithArguments("M1", "delegate*<C,void>").WithLocation(10, 35),
Copy link
Contributor

Choose a reason for hiding this comment

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

ERR_MethFuncPtrMismatch [](start = 37, length = 23)

Shouldn't we get ERR_CannotUseReducedExtensionMethodInAddressOf here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't even get to that point in this codepath: overload resolution for extension methods does not succeed. I'd have to modify the rules around resolution in order to add the method to be considered, and I don't think that's worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't even get to that point in this codepath: overload resolution for extension methods does not succeed. I'd have to modify the rules around resolution in order to add the method to be considered, and I don't think that's worth it.

Why do we even try to do overload resolution in this case? We know that no extension method with receiver is going to be accepted, no matter what. I think we should consider improving diagnostic experience around extension methods later.

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 26, 2020 •
edited
Loading

Done with review pass (iteration 6) #Closed

@333fred
Copy link
Member Author

333fred commented Mar 26, 2020

@AlekseyTs addressed feedback.

AlekseyTs
AlekseyTs approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 8), modulo the test issue.

@333fred 333fred requested a review from gafter March 26, 2020 16:26
gafter
gafter approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

Looks good.

@333fred
Copy link
Member Author

333fred commented Mar 26, 2020

Alright. After #42799 is merged, I'm going to rebase this on top of it.

@333fred
Support ldftn
2908863 
This change enables an address-of operator (&) to target a method group,
and for that result to be converted a function pointer type. Much like a
method-group-to-delegate conversion, an address-of-method-group
has no natural type, and must be target-typed to something with a
compatible function pointer type signature. Compatible is defined in
our function pointer spec here:
https://github.com/dotnet/csharplang/blob/master/proposals/function-pointers.md#function-pointer-conversions
An address of a method group, if valid, will load the function with the
ldftm CIL instruction, and this token can then be `calli`'d. This PR
does not implement support for `NativeCallableAttribute`, that will
come in a future PR. When that does, it will change the calling
convention that the method can be converted to.
@333fred
PR Feedback
4cc0ba0 
Some spelling changes as requested.
Fixed a bug by specifying the parameter name to PerformOverloadResolution
Updated baselines
Added a test for `void*`
@333fred
Address PR Feedback:
77b08e3 
Handle additional conversion cases.
Clean up error reporting.
Add more tests.
Handle a couple of bad-code crashes.
@333fred
Readd parameter check.
a3ac488
@333fred
PR Feedback
d8440d9 
Additional calling convention validation.
Test adjustments.
@333fred
PR Feedback
c196e3b 
Adjust some error messages and reporting for clarity.
Add new test for invalid metadata.
@333fred
Addressed PR feedback
28bc3db 
Error wording.
Simply if.
ghost
ghost approved these changes Mar 27, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 66e5fc8 into dotnet:features/function-pointers Mar 28, 2020
@333fred 333fred deleted the method-conversions branch March 28, 2020 00:27
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar left review comments

@gafter gafter gafter approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers auto-merge Feature - Function Pointers Adding Function Pointers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@333fred @gafter @AlekseyTs @jaredpar