roslyn: UnmanagedCallersOnly functions with ref parameters should be an error

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:<unknown assembly> type:<unknown 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

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 18 (13 by maintainers)

Commits related to this issue

Error when ref is used on a parameter or return type of an UnmanagedCallersOnly method Fixes https://github.com/dotnet/roslyn/issues/57025. — committed to 333fred/roslyn by 333fred 3 years ago
Error when ref is used on a parameter or return type of an UnmanagedCallersOnly method (#57043) Fixes https://github.com/dotnet/roslyn/issues/57025. — committed to dotnet/roslyn by 333fred 3 years ago
Fix https://github.com/dotnet/roslyn/issues/57025 — committed to wegylexy/XPLM by wegylexy 2 years ago
Fix https://github.com/dotnet/roslyn/issues/57025 — committed to wegylexy/XPLM by wegylexy 2 years ago

Most upvoted comments

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.

jkoritzinsky on Jan 13, 2022

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. 🙂

xoofx on Apr 8, 2022

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.

jkotas on Oct 7, 2021

It is working as expected with NativeAOT.

It is a bug / TODO in NativeAOT.

jkotas on Jan 13, 2022

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.

jkotas on Jan 12, 2022

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.

jaredpar on Oct 8, 2021

@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?

333fred on Oct 7, 2021