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.

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

Open Issues in "Function Pointers" #39865

Closed
17 of 18 tasks
333fred opened this issue Nov 18, 2019 · 43 comments · Fixed by #51038
Closed
17 of 18 tasks

Open Issues in "Function Pointers" #39865

333fred opened this issue Nov 18, 2019 · 43 comments · Fixed by #51038

Comments

@333fred
Copy link
Member

333fred commented Nov 18, 2019

Tracking issue for open function pointer questions:

Compilers

IDE

Runtime changes

@tannergooding
Copy link
Member

@jkotas, @AaronRobinsonMSFT, @jkoritzinsky.

Could one of you comment on the custom attributes question and how this would work or if it is something that will likely never be supported?

@jkotas
Copy link
Member

jkotas commented Mar 9, 2020

It is likely that will need to support new calling conventions, or flags on the existing calling conventions, in future. The current design does not have an obvious way to do that is unfortunate.

There are two parts of the problem:

  1. The calling convention keywords are part of the C# language. Adding new calling convention requires modification of the C# compiler. My proposal was fix this by leveraging CallingConvention enum to specify the calling convention, but @jaredpar and @333fred explained to me that it is hard/impossible. However that was before the latest iteration of the function pointer syntax. Is it still as hard with the latest function pointer syntax? For example, can something like delegate * unmanaged(CallingConvention.CDecl) <int, int> work?

  2. Encoding of the new calling conventions in metadata. The current encoding has zero extensible, so we would need to add that extensibility for anything we do here. The new encoding may or may not be based on custom attributes, there are options. The existing legacy calling conventions can be encoded using the current non-extensible encoding as a special case in the metadata emitter, so that one can use the function pointers on existing runtimes.

@jkotas
Copy link
Member

jkotas commented Mar 9, 2020

Just to expand on my example in 1:

The expression following unmanaged would be required to evaluate as a constant, so that the calling convention and the flags can be combined together, e.g.:

delegate * unmanaged(CallingConvention.CDecl | CallingConvention.SuppressGCTransition) <string, int>

@jkotas
Copy link
Member

jkotas commented Mar 9, 2020

Also, I think doing something about 1 now is more important than doing something about 2 now. We can add 2 later without introducing ugly user facing warts. But if we do not do something about 1 now, we will likely need to always change Roslyn for any calling convention addition.

@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

For 1, I think we can make something work if we get a definition in ECMA-335 for extensible calling conventions. The proposal I'd have would be something like this:

  • 335 defines a calling convention bit that means "Look in a modreq on the return type for the convention".
  • We define the set of conventions in C# as platform dependent.
  • The implementation will look at the System.Runtime.CompilerServices.CallingConvention namespace, or whatever namespace of types the runtime wants us to look at.
    • Each element of the CallingConvention namespace is attributed with some well-known attribute that we look at to determine what the corresponding C# keyword should be.
    • If we can match the keyword typed with one of the conventions, we use that convention.
    • We emit a StandaloneMethodSig with the aforementioned "Look for a modreq" calling convention, and emit a modreq on the return type with the mapped type.
    • If that namespace doesn't exist (ie older platforms) or the calling convention doesn't exist in the namespace, we fall back to the hardcoded list we have today.

We could also potentially use this format to encode things like SuppressGCTransition. If we want to start marking things other than the calling convention, though, I do think we'll want some new syntax form.

For both of these things, I personally see them as things that can happen after we ship the initial version of function pointers. The extensible calling convention bit is transparent to users, and the SuppressGCTransition stuff will need new syntactic forms that will need to go through LDM.

@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

/cc @AlekseyTs as well.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2020

We define the set of conventions in C# as platform dependent.

What do you mean by platform in this context? Platform typically means OS, but I do not think you meant that.

The implementation will look at the System.Runtime.CompilerServices.CallingConvention namespace, or whatever namespace of types the runtime wants us to look at.

These attributes do exist for managed C++ already (System.Runtime.CompilerServices.CallConvCdecl, etc.). Can we reuse those? E.g. the compiler would look at all types under System.Runtime.CompilerServices with names starting with CallConv.

Each element of the CallingConvention namespace is attributed with some well-known attribute that we look at to determine what the corresponding C# keyword should be.

The fewer names we have for the same thing the better. Can it be 1:1 mapping? E.g. assuming we would use the existing types: CallConvCdecl -> the C# keyword is Cdecl, CallConvThiscall -> the C# keyword is Thiscall, etc.

If we want to start marking things other than the calling convention, though, I do think we'll want some new syntax form.

What is the new syntax form you would propose?

the SuppressGCTransition stuff will need new syntactic forms that will need to go through LDM.

I am worried that the new syntactic form won't work well with the shipped syntactic form.

@jaredpar
Copy link
Member

I think we need to settle on particular aspect: is the explicit goal to support attributes like SuppressGCTransition in the future? So far the only suggestion i've seen that it's a core scenario is a comment from @tannergooding in a PR that was left dangling for several months and had no backing documentation, feature links, etc ... to indicate its priority.

If this is an explicit future goal then we need to plan for it. Neither our current syntax form nor the underlying runtime ABI that we depend on will support it (to my understanding at least). If this is an explicit request does the runtime have a proposal for the underlying ABI changes that we can look at to evaluate the path forward?

Appologies if this has been communicated in a forum I'm unaware of (or if I just deleted the email on accident). 😦

@AaronRobinsonMSFT
Copy link
Member

I think we need to settle on particular aspect: is the explicit goal to support attributes like SuppressGCTransition in the future? So far the only suggestion i've seen that it's a core scenario is a comment from @tannergooding in a PR that was left dangling for several months and had no backing documentation, feature links, etc ... to indicate its priority.

@jaredpar I think that is a very fair assessment.

The general feeling is that we know there are a myriad of calling conventions that we may want to support. Which ones is basically an unknown, but the fact they exist is something we want users to be able to communicate to the runtime through the function pointer syntax since language features aren't simple to quickly get through. So this is merely for let's make sure all calling conventions have a possible path forward.

The above is probably an agreed upon stance with little debate.

The concept of additional attributes (e.g. SuppressGCTransition) similarly falls into this bucket because the function pointer syntax doesn't seem to have an obvious place for that data and I think we are trying to anticipate where this additional syntax and metadata would go. Since calling conventions are already presented in an attribute form, having a generic "add attribute to function pointer" for SuppressGCTransition as well is the example. If we could have a design that satisfied calling conventions through existing attribute mechanisms then why not any attribute? I think that is the beginning and end of why this is coming up.

I don't think we have any additional example other than the existing SuppressGCTransition. @jkotas may have some forward looking idea.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2020

Agree with @AaronRobinsonMSFT

SuppressGCTransition

This is a new attribute that we have not shipped as stable yet. We have still opportunity to change how it is done to work better with the other features if necessary.

If we could have a design that satisfied calling conventions through existing attribute mechanisms then why not any attribute?

Attributes cannot be represented in natural way to function pointers in IL metadata today. It is what makes the attribute-centric approach problematic...

@jkoritzinsky
Copy link
Member

I will say that I do like the extensibility of the modreq approach presented by @333fred since it can cover calling conventions as well as parameterless attributes (since an attribute A with no parameters could be emitted as a modreq(A) with no data loss) which would encompass the SuppressGCTransitionAttribute and has no extra limitations on number of supported conventions like an enum-based design would. However, I also agree that we need a way to add new calling conventions at the runtime level without having to come back to LDM to get new syntax or new context-sensitive keywords approved.

I think @jkotas' design around defining a simple mapping from a "context-sensitive keyword" to a modreq of a type in System.Runtime.CompilerServices named CallConv[keyword with first letter capitalized] that doesn't have a defined closed list at the language level would work well if it doesn't complicate the parser.

@tannergooding
Copy link
Member

as well as parameterless attributes (since an attribute A with no parameters could be emitted as a modreq(A) with no data loss) which would encompass the SuppressGCTransitionAttribute

I think it can be used to cover the calling convention, but I don't think it can be used to cover other attributes. modreq generally become part of the signature, in which case adding an attribute becomes a breaking change (which is fine for the calling convention, but I would imagine is undesirable for other attributes).

@jkoritzinsky
Copy link
Member

as well as parameterless attributes (since an attribute A with no parameters could be emitted as a modreq(A) with no data loss) which would encompass the SuppressGCTransitionAttribute

I think it can be used to cover the calling convention, but I don't think it can be used to cover other attributes. modreq generally become part of the signature, in which case adding an attribute becomes a breaking change (which is fine for the calling convention, but I would imagine is undesirable for other attributes).

Since any attributes on function pointers would only have any effect at the call site, I think they should be part of the function pointer type. Otherwise the attribute could be implicitly lost through assignment or being passed as a parameter.

@tannergooding
Copy link
Member

Since any attributes on function pointers would only have any effect at the call site, I think they should be part of the function pointer type. Otherwise the attribute could be implicitly lost through assignment or being passed as a parameter.

Where would that be encoded in metadata?

There isn't an actual typedef for fnptr like there is for classes/value types, instead it is encoded inline as:

Type ::=
| FNPTR MethodDefSig
| FNPTR MethodRefSig

and I don't believe you can attach an attribute to a MethodDefSig, just a MethodDef...

So the problem is the user has void Method(delegate* unmanaged<void> fnptr) and they want to mark fnptr as SuppressGCTransitionAttribute to avoid the transition for a cheap call, but if it is a modreq/modopt, it changes the signature and becomes breaking.

The CLI itself shall treat required and optional modifiers in the same manner. Two signatures that differ only by the addition of a custom modifier (required or optional) shall not be considered to match.
...
[Note: Unlike parameter attributes, custom modifiers (modopt and modreq) are part of the signature. Thus, modifiers form part of the method’s contract while parameter attributes do not. end note]

There is a section on assignment compatibility, which (if I read it correctly) says the modreq can be ignored, but I don't think that would change the signature compatibility of a method between v1 and v2 of a library.

@jaredpar
Copy link
Member

@tannergooding do you have a documentation link, an issue, etc ... anything concrete you can show us that describes SuppressGCTransitionAttribute? You originally raised this as an issue but haven't really given us much to go on here. The attribute is a 100% a mystery to my team.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2020

dotnet/runtime#30741

@jaredpar
Copy link
Member

I do understand the issues around calling convention and extensibility. I think my team has enough to go on there to look for better solutions. It's definitely on the list of items we need to solve before RTM. So it's an open issue but one that we understand the ramifications around and can iterate no solve a solution.

I'm less clear on how to move forward on attributes as an extensible feature.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2020

I'm less clear on how to move forward on attributes as an extensible feature.

Right, I agree that attributes do not naturally work for function pointers. They should not be the solution for the encoding this info metadata that we lead with. modops/modreqs sound more workable as the metadata encoding for function pointers.

@333fred 333fred added the Feature - Function Pointers Adding Function Pointers label Mar 10, 2020
@333fred
Copy link
Member Author

333fred commented Mar 10, 2020

What do you mean by platform in this context? Platform typically means OS, but I do not think you meant that.

By platform here I meant runtime.

These attributes do exist for managed C++ already (System.Runtime.CompilerServices.CallConvCdecl, etc.). Can we reuse those?

Sure, I think this would work. The only issue I see is that we have a CallConvFastcall there, but I'm fine with exposing it under the "calling conventions are platform-dependent" statement.

E.g. assuming we would use the existing types: CallConvCdecl -> the C# keyword is Cdecl, CallConvThiscall -> the C# keyword is Thiscall, etc.

C# doesn't capitalize keywords, so I don't think that would work. We could do a pattern-based approach where we substring the CallConv off the front and then call ToLower() on the remaining part of the string, but I do worry about possibly needing to expose a calling convention under multiple names. However, I think that's a bridge we can cross when we come to it.

@333fred
Copy link
Member Author

333fred commented Mar 18, 2020

Meeting notes (with @jaredpar, @AlekseyTs, @jkotas, @AaronRobinsonMSFT, @tannergooding, and @333fred):

  • Calling convention types must be in CorLib. If a use defines something in the appropriate namespace in a different DLL, we will not look at it.
  • Overall we like the proposal:
    • Add a new bit to 335's calling conventions for "this calling convention is defined as a modreq on the return type"
    • The calling convention comes from a type in the System.Runtime.CompilerServices.CallingConvention namespace.
    • You can put 1 and only 1 calling convention on the function pointer.
    • If the calling convention is one of the ones understood by the platform today (managed, thiscall, stdcall, cdecl), we'll use the existing bit.
    • Otherwise, we use the new bit and modreq.
  • We're fine with GCSuppressTransition being part of a type signature, and are actually explicitly in favor of this. It adds constraints to what a call is allowed to do. If a user wants to transition an API from non-suppress transition to suppress transition, they will have to either accept the break or introduce a new API. This parallels Task and ValueTask.
  • Support for GCSuppressTransition can come via a new syntax form to allow multiple things to be specified in the calling convention spot. An initial strawman would be delegate*(native | GCSuppressTransition)<void>. Initial C# preview of this feature will ship without support, and we'll revisit later.

@333fred
Copy link
Member Author

333fred commented Apr 1, 2020

@jkotas @AaronRobinsonMSFT @tannergooding @jaredpar @AlekseyTs one small update to that proposal:

The existing calling convention types are of this format: System.Runtime.CompilerServices.CallConvNAME, so CallConvCdecl, CallConvStdcall, etc. So if we want to continue using those convention types, we'll need to modify the calling convention spec to be:

  • The calling convention comes from a type in the System.Runtime.CompilerServices that starts with CallConv.

Does this make sense to everyone? We'll discuss this point in LDM tomorrow.

@tannergooding
Copy link
Member

That makes sense to me. CC @terrajobst and @dotnet/fxdc as they are the general reviewers/approvers of API design decisions.

@tannergooding
Copy link
Member

tannergooding commented Apr 29, 2020

can you elaborate what you mean? is bool not allowed but you would need an analyzer to get a warning?

bool, AFAIK, is always normalized to 0 or 1 when returned by value and to the target true/false when passed, even if you force it to be marshaled as a single byte.
If you look at ilmarshalers, you'll see there are a number types that inherit from ILBoolMarshaler: https://github.com/dotnet/runtime/blob/ddd458e8beb5ca7711b6082aeb8ed88f6580512e/src/coreclr/src/vm/ilmarshalers.h#L2143
The one used for 1 byte bool is ILCBoolMarshaler: https://github.com/dotnet/runtime/blob/ddd458e8beb5ca7711b6082aeb8ed88f6580512e/src/coreclr/src/vm/ilmarshalers.h#L2188-L2216
However, in all cases there is a stub emitted that normalizes the data by comparing against 0: https://github.com/dotnet/runtime/blob/master/src/coreclr/src/vm/ilmarshalers.cpp#L225-L239

@AaronRobinsonMSFT might be able to comment if there is any edge case I am not thinking about and what behavior calli/UnmanagedCallersOnlyAttribute have if you use bool in the signature (it might be likewise useful to get confirmation for the behavior of char, LayoutKind=Auto, and any other struct types that are or may be restricted).

@weltkante
Copy link
Contributor

I think my confusion comes from calling bool non-blittable yet dotnet/runtime#13772 and dotnet/runtime#1866 started to allow bool as if it were blittable when having a pointer-to-struct-containing-bool. (Just found those today in a different context, so wasn't aware of it back when I asked.)

@jkoritzinsky
Copy link
Member

@333fred I've started using function pointers for C#/WinRT and I've run into an issue. I can't cast an "address-of-method-group" for a non-overloaded static function to void*. It seems that I need to assign it to a variable first before casting. This causes a problem (that I could work around for now but would have to revert) when I'm using [UnmanagedCallersOnly], where I currently have a calling convention mismatch (which is why I'm casting to void* in the first place).

@333fred
Copy link
Member Author

333fred commented Jun 4, 2020

@jkoritzinsky that's intended behavior, and the non-overloaded mention in the spec will be removed when I'm back from vacation next week. You will have to either assign it to a variable or cast it to a compatible function pointer type first.

@jkoritzinsky
Copy link
Member

Ok. That's understandable.

Do you have an ETA on supporting UnmanagedCallersOnly? I'd really like to avoid having to do multiple iterations to work around feature deficiencies since some of the code I'm working on isn't auto-generated.

@tannergooding
Copy link
Member

tannergooding commented Jun 4, 2020

For reference since I missed it the first time as well, void* x = &Method isn't allowed because the compiler can't know which overload of Method to pick and if there was only one method overload it would become source breaking to add a secondary one.
So the target of the &Method has to be a delegate* with the appropriate calling convention and method arguments (and return type?).

It might be nice in some future version to allow var x = &Method(int, int) just like you can do <see cref="Method(int, int)" /> in XML comments. This would presumably also allow void* x = ...
I think this could possibly even be a more general mechanism to pick a method group that would work with delegates....

@333fred
Copy link
Member Author

333fred commented Jun 4, 2020

@tannergooding Eric Lippert actually blogged about that: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/in-foof-we-trust-a-dialogue

@tannergooding
Copy link
Member

Right, it isn't necessarily easy, hence nice, etc 😄

@333fred
Copy link
Member Author

333fred commented Jun 22, 2020

Open issues for LDM 6/24:

Function pointer calling convention syntax:

func_ptr_calling_convention
   : 'managed'
   | 'unmanaged' '[' func_ptr_callkind ']'?

func_ptr_callkind
  : 'CallConvCdecl'
  | 'CallConvStdcall'
  | 'CallConvThiscall'
  | 'CallConvFastcall'
  | identifier (',' identifier)*
  1. Confirm whether we're using special name lookup rules for the calling convention identifiers. Is it that we won't consider anything in the current scope and will instead look in the specific namespace, or will it use the normal lookup rules and we just specify that it must either bind to one of these types or bind to nothing (and be one of the special-cased names).
    • If the answer is that we use standard name lookup, should we allow a full type syntax in the calling convention spot? Or just an identifier? I suspect that if we want to make it standard name lookup, we should allow a full type specifier there.
  2. Confirm whether we allow the CallConv to be dropped. This is orthogonal to 1, but my preference for whether this is allowed or not changes based on the answer to 1.
    • If CallConv is allowed to be dropped, is it allowed at all? Or must it be dropped?

@jkotas
Copy link
Member

jkotas commented Jun 22, 2020

we allow the CallConv to be dropped

What does this mean? When would it happen?

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 22, 2020

What does this mean? When would it happen?

i.e. could you write:

delegate* unmanaged[Cdecl]<...> instead of
delegate* unmanaged[CallConvCdecl]<...>

Note: IMO this syntax is just getting heinous.

@ufcpp
Copy link
Contributor

ufcpp commented Jun 24, 2020

version used:
Visual Studio 16.7.0 Preview 3.0
C# Tools 3.7.0-3.20312.3+ec4841263590f5456e32728d98097e97c1605e22

unsafe class FunctionPointer
{
    public static void M()
    {
        delegate*<void> a = &Static;
        a(); // OK

        a = &local;
        a(); // Method not found: 'Void FunctionPointer.local()'.

        static void local() => System.Console.WriteLine("local");
    }

    static void Static() => System.Console.WriteLine("static");
}

@hypeartist
Copy link

#45418

@333fred
Copy link
Member Author

333fred commented Jun 25, 2020

@ufcpp I've broken that out into a separate issue: #45447. For the future, it'd be great if you can file as a separate issue. I'm using this bug for tracking general outstanding work, and I'd like to keep user-found bugs in separate locations for prioritization. Thanks!

lambdageek added a commit to lambdageek/mono that referenced this issue Jul 8, 2020
A MethodRefSig or a StandaloneSig can have the calling convention "unmanaged"
which is the default platform calling convention with optional modifiers
encoded in the modopts of the return type.

See dotnet/roslyn#39865 (comment)
and dotnet/runtime#34805

Contributes to dotnet/runtime#38480
@sakno
Copy link

sakno commented Dec 4, 2020

Probably, it's reasonable to allow the usage of static anonymous function as a value of function pointer type:

delegate*<int, string> toStr = &static i => i.ToString();

@PathogenDavid
Copy link
Contributor

@sakno That would require a language change. There's an existing proposal for it here: dotnet/csharplang#3476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.