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

Update the calling convention and UnmanagedCallersOnly attribute. #3441

Merged
merged 4 commits into from
Jul 1, 2020

Conversation

333fred
Copy link
Member

@333fred 333fred commented May 8, 2020

This adds the changes to extensible calling conventions and support for UnmanagedCallersOnlyAttribute.

the method and then invoke that pointer.
* It is an error to apply the attribute to anything other than a static method. The C# compiler will mark any non-static
methods imported from metadata with this attribute as unsupported by the language.
* It is an error to have non-blittable types as parameters or the return type of a method marked with the attribute.
Copy link
Member

Choose a reason for hiding this comment

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

The language doesn't, afaik, have a concept of "blittable" today; only "unmanaged". Is it introducing the former concept or using the latter?

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 mean to write unmanaged, will fix.

Copy link
Member

Choose a reason for hiding this comment

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

Will the same apply to unmanaged function pointers or just UnmanagedCallersOnlyAttribute?

// This method will be invoked using the stdcall calling convention
delegate* stdcall<int, int>;
// This method will be invoked using the stdcall calling convention, and suppresses GC transition
delegate* unmanaged[CallConvStdCall, CallConvSuppressGCTransition] <int, int>;
Copy link
Member

Choose a reason for hiding this comment

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

I take it we are not allowing unmanaged[StdCall, SuppressGCTransition] then? I don't remember a decision being made here, but I may have missed that bit 😄

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 went for simplicity of implementation, I'll add a consideration note.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, it's mostly a convenience thing but I think it would help greatly with readability and with typing when you have hundreds of function pointers you are declaring/reviewing (such as DirectX which is all COM ThisCall + StdCall)...

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -169,7 +178,7 @@ In an unsafe context, a method `M` is compatible with a function pointer type `F
- For each `ref`, `out`, or `in` parameter, the parameter type in `M` is the same as the corresponding parameter type in `F`.
Copy link
Member

Choose a reason for hiding this comment

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

D is mentioned above. Should that be updated to F?

modopt on the return type of the function pointer. For any other `CallConv` type or combination of such types, the compiler
emits a calling convention bit of Unmanaged (0x9), and encodes all `CallConv` types specified as the first modopts on the return
type of the function pointer. What calling conventions and combinations are valid is determined by the runtime and platform of
final execution, and no validation is performed that a valid combination of calling conventions has been specified beyond
Copy link
Member

Choose a reason for hiding this comment

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

What calling conventions and combinations are valid is determined by the runtime and platform of final execution

/cc @lambdageek

@333fred 333fred changed the title Update the calling convention and UnamangedCallersOnly attribute. Update the calling convention and UnmanagedCallersOnly attribute. May 9, 2020
Comment on lines +56 to +57
// This method will be invoked using the stdcall calling convention, and suppresses GC transition
delegate* unmanaged[CallConvStdCall, CallConvSuppressGCTransition] <int, int>;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is a distinction between the calling convention (like StdCall) and calling convention "modifier" (like SuppressGCTransition), but this proposal ignores that.

Would it make sense to somehow perform basic checking based on that, so that e.g. the nonsensical unmanaged[CallConvCdecl, CallConvStdCall] did not compile? Even if the compiler didn't understand exactly which combinations of calling conventions and their modifiers are allowed, I think such basic checking could still be worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

It's not readily possible to know what combinations are valid and what are not or what will/wont be in the future. For example thiscall can be combined with basically any other convention is in some cases (thiscall+stdcall is the default for x86 COM, for example).

It's easier to just allow any combination and let the runtime decide.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to somehow perform basic checking based on that

@svick This concern was raised during design. The reason for not checking is an attempt to avoid having to update the compiler whenever a new combination is supported by the runtime. As @tannergooding mentions there are some combinations that could be described as a combination of multiple CallConv* types. Letting the implementing runtime decide what it supports avoids encoding a support matrix in more than one location (e.g. Roslyn and runtime) and lessens the time to market for consumption of those combinations.

The counter argument here is that deferring the error until run time is less than ideal for confidence in a compiled assembly. We can all appreciate that concern. The benefit of quicker adoption of desired combinations in an already advanced interop scenario outweighed the cost of degraded compile time validation. This is why the decision was made.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT @tannergooding I get that hardcoding the full support matrix into the compiler is undesirable.

What I was proposing is that there would be two kinds of CallConv: "base" (like Cdecl or StdCall) and "modifier" (like Thiscall or SuppressGCTransition). An unmanaged function pointer could then have:

  • Zero or one base CallConv.
  • Zero or more modifier CallConv.

I think that this way, all valid combinations will still be allowed while many invalid combination will be disallowed. Or am I wrong?

Though thinking about it some more, I think it would actually be possible to encode the support matrix into the metadata of the corelib reference assembly: each base CallConv could have an attribute that specified the modifier CallConvs that it allows, or even the combination of modifier CallConv it allows.

Using this approach, the compiler can know the support matrix, even though altering the support matrix does not involve updating the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

That would still require updating the compiler any time a new convention is released. Having an analyzer that flags potentially bad or unsupported combinations is likely a much better alternative as it is more trivially updated, shipped, and ignored (if desired).

The support might also differ across various runtime implementations. That is, while CoreCLR might support X, Y, and Z; you might have another runtime like Unity or a CIL to LLVM transpiler which supports a different set (or different combinations).

Copy link
Contributor

Choose a reason for hiding this comment

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

@tannergooding

That would still require updating the compiler any time a new convention is released.

It wouldn't, but I forgot to explain that: the compiler would differentiate between base and modifier CallConv based on metadata. That could be represented by a base class of the CallConv class (e.g. public class CallConvThiscall : ModifierCallConv { … }), or an attribute applied to the CallConv class.

The support might also differ across various runtime implementations.

If they use the same corlib reference assembly, yes, that would be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

That would still require updating the compiler any time a new convention is released.

I've thought a lot about this and I keep asking myself the following: is that so bad? The compiler and runtime release as a pair now. Updating the calling convention is a small amount of work, or would be if we designed for this future. So is it really that bad if we said that every time the runtime adds a new calling convention that we have to change the compiler to support it?

Copy link
Member

Choose a reason for hiding this comment

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

There are many runtime failure modes for interop that have complex conditions and that the compiler does not check for today. One example from many - the compiler does not check whether MarshalAs makes sense for given argument type in regular PInvokes. We could duplicate the runtime checks for all these in the compiler, but the value of it seems very low. I do not think that the support matrix for the unmanaged calling convention modifiers should be on different plan from the rest of interop.

@@ -78,6 +88,7 @@ Restrictions:
- Custom attributes cannot be applied to a `delegate*` or any of its elements.
- A `delegate*` parameter cannot be marked as `params`
- A `delegate*` type has all of the restrictions of a normal pointer type.
- Pointer arithmetic cannot be performed directly on pointer types.
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

on pointer types [](start = 50, length = 16)

On function pointer types? #Closed

@@ -114,7 +121,9 @@ funcptr_return_modifier
```

The `unmanaged` calling convention represents the default calling convention for native code on the current platform, and is encoded as winapi.
All `calling_convention`s are contextual keywords when preceded by a `delegate*`.
All `identifer`s in the `unmanaged` case of the `calling_convention_specifier` must start with `CallConv`, and come from the
`System.Runtime.CompilerServices` namespace in the program's core library (defined as one library that references no dependencies and defines
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

Identifiers cannot really come from a namespace. Perhaps we should say that the identifier must match the name of a type in that namespace instead? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Instead of come from I suggest bind to a type in


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

* `System.Runtime.CompilerServices.CallConvThiscall`

If the identifier specified for the `unmanaged` calling convention binds to one of these types, then the function pointer
is emitted with the calling convention bit set to the appropriate value, and encodes the calling convention type as the first
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

with the calling convention bit set to the appropriate value [](start = 11, length = 60)

We should be specific what is the value. #Closed


If the identifier specified for the `unmanaged` calling convention binds to one of these types, then the function pointer
is emitted with the calling convention bit set to the appropriate value, and encodes the calling convention type as the first
modopt on the return type of the function pointer. For any other `CallConv` type or combination of such types, the compiler
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

Regarding "as the first modopt". I do not believe there is such a requirement. I think we should simply say that the modopt is added to the set of custom modifiers at the beginning of the function pointer signature. #Closed


If the identifier specified for the `unmanaged` calling convention binds to one of these types, then the function pointer
is emitted with the calling convention bit set to the appropriate value, and encodes the calling convention type as the first
modopt on the return type of the function pointer. For any other `CallConv` type or combination of such types, the compiler
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

on the return type [](start = 7, length = 18)

"on the return type" is also not accurate, the modifier can be on ref rather than on type. #Closed

If the identifier specified for the `unmanaged` calling convention binds to one of these types, then the function pointer
is emitted with the calling convention bit set to the appropriate value, and encodes the calling convention type as the first
modopt on the return type of the function pointer. For any other `CallConv` type or combination of such types, the compiler
emits a calling convention bit of Unmanaged (0x9), and encodes all `CallConv` types specified as the first modopts on the return
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

as the first modopts on the return [](start = 94, length = 34)

Same comment here. And we shouldn't specify any relative order for modifiers because in case of overriding/implementing the syntax doesn't really define the order. #Closed

type of the function pointer. What calling conventions and combinations are valid is determined by the runtime and platform of
final execution, and no validation is performed that a valid combination of calling conventions has been specified beyond
ensuring that all calling convention types come from the right namespace in the core library of the program. If the return
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

I do not believe this is accurate. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

No, this isn't accurate. These modifiers must directly follow ParamCount in the method signature.

ensuring that all calling convention types come from the right namespace in the core library of the program. If the return
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.

When interpreting a function pointer in metadata, all modopts that start with `CallConv` and come from the
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

all modopts [](start = 50, length = 11)

I think this doesn't quite match the latest proposal discussed on the internal thread. #Closed

* It is an error to have non-blittable types as parameters or the return type of a method marked with the attribute.
* It is an error to convert a method marked with the attribute to a delegate type.
* The calling convention specified by the attribute contributes to the calling convention the compiler recognizes when
a method is converted to a function pointer.
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

How exactly compiler recognizes them, what do they affect? How the attribute is defined? #Closed

@@ -169,7 +178,7 @@ In an unsafe context, a method `M` is compatible with a function pointer type `F
- For each `ref`, `out`, or `in` parameter, the parameter type in `M` is the same as the corresponding parameter type in `F`.
- If the return type is by value (no `ref` or `ref readonly`), an identity, implicit reference, or implicit pointer conversion exists from the return type of `F` to the return type of `M`.
- If the return type is by reference (`ref` or `ref readonly`), the return type and `ref` modifiers of `F` are the same as the return type and `ref` modifiers of `M`.
- The calling convention of `M` is the same as the calling convention of `F`.
- The calling convention of `M` is the same as the calling convention of `F`. This includes both the calling convention bit, as well as any calling convention flags specified in the unmanaged identifier.
Copy link
Contributor

@AlekseyTs AlekseyTs May 9, 2020

Choose a reason for hiding this comment

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

This includes both the calling convention bit, as well as any calling convention flags specified in the unmanaged identifier. [](start = 78, length = 125)

I think this doesn't quite match the last proposal discussed on the internal thread. We need to specify precise rules for determining if calling conventions are compatible. Also, I am not sure if talking about that in syntactic terms ("calling convention flags specified in the unmanaged identifier") is the right approach. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented May 9, 2020

Done with review pass (iteration 2) #Closed

ensuring that all calling convention types come from the right namespace in the core library of the program. If the return
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.

When interpreting a function pointer in metadata, all modopts that start with `CallConv` and come from the
Copy link
Member

Choose a reason for hiding this comment

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

Is this truly "all modopts" or is it ones that appear in specific locations?

type of the function pointer. What calling conventions and combinations are valid is determined by the runtime and platform of
final execution, and no validation is performed that a valid combination of calling conventions has been specified beyond
ensuring that all calling convention types come from the right namespace in the core library of the program. If the return
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.
type of a pointer is by `ref`, then all calling conventions will be encoded after the `ref` specifier.

`System.Runtime.CompilerServices` namespace in the core library are considered to be part of the calling convention of the
function pointer.

### UnmanagedCallersOnlyAttribute
Copy link
Member

Choose a reason for hiding this comment

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

Consider listing the FQN of this type in this section.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Please fix the custom modifier location in the method signature signature.

type of the function pointer. What calling conventions and combinations are valid is determined by the runtime and platform of
final execution, and no validation is performed that a valid combination of calling conventions has been specified beyond
ensuring that all calling convention types come from the right namespace in the core library of the program. If the return
type of a pointer is by `ref`, then all calling conventions will be encoded after the ref specifier.
Copy link
Member

Choose a reason for hiding this comment

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

No, this isn't accurate. These modifiers must directly follow ParamCount in the method signature.

@333fred
Copy link
Member Author

333fred commented Jun 30, 2020

@AlekseyTs @davidwrighton @jkotas @jaredpar I've updated the proposal with the latest design decisions. Please let me know if there's any more feedback.

signature. As a note, these rules mean that users cannot prefix these `identifier`s with `CallConv`, as that will result
in looking up `CallConvCallConvVectorCall`.

When interpreting metadata, the we first look at the `CallKind`. If it is anything other than `unmanaged ext`, we ignore all
Copy link
Member

Choose a reason for hiding this comment

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

the we -> "we"


It is an error to attempt to use a function pointer with a `CallKind` of `unmanaged ext` if the target runtime does not
support the feature. This will be determined by looking for the presence of the
`System.Runtime.CompilerServices.RuntimeFeature.UnmanagedCallKind` constant. If this constant is present, the runtime is
Copy link
Member

Choose a reason for hiding this comment

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

I thought the presence of the UnmanagedCallersOnlyAttribute was going to be enough here?

Copy link
Member

Choose a reason for hiding this comment

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

I see the note below.

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 don't particularly want to condition the support based on that attribute, as I think they're orthogonal and we shouldn't tie them together, but we can discuss that Thursday in design review :).

- For each value parameter (a parameter with no `ref`, `out`, or `in` modifier), an identity conversion, implicit reference conversion, or implicit pointer conversion exists from the parameter type in `M` to the corresponding parameter type in `F`.
- For each `ref`, `out`, or `in` parameter, the parameter type in `M` is the same as the corresponding parameter type in `F`.
- If the return type is by value (no `ref` or `ref readonly`), an identity, implicit reference, or implicit pointer conversion exists from the return type of `F` to the return type of `M`.
- If the return type is by reference (`ref` or `ref readonly`), the return type and `ref` modifiers of `F` are the same as the return type and `ref` modifiers of `M`.
- The calling convention of `M` is the same as the calling convention of `F`.
- The calling convention of `M` is the same as the calling convention of `F`. This includes both the calling convention bit, as well as any calling convention flags specified in the unmanaged identifier.
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 1, 2020

Choose a reason for hiding this comment

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

This includes both the calling convention bit, as well as any calling convention flags specified in the unmanaged identifier. [](start = 78, length = 125)

I think we would want to expand on that. In particular, I think we should have a special handling of CallConvCDecl modifier (and the like "special" legacy modifiers) in presence of a cdecl bit, as apposed to in presence of unmanaged bit (what ever the new bit we are going to use). One of the reasons, is that, I think, we would want to support overriding a method with a cdecl with modifier function pointer (C++ creates those) by a method with just cdecl function pointer in it. While looking for an override, we should make sure calling conventions match, once we find the matching override, the normal process will inherit the modifiers. Same goes for interface implementations. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you expanded on that in a section below. Looks reasonable.


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

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 3)

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM and matches what I recall from the discussion

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

This looks right. I'm excited to see this feature finish up!


// This method will be invoked using whatever the default unmanaged calling convention on the runtime
// platform is. This is platform and architecture dependent and is determined by the CLR at runtime.
delegate* unmanged<int, int>;
Copy link
Member

Choose a reason for hiding this comment

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

unmanaged should be spelled unmanaged not unmanged. There are several places with this error.


// This method will be invoked using whatever the default unmanaged calling convention on the runtime
// platform is. This is platform and architecture dependent and is determined by the CLR at runtime.
delegate* unmanged<int, int>;
Copy link
Member

Choose a reason for hiding this comment

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

unmanged should be unmanaged, both here, and in the next example.

@333fred 333fred merged commit 1bb4548 into dotnet:master Jul 1, 2020
@333fred 333fred deleted the update-func-ptrs branch July 1, 2020 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants