Make NativeCallableAttribute public.

[AaronRobinsonMSFT]

Fixes#32462

TODO:

• Remove usage overhead by avoiding COM Delegate creation.
• Existing NativeCallableAttribute tests pass.
• Write new tests for negative cases.
• Handle exceptions on Windows.
• Properly fail-fast for exceptions on non-Windows.
• Update CrossGen2.

/cc @jkotas@fadimounir@jeffschwMSFT@jkoritzinsky

[AaronRobinsonMSFT]

/cc @Scottj1s

[AaronRobinsonMSFT]

/cc @MichalStrehovsky for CrossGen2

[AaronRobinsonMSFT]

@janvorli Can you take a look at the work here and offer your thoughts on exceptions handling at the margins. @jkotas and I are in agreement that on Unix exceptions here aren't supported at this transition point, but on Windows we would want the typical behavior reverse P/Invoke behavior. Can you point me at an example of what I would need to do? Also, do you have any thoughts on the cost to support x86?

[janvorli]

@AaronRobinsonMSFT as for the EH on Linux, to ensure that we fail fast if there is an exception leaking from a method marked with NativeCallable, I can see two options here:

1. Modify UnwindManagedExceptionPass1 so that when it crosses the managed to native boundary, it checks if the managed method has native callable attribute and fail fast the process with unhandled exception in that case. The change would be trivial, but I am not sure about the cost of of the call to MethodDesc::HasNativeCallable at each managed to native boundary. @jkotas, do you think the cost of this call would be negligible here?
2. Modify JIT so that it wraps bodies of methods marked with the NativeCallable attribute in a try / catch and calls FailFast in the catch.

[AaronRobinsonMSFT]

Thanks @janvorli. (1) seems like a generally less ideal approach - doesn't really handle the pay-for-play mentality we try to have in the interop space. I had been thinking about (2) as an option but wanted to hear from you first. Since you suggested it, I will reach out to the @dotnet/jit-contrib team and ask for some advice if flags already exist or how I should think about the work. Would (2) apply for both Windows and non-Windows?

[jkotas]

I think that (1) is the right way to do it. (2) adds performance overhead to the non-exception path for no good reason.

if the managed method has native callable attribute

This fact is encoded in GCInfo. So all you need to do is to crack GCInfo of the method that is not a big deal on exception handing path. .NET Native / CoreRT - that has very pay-for-play and performance minded design - does exactly that.

[janvorli]

This fact is encoded in GCInfo

Ah, I have forgotten that. It is the reverse pinvoke frame stack slot info presence, right?

[jkotas]

It is the reverse pinvoke frame stack slot info presence, right?

Yep

[AaronRobinsonMSFT]

I think that (1) is the right way to do it. (2) adds performance overhead to the non-exception path for no good reason.

@jkotas Doesn't (1) imply that whenever a managed exception is thrown this check can occur? As opposed to (2) that will only occur if a reverse P/Invoke happens. I would assume that managed exceptions occur far more often than a reverse P/Invoke or am I missing something here?

[AaronRobinsonMSFT]

Update for PR. Spoke offline with @jkotas. The gist of the argument is that the exception path is already expensive and this check in the suggested area isn't going to move the needle in a measurable way; ergo (1) is appropriate.

[janvorli]

@AaronRobinsonMSFT I've made the change in the UnwindManagedExceptionPass1 and I was really simple. I can either share the diff or I can make it a separate PR after your changes get in.

[AaronRobinsonMSFT]

@janvorli W00t! Thank you much. If you can share the diff I will apply it.

[AaronRobinsonMSFT]

@jkotas@janvorli@MichalStrehovsky@jkoritzinsky Please take another look. I believe I have addressed all outstanding issues and added sufficient scenario testing.

[jkotas]

This is a performance optimization. Do you have any performance numbers?

[kouvel]

In the original comment an "error case" was mentioned that I don't see so I am confused as to what the original comment was alluding at.

Oh ok, yea the link is not working well. With this link c78f4a4#diff-b06261f7127af3268df1f78173d20b34R1659 it's in codeversion.cpp on line 1659 on the right side (have to expand the default view to that line and it then should show the highlight).

I think your change would cover this case as well, it would go in with Unknown and use the slow path.

[kouvel]

Edited above to add file name

[jkotas]

Just last few nits, but looks great otherwise

[AaronRobinsonMSFT]

The Linux x64 Debug and Checked versions passed prior to the exception change in CrossGen2 so assuming they are not related to any changes here.