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

Function Pointer Proposal #1951

Merged
merged 7 commits into from
Oct 23, 2018
Merged

Function Pointer Proposal #1951

merged 7 commits into from
Oct 23, 2018

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Oct 23, 2018

This is the function pointer feature reviewed in LDM on 10/15. Notes for that meeting are here. This is an alternate approach to intrinsics for exposing the calli and ldftn instructions.

This is the proposal as discussed in the meeting. Uploading this version so that the notes will make a bit more sense. Will upload a new version, based on the changes discussed in LDM, in the next few days.

@jaredpar
Copy link
Member Author

Merging so I can put good links in the LDM notes. Please continue to provide feedback.

@jaredpar jaredpar merged commit f4433cc into dotnet:master Oct 23, 2018
The conversion of an address-of method group to `funcptr` has roughly the same process as method group to `delegate`
conversion. There are two additional restrictions to the existing process:
- Only members of the method group that are marked as `static` will be considered.
- Only a `funcptr` with a managed calling convention can be the target of such a conversion.
Copy link
Member

Choose a reason for hiding this comment

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

This makes it unusable for interop that is one of the main scenarios for this.

Can we tie this with the NativeCallableAttribute to make it work for interop?

  • If the target method is not marked with NativeCallableAttribute, the result is function pointer with the managed calling convention
  • If the target method is marked with NativeCallableAttribute, the result is function pointer with the unmanaged calling convention specified in the NativeCallableAttribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is pretty doable. We'd likely need to expose that attribute though and make it a supported API

Copy link

Choose a reason for hiding this comment

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

Is NativeCallableAttribute exposed in .NET Framework? This will make this feature not useful on .NET Framework or until we decide to put NativeCallableAttribute in .NET Framework. Any reason why we can't have this for all supported calling conventions with types already present on desktop and .net core?

Copy link
Member

@jkotas jkotas Jan 1, 2019

Choose a reason for hiding this comment

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

Is NativeCallableAttribute exposed in .NET Framework

Nope. (In fact, it is not exposed in the public surface of any shipping runtime today.)

Any reason why we can't have this for all supported calling conventions with types already present on desktop and .net core?

What would you propose? I do not think that the runtime support required to make this work well exists in desktop.


The `calli` instruction requires the calling convention be specified as a part of the invocation. The default
for `funcptr` will be managed. Alternate forms can be specified by adding the appropriate modifier after the
`funcptr` keyword: `cdecl`, `fastcall`, `stdcall`, `thiscall` or `winapi`. Example:
Copy link
Member

@jkotas jkotas Oct 23, 2018

Choose a reason for hiding this comment

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

We should have identifier for the default unmanaged calling convention - what you get using default(CallingConvention) today. Would funcptr unmanaged work?

winapi maps to default unmanaged calling convention, but it is not a very good name because of it sounds Windows-specific. It should be fine to not have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Overall that seems fine. The proposal today though doesn't rely on the type CallingConvention. Is there a different way to calculate the default?

Copy link
Member

@jkotas jkotas Oct 23, 2018

Choose a reason for hiding this comment

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

Hmmm, the enum that describes calling conventions in signatures does not have neither winapi nor value for default unmanaged calling convention:

https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.Reflection.Metadata/src/System/Reflection/Metadata/Signatures/SignatureCallingConvention.cs#L22

Maybe we should consider adding value for the default unmanaged calling convention here? (Of course, it would work on new runtimes only...)

}
```

These types are represented using the function pointer type as outlined in ECMA-335. This means invocation
Copy link
Member

Choose a reason for hiding this comment

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

What is the named funcptr going to look like in the metadata? I know that ECMA-335 allows unnamed funcptrs, but I do not recall named funcptrs.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see that this is in the Open Issues.


This means that it is possible to overload on `void*` and a `funcptr` and still sensibly use the address-of operator.

## Open Issuess
Copy link
Member

Choose a reason for hiding this comment

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

Typo: Issuess

## Detailed Design

### funcptr
The language will allow for the declaration of function pointers using the `funcptr` contextual keyword. The
Copy link

Choose a reason for hiding this comment

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

Hmm, funcptr, kind of ugly for various reasons, including the fact that C#/.NET tends to deal in methods and not functions. And I'm no fan of the C/C++ syntax for function pointers but it's weird that C# data pointers are declared like C/C++ data pointers while C# function pointers, named or unnamed, require this distinct syntax.

And coupled with the names/no names issue, it's even more questionable. This introduces a very specific way to assign a name to an otherwise unnamed type even though such a feature (typedef basically) is requested from time to time. Not saying that C# should get typedef but this seems like a "let's rush things up and get this done" kind of thing.

Maybe funcptr, if added, should be just syntactic sugar for:

unsafe struct Action { 
    void (*_ptr)(); 
    Action(void (*ptr)()) => _ptr = ptr; 
    public void Invoke() => _ptr(); 
}

I guess this kind of shots down half of the proposal. But hey, just my 2 cents :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe funcptr, if added, should be just syntactic sugar for

In this case it can't really be. The function pointer concept is actually a CLI primitive value. The goal is to just directly expose that.

Copy link

Choose a reason for hiding this comment

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

In this case it can't really be. The function pointer concept is actually a CLI primitive value. The goal is to just directly expose that.

Yep, I wasn't very clear. The idea was to expose that primitive value through a more primitive syntax - void (*_ptr)(); or something similar, that doesn't need a keyword that tries to look like a delegate and ends up looking like a typedef.

Choose a reason for hiding this comment

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

I agree this doesn't feel quite right.

I particularly don't like the many new contextual keywords that would have to be added for the calling conventions. This starts to feel like SQL with its piles of keywords that are relevant only in one place (I know these would be contextual in C#, but still). An attribute would be much better IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can reduce the number of keywords by using the calling convention enum to describe the unmanaged calling convention details? And use the variant without parameter to map to the default calling convention (if we do something about it)?

funcptr unmanaged(CallingConvention.Cdecl) int F1()
funcptr unmanaged int F2()

Note that there may be more unmanaged calling conventions coming in future: Windows have introduced vectorcall calling convention some time ago that .NET does not support yet.

- Round tripping function pointer names, as well as parameter names, through metadata will require additional work. The
function pointer type itself is natively supported by CLI but that does not include any names. This is not anticipated
to be a big issue, just needs design work.
- The address-of operator is limited to `static` methods in this proposal. It can be made to work with instance methods
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this would not always work. Static methods and instance methods do not have the same calling convention. static function pointer to instance method cannot be manufactured by pre-pending "this" pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where can I get some more info on how this does and doesn't work?

Copy link
Member

@jkotas jkotas Oct 23, 2018

Choose a reason for hiding this comment

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

The ECMA spec mentions HASTHIS and EXPLICITTHIS that try to describe it.

If we were to make this work, I think we would need yet another "this" calling convention that would be used for function pointers that point to instance methods.

BTW: It works same way as C++:

class MyType
{
public:
	void InstanceMethod();
	static void StaticMethod(MyType*);

	void Test()
	{
		void(*pfn)(MyType*);

		pfn = &InstanceMethod; // Does not compile
		pfn = &StaticMethod; // Compiles fine
	}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkotas

FWIW, this would not always work. Static methods and instance methods do not have the same calling convention. static function pointer to instance method cannot be manufactured by pre-pending "this" pointer.

Why not? What are the cases where it would not work?

Copy link
Member

@jkotas jkotas Oct 24, 2018

Choose a reason for hiding this comment

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

Methods with return buffers. For example:

struct BigStruct { long a,b,c,d,e,f; }

class MyClass {
    BigStruct MyMethod();
}

On Windows x64, MyMethod takes this in rcx and return buffer pointer in rdx.

Static BigStruct pfn(MyClass) function pointer takes return buffer in rcx and MyClass argument in rdx.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkotas Thanks for the explanation.

@xoofx
Copy link
Member

xoofx commented Oct 23, 2018

I'm really glad to see this feature coming 👍

Not a super fan about the introduction about a new keyword funcptr while it's quite explicit, I would have preferred maybe to recycle something like static delegate... but I can see that it would not be as explicit (static referring to storage while here it would be slightly related but still different)... so yep, not sure if we could come up with a better declaration for it...

@ltrzesniewski
Copy link

Why static delegate when you can write unmanaged delegate? 😉

@mikedn
Copy link

mikedn commented Oct 23, 2018

Why static delegate when you can write unmanaged delegate?

Hmm, that seems better indeed.

@Lakritzator
Copy link

Some time ago I was discussing performance of calling native Win32 APIs, and was referred to the project AdvanceDLSupport, especially the documentation on indirect calling.

Using AdvanceDLSupport did improve the performance but it also impacted the way I write my projects.
Can I use any of this to improve DllImport performance, or can this be used internally to assist?

@ltrzesniewski
Copy link

@Lakritzator I did a couple benchmarks here, and my conclusion is that calli isn't faster than P/Invoke when you do P/Invoke right (which means using [SuppressUnmanagedCodeSecurity] on the .NET framework for instance). The runtime still needs to do its stuff at each call.
Also, I found no way to tell it to marshal strings by pointer instead of copying them.

Anyway this proposal aims to let you access calli from C# without the need for external tools or run-time codegen.

@Lakritzator
Copy link

@ltrzesniewski thanks for the super valuable benchmark. I will need to read up on the SuppressUnmanagedCodeSecurityAttribute 👍

@jkotas
Copy link
Member

jkotas commented Oct 23, 2018

AdvanceDLSupport, especially the documentation on indirect calling.

Correct, the indirect calls via calli do not have any fundamental performance advantage over regular DllImport. The performance difference you have seen can be caused by:

@xoofx
Copy link
Member

xoofx commented Oct 23, 2018

Why static delegate when you can write unmanaged delegate?

yep, why not, I like it 👍

@jaredpar
Copy link
Member Author

Couple reasons for not using unmanaged delegate:

  • The LDM notes essentially pushed us even further away from delegates by removing named function pointers altogether. It makes me even more wary of naming them anything delegate as it may set incorrect expectations.
  • The keyword unmanaged is already used in the language and has a substantially different meaning.

The exact syntax for the declarations is still being worked out but I think it will look closer to C style function pointers than C# delegates in the end.

@ltrzesniewski
Copy link

Well, I suggested unmanaged precisely because it's an existing keyword, and it fits this usage IMO. It has a different meaning in the context of generic parameter constraints, but I think this meaning is pretty close. After all, delegates are somewhat the managed equivalent of unmanaged function pointers.

Still, if you get closer to the C syntax and drop names, then it also seems good to me. I don't really like that funcptr thing.

@svick
Copy link
Contributor

svick commented Oct 24, 2018

First of all, why is this the first time we (the community) hear about this proposal? If C# is designed in the open, then I think there should be an issue to discuss each proposal before it goes to the LDM, especially since LDM notes are still sometimes published with a delay. As far as I can tell, there is still no issue for this proposal (this merged PR does not count and I think #191 does not either, since it's a very different proposal).


Second, I feel like this proposal is too heavyweight, especially when it comes to syntax, for such a niche feature. I would much rather have something that doesn't change the syntax much. I think the funcptr void FAction(int a); syntax does that reasonably well. I don't see the need for having calling convention keywords, when an attribute could be used as well (maybe reuse [UnmanagedFunctionPointer]). And I'm not sure syntax for unnamed function pointers is necessary, even if it's closer to CLI.


Third, how would this handle parameter variations that are encoded as attributes for methods and delegates? That includes tuples with names, in, out and dynamic.


Fourth, if you're going to add the unnamed syntax, have you considered the relationship of that with a unified syntax for delegates (#470, #269). On the surface, they look very similar, even if their goals and problems are very different. I wouldn't want to get into a situation in the future where we have one syntax for unnamed function pointers and a different syntax for unnamed delegates.


Fifth, I think the issues with requiring names always are solvable. The proposal doesn't say why exactly interop with existing unnamed function pointers would be an issue, but I think it's cases like overriding.

E.g. you have this code (implemented in IL):

class Base
{
    public virtual void F(funcptr int(int) ptr) {}
}

and you want to override the method without having access to the unnamed syntax. I think it should be possible to override it with a named function pointer like this:

class Derived : Base
{
    funcptr int FP(int arg);
    public override void F(FP ptr) {}
}

The language already sometimes allows overriding with a different type if there's an identity conversion between the types (if the difference is switching dynamic for object), so I think this would not be too much of a stretch.

This might appear cumbersome, but I think that's okay for an edge case of an advanced feature.

the conversion to `object` can't be allowed, it can't be a member of a `class`, etc ... The C# design is to require
`unsafe` for all pointer uses and hence this design follows that.

Developers will still be capable of preventing a _safe_ wrapper on top of `funcptr` values the same way that they do
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to "prevent a safe wrapper"? Did you mean "provide"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep I meant provide

Encoding this in IL is problematic. The underlying value needs to be represented as a pointer yet it also must:

1. Have a unique type to allow for overloads with different function pointer types.
1. Be equivalent for OHI purposes across assembly boundaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's OHI? I found it in dotnet/roslyn#20528, where it means "override, hiding, implementing", but I think it's not a familiar acronym and so it should be expanded here.

Copy link
Member Author

Choose a reason for hiding this comment

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

OHI is the term to describe how method signature are compared when we are doing interface implementation, override and member hiding hookup. More specifically:

class Base {
  protected virtual void M(int i) { ... }
}
class Derived : Base { 
  protected override void M(int j) { ... }
}

The rules for determining if Deride.M is a proper override for Base.M is generally referred to as OHI.

@jaredpar
Copy link
Member Author

@svick

First of all, why is this the first time we (the community) hear about this proposal? If C# is designed in the open, then I think there should be an issue to discuss each proposal before it goes to the LDM,

Generally I like to post my proposals a couple days before LDM meetings for everyone to see. This time though we shuffled around the topics fairly last minute, and as a result I didn't finish the proposal till the morning of the meeting.

I published it in the exact form it had for the LDM discussion vs. adjusting for LDM feedback and then publishing. So there isn't any loss of information here, it will line up with the notes from the meeting and I'm still processing all the feedback left.

since LDM notes are still sometimes published with a delay

The delay these days is generally two days now that @agocke owns notes. The delay for this specific meeting was one week and that's just due to us trying to hit the Dev16 Preview 1 ship dates.

As far as I can tell, there is still no issue for this proposal (this merged PR does not count and I think #191 does not either, since it's a very different proposal).

I didn't file a new issue for this because in many ways it's still #191. This proposal is trying to solve the same set of problems: provide access to calli and ldftn. The mechanism is very different yes but it's still the same issue we're looking at. In general we keep the same issue open in cases like this until we settle on a design.

Note: this is the third proposed implementation of compiler intrinsics, not the second. All three have used #191 as the issue.

I don't see the need for having calling convention keywords, when an attribute could be used as well (maybe reuse [UnmanagedFunctionPointer])

There are several problems problems with the attribute approach:

  1. Conversions are done based on method signatures and there is no place in C# today where attributes factor into such conversions.
  2. Function pointers can't have attributes as a rule. Adding an exception for this case would be strange.
  3. What would the semantic model return for such an attribute? It is not, and cannot, be an attribute in the metadata sense. It would be emitted as part of the signature element of the function pointer. Hence it' would be a very leaky abstraction.
  4. The set of calling conventions is not actually fixed hence an enum isn't a good choice to represent the set of values.

A simple contextual keyword is pretty light weight here.

I would much rather have something that doesn't change the syntax much

LDM disagrees here. The feedback from the second intrinsics proposal was specifically that we'd avoided pretty much any new syntax and as a result the feature felt "cute" vs. well designed. This proposal to give more syntax is a direct result of this previous meeting.

And I'm not sure syntax for unnamed function pointers is necessary, even if it's closer to CLI.

LDM disagrees here. The result of the 10/15 meeting is to remove all names and only have the unnamed form.

Third, how would this handle parameter variations that are encoded as attributes for methods and delegates? That includes tuples with names, in, out and dynamic

Most of these are TBD. I'm pretty sure we can get most of the features like in and out working. Those can be encoded as modopt and modreq and hence can fit into a CLI function pointer usage. The same could be done for dynamic but I'm not sure we're going to find value in doing that. Have some meetings scheduled with CLR team to work out some of these details.

Fourth, if you're going to add the unnamed syntax, have you considered the relationship of that with a unified syntax for delegates

A little bit. I plan on looking at it more later now that the emphasis is on unnamed delegates.

Fifth, I think the issues with requiring names always are solvable.

This is essentially what I suggested in LDM. Still the decision was against names of any kind hence it's a moot point. 😦

@svick
Copy link
Contributor

svick commented Oct 24, 2018

@jaredpar Thanks for the explanations.

I still think a separate issue to discuss this proposal would be nice. If you really want to reuse #191, then I think it's important that its top post is updated and also a comment added.

Right now, people who follow #191 or who read it today have almost no way of knowing that this proposal even exists.

The set of calling conventions is not actually fixed hence an enum isn't a good choice to represent the set of values.

Doesn't that mean language keywords are an even worse choice? An enum can have different values for different target frameworks and you can even cast an int to it if you know it's a valid value not exposed by the standard library. You can't do any of that with a set of contextual keywords.

I guess with your arguments against attributes, that would point to @jkotas's proposal to use something like unmanaged(CallingConvention.Cdecl).

@jaredpar
Copy link
Member Author

You can't do any of that with a set of contextual keywords.

That's intended here and a bonus. When emitting to IL the compiler has to determine which bits to set in the calling convention section for a given calling convention. That does not necessarily line up with the values in any particular enum. Additionally not all runtimes support all calling conventions hence allowing a cast to int gives users an opportunity to introduce unsupported behavior. A small set of contextual keywords solves both these problems:

  • Compiler knows the mapping between calling convention and enconding.
  • Compiler knows which runtimes support which calling conventions and can issue appropriate errors when there is a mismatch.

I still think a separate issue to discuss this proposal would be nice

I'll likely create a new issue once I get this updated based on LDM feedback.

@HaloFour
Copy link
Contributor

@jaredpar

There are several problems problems with the attribute approach

I don't understand this argument. That's one of the reasons why (pseudo-)attributes exist and this is exactly how P/Invoke was designed. That calling convention is embedded directly in IL metadata, not as a custom attribute.

I'm not taking a side on the argument, I just don't understand the position that attributes can't be used here when the language already demonstrates otherwise.

@jkotas
Copy link
Member

jkotas commented Oct 24, 2018

A small set of contextual keywords solves both these problems

I would think that these problems can be solved with the calling convention enum equally well in the compiler. The compiler code can either look like this:

switch (keyword)
{
case "cdecl":
   if (cdecl not supported) throw; 
   encode cdecl;
   break;
case "stdcall":
   if (stdcall not supported) throw; 
   encode stdcall
   break;
...
}

or look like this:

switch (callconv)
{
case CallingConvention.CDecl:
   if (cdecl not supported) throw; 
   encode cdecl;
   break;
case CallingConvention.StdCall:
   if (stdcall not supported) throw; 
   encode stdcall
   break;
...
}

@jaredpar
Copy link
Member Author

@HaloFour

I don't understand this argument

Several points were raised. Which one did you not understand?

That's one of the reasons why (pseudo-)attributes exist and this is exactly how P/Invoke was designed.

P/Invoke doesn't have to deal with type identity / conversions issues. Function pointers must address that. There is no case today where user attributes affect type identity in the language. Also please consider the points about API and encoding

Bottom line for me: these would never be represented as attributes hence why use them? It's an explicit lie that we have to spend lots of time covering up.

@CyrusNajmabadi
Copy link
Member

Bottom line for me: these would never be represented as attributes

I think the different perspective comes from how one views an attribute. It can be a very .net/metadata-centric view, in which case using attributes that don't end up as actual IL attributes seems weird. Or one can have a very C#-centric view, where this is jsut a flexible language construct that can be used to annotate and configure code, without necessarily having that annotation ever be reflected (no pun intended) at runtime. In hte latter view of the world, attributes serve as a very useful tool to help avoid a continual stream of 'one off' language/syntactic features around lanuage knobs.

@HaloFour
Copy link
Contributor

@CyrusNajmabadi

Or one can have a very C#-centric view, where this is jsut a flexible language construct that can be used to annotate and configure code, without necessarily having that annotation ever be reflected (no pun intended) at runtime. In hte latter view of the world, attributes serve as a very useful tool to help avoid a continual stream of 'one off' language/syntactic features around lanuage knobs.

This is where I'm coming from. Granted, pseudo-attributes are fairly rare in .NET. But I do understand the issues that are being raised here, and if you had to use attribute syntax to specify some of this metadata but you couldn't apply any other attributes that would certainly be strange.

@pentp
Copy link

pentp commented Oct 24, 2018

@jkotas's proposal to use unmanaged(CallingConvention.Cdecl) could be better for discoverability of calling conventions.

This proposal does not cover at least half of #191 - handleof (ldtoken). Better to have a separate discussion thread for this proposal.

My only other concern is that this should leave open the future possibility of adding ldftn and ldvirtftn for instance methods (e.g. &VirtMethod and &this.VirtMethod). At first look this seems to be the case, but I'm not a language designer 😄

@jaredpar
Copy link
Member Author

@pentp

This proposal does not cover at least half of #191 - handleof (ldtoken). Better to have a separate discussion thread for this proposal.

The handleof feature wasn't contentious. Only the parts around calli and ldftn. That's why I didn't include handleof in this write up.

My only other concern is that this should leave open the future possibility of adding ldftn and ldvirtftn for instance methods (e.g. &VirtMethod and &this.VirtMethod). At first look this seems to be the case, but I'm not a language designer

Meeting with CLR folks later today to discuss the realities of this. Should have a better picture of whether or not this is feasible to implement after that. If there is a way to do instance methods we'll likely just do it in the initial feature implementation. Don't want to revisit function pointers later, other than to possibly add new calling conventions.

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.