JIT: boxing shouldn't be removed for escaping managed references

[EgorBo]

This issue was reported through other channels

[MethodImpl(MethodImplOptions.NoInlining)]
ref int Test(Struct1 x)
{
    return ref ((I1)x).Method1();
}

public struct Struct1 : I1
{
    int field;

    [UnscopedRef]
    [MethodImpl(MethodImplOptions.NoInlining)]
    public ref int Method1() => ref field;
}

public interface I1
{
    [UnscopedRef]
    ref int Method1();
}

Here in Test JIT optimizes boxing for (I1)x) into:

; Method Class1:Test(Struct1):byref:this (FullOpts)
       sub      rsp, 40
       mov      dword ptr [rsp+0x20], edx
       lea      rcx, [rsp+0x20]
       call     [Struct1:Method1():byref:this]
       nop      
       add      rsp, 40
       ret      
; Total bytes of code: 25

Presumably, it's an UB for unmanaged pointers, but for all managed we probably should keep that boxing since it's a safe C# (the reference may also expose through a byref-like return type or a ref ByRefLikeType arg parameter.

cc @dotnet/jit-contrib

[jkotas]

Presumably, it's an UB for unmanaged pointers

Do we have a written statement somewhere that implies that there is no way to get into this situation via valid unsafe code?

[EgorBo]

Presumably, it's an UB for unmanaged pointers

Do we have a written statement somewhere that implies that there is no way to get into this situation via valid unsafe code?

I can't think of any valid way other than exposing a pointer outside of the fixed scope (which is defined as a non-valid code by ECMA-334 23.4 "address remains valid only for the duration of that fixed statement." or converting a managed pointer to an unmanaged using some unsafe API such as Unsafe.AsPointer.

Disabling this optimization for unmanaged pointers effectively means removing this optimization completely as they can be passed through any >=pointer-sized return type or out arg (which will be a big perf regressions - huge diffs last I checked).

[jkotas]

How about this?

using System.Runtime.CompilerServices;

unsafe class My
{
    static StashedRef* s;

    static void Main()
    {
        StashedRef r = default;
        s = &r;
        Test(default);
        Console.WriteLine(r.p);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Test(Struct1 x)
    {
        ((I1)x).MethodThatIsExpectedToBeCalledOnABoxedCopy();
    }

    public ref struct StashedRef
    {
        public ref int p;
    }

    public struct Struct1 : I1
    {
        int field;

        [MethodImpl(MethodImplOptions.NoInlining)]
        public void MethodThatIsExpectedToBeCalledOnABoxedCopy()
        {
            // We expect this method to be only called on a boxed copy, so it is safe to store the reference into a static.
            s->p = ref field;
        }
    }

    public interface I1
    {
        void MethodThatIsExpectedToBeCalledOnABoxedCopy();
    }
}

Disabling this optimization

I am not asking for this optimization to be disabled. I am asking for having a doc somewhere (RuntimeAugments?) that implies my example is invalid unsafe code.

[EgorBo]

I think @jakobbotsch mentioned that this problem is related to the boxing Nullable<> problem (a valid IL that is not currently supported by the runtime, probably also can be put into the augments)

[jkotas]

the boxing Nullable<> problem

I do not think you can hit the boxing Nullable<> problem in vanilla (unsafe) C#. This one is different - one can write vanilla (unsafe) C# that hits it.

[jakobbotsch]

It's the same problem as we discussed around #112543, cc @AndyAyersMS.
If we change the spec to allow this optimization then I wonder if it is possible to at the same time allow that one.
Even with UnscopedRef on MethodThatIsExpectedToBeCalledOnABoxedCopy we see a warning emitted by Roslyn:

warning CS9093: This ref-assigns 'field' to 'p' but 'field' can only escape the current method through a return statement.

Seems like some good inspiration for what the spec augment should look like.

[EgorBo]

Should it be promoted to an error in Roslyn then 🤔

[jkotas]

Roslyn allows you to do unsafe byref assignments in unsafe code. It is a feature. You have to know what you are doing if you write unsafe code. I do not see why we would want to change that. It would be a breaking change that is hard to justify.

Unsafe code needs to come with rules that need to be followed to make the unsafe code valid (safe).

It's the same problem as we discussed around #112543,
If we change the spec to allow this optimization then I wonder if it is possible to at the same time allow that one.

I do not see any unsafe code in #112543 (comment) . It is not ok to introduce rules that need to be followed to make vanilla safe C# memory safe.

[jakobbotsch]

I do not see any unsafe code in #112543 (comment) . It is not ok to introduce rules that need to be followed to make vanilla safe C# memory safe.

The nail in the coffin for #112543 was an example like the one you shared above. Basically escape a span upwards on the stack via unsafe code such that it is impossible to know from the signature whether something may escape.