UnmanagedCallersOnly functions with ref parameters should be an error

[rolfbjarne]

Description

Test case:

[System.Runtime.InteropServices.UnmanagedCallersOnly]
static void ScheduledAudioFileRegionCallback (ref int fileRegion)
{
}

Results in:

• Assertion at /Users/runner/work/1/s/src/mono/mono/mini/aot-compiler.c:5142, condition `is_ok (error)' not met, function:add_wrappers, method blittable_unmanagedattribute.SceneDelegate:ScheduledAudioFileRegionCallback (int&) with UnmanagedCallersOnlyAttribute has non-blittable parameters or return type assembly: type: member:(null)

Complete test project: blittable-unmanagedattribute-6a124aa.zip

Repro: download & extract & execute dotnet build
Binlog: msbuild.binlog.zip

I'm guessing it's hitting this:

https://github.com/dotnet/runtime/blob/36229d9d3a5abcf161fbee3a8917dcbdd3070946/src/mono/mono/metadata/marshal.c#L3694-L3697

Note that the C# code is valid, because otherwise the C# compiler should've have compiled it successfully.

Configuration

$ dotnet --version
6.0.100-rtm.21477.21

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jkotas]

@333fred Is it expected that Roslyn allows this?

https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/function-pointers.md#systemruntimeinteropservicesunmanagedcallersonlyattribute says that only unmanaged types should be allowed on UnmanagedCallersOnly. ref int is not unmanaged type.

cc @AaronRobinsonMSFT @jkoritzinsky

[rolfbjarne]

This works just fine in .NET:

class App {
	public unsafe static void Main ()
	{
		delegate* unmanaged<ref int, void> i = &I;
		Console.WriteLine ((object) (IntPtr) (void*) i);
	}

	[UnmanagedCallersOnly]
	static void I (ref int fileRegion)
	{
	}
}

$ dotnet run
4698272552

[jkotas]

It does not work. The UnmanagedCallersOnly signatures are validated lazily by the runtime, once the method is called. Try this:

using System.Runtime.InteropServices;

class App
{
    public unsafe static void Main ()
    {
        delegate* unmanaged<ref int, void> i = &I;
       
        int fileRegion = 0;
        i(ref fileRegion);
    }

    [UnmanagedCallersOnly]
    static void I (ref int fileRegion)
    {
    }
}

It fails with Unhandled exception. System.InvalidProgramException: Non-blittable parameter types are invalid for UnmanagedCallersOnly methods.

[333fred]

@333fred Is it expected that Roslyn allows this?

I would say that since I didn't know refs weren't blittable, it's expected that Roslyn allows this. We could probably make a bugfix to that error to not permit them though, as the scenario is truly broken. @jaredpar, do you have thoughts on whether we should break that?

[jaredpar]

I think we should just take a bug fix here and make the scenario an error. Anyone depending on this is depending on broken / dangerous behavior. The only case where it can be seemingly used without error is just to print out the address.

[wegylexy]

I depended on in to pass in by pointer. Now it's broken.

[jkotas]

I depended on in to pass in by pointer. Now it's broken.

Did it work for you at runtime? The runtime should have been failing for this case.

[wegylexy]

I depended on in to pass in by pointer. Now it's broken.

Did it work for you at runtime? The runtime should have been failing for this case.

It is working as expected with NativeAOT.

// native code invokes it with a pointer to `struct MyValueType`
[UnmanagedCallersOnly(EntryPoint = "Test")]
static void Test(in MyValueType value)
{
  var copiedValue = value;
  // copiedValue is exactly what is expected
}

In the new version, I have to use unsafe pointer.

[UnmanagedCallersOnly(EntryPoint = "Test")]
static unsafe void Test(MyValueType* value)
{
  var copiedValue = *value;
}

ref and out probably don't make sense, but in should be kept supported.

[jkotas]

It is working as expected with NativeAOT.

It is a bug / TODO in NativeAOT.

[AaronRobinsonMSFT]

ref and out probably don't make sense, but in should be kept supported.

@wegylexy The design of UnmanagedCallersOnly specifically called out no support for non-blittable arguments. The fact that by-refs are non-blittable wasn't properly conveyed to the Roslyn team and was therefore missed on the language side. The inability to use any by-ref value (i.e., in, ref, out) is therefore by-design. If you would like to see this relaxed it would be a new feature request. It is unlikely it would be relaxed though.

[wegylexy]

What about DllImport? It seems to pin and pass the pointer.
I thought UnmanagedCallersOnly wrap those params as native pointers into unsafe references like Unsafe.AsRef<T>(void*) does.

[jkoritzinsky]

The pinning that DllImport does is due to the fact that in, ref, and out are not blittable. There needs to be extra code emitted to pin the values before passing them to native code. One of the main goals of UnmanagedCallersOnly is that it does zero wrapping/conversion of any parameters or return values.

[xoofx]

Hey,

We are discovering this problem at Unity as some of our unit tests are now failing compiling by updating Roslyn. That could be quite a breaking change on our side.

Could someone explain exactly why having a ref arg is not considered as blittable as it is considered from the native side as a pure pointer and from managed side, it is still a pointer in a register?

Behind the scene for the pure codegen part, I don't see how the code could be different. So if it is not a codegen problem, what is it? GC tracking ref?

Or differently, what is the difference with doing:

[System.Runtime.InteropServices.UnmanagedCallersOnly]
static unsafe void ScheduledAudioFileRegionCallback (int* fileRegion)
{
    ActualScheduledAudioFileRegionCallback(ref *fileRegion);
}

static void ActualScheduledAudioFileRegionCallback (ref int fileRegion)
{
  // ...
}

cc: @jkotas, @tgjones

[AaronRobinsonMSFT]

@xoofx The issue is one of correctness not necessarily code-gen. The guarantees around ref values are such that typing an argument from a method that is specifically designed to be called by an unmanaged caller means there is nothing to say that a nullptr is being passed in. This makes usage of the ref dubious in this scenario. The expectation here would be for users to either confirm the input state is non-null and proceed or know the caller's intent and immediately convert to a ref as in the example above. Either way, we are expecting a user action to be explicit rather than providing a guarantee that isn't provably correct.

[wegylexy]

An exception to the correctness is the return reference of Unsafe.NullRef<T>() and that Unsafe.IsNullRef<T>() may return true.

[AaronRobinsonMSFT]

An exception to the correctness is the return reference of Unsafe.NullRef() and that Unsafe.IsNullRef() may return true.

That's not really an exception. The use of anything in the Unsafe class is by-definition "unsafe" and it is therefore up to the user to do the right thing, not the compiler. That same logic holds true for this scenario, but since the user isn't creating the ref explicitly it can lead users to expect that the ref is compiler approved.

[xoofx]

Either way, we are expecting a user action to be explicit rather than providing a guarantee that isn't provably correct.

Thank you, I understand. On our side, the native code is actually C# code that we native compile with our Burst compiler, so it is explicit for the user that if it's declared with a ref, it means that it's not null. But we should be able to workaround if with an IL postprocessing step (we have many others) that will rewrite it to what I described above, so at least we have a workaround, just a bit unfortunate that we have to do that, but I hope it will work on our side. 🙂