Add III.1.7 "Managed pointers exposing parameters outside of the method scope" to Ecma-335-Augments.md

[EgorBo]

Motivation:

1. Set written rules for the JIT optimizer when it can and when it cannot optimize boxings for structs JIT: boxing shouldn't be removed for escaping managed references #122099. If we don't set the rules, the optimization that the JIT does is only valid when the method over the boxed struct is inlined and JIT's escape analysis clearly sees that nothing escapes anywhere. If we conservatively make it so, it will be a noticeable performance regression over previous releases.
2. Allow JIT to perform a free inter-procedural escape analysis by only looking at call's signature, for an example:

Span<int> buffer = new int[10];
Consume(buffer); 

given the void Consume(Span<int>) signature, JIT can be sure that the buffer never escapes anywhere without looking into actual implementation. We expect this to unlock the potential of the current Escape Analysis and handle a lot more cases.

Another examples:

void Case1(ref Span<int> X)
{
    Span<int> buffer = new int[10];
    Consume(buffer, ref X); // buffer cannot be stack-allocated as it might escape through X
}

ref int Case2()
{
    Span<int> buffer = new int[10];
    return ref Consume(buffer); // buffer cannot be stack-allocated as it might escape through return ref
}

ByrefLikeType Case3()
{
    Span<int> buffer = new int[10];
    return Consume(buffer); // buffer cannot be stack-allocated as it might escape through return ByrefLikeType
}

// ByrefLikeTypeWithRefRef can not be declared today
void Case4(ByrefLikeTypeWithRefRef p)
{
    Span<int> buffer = new int[10];
    return Consume(buffer, p); // buffer cannot be stack-allocated as it might escape through p parameter
}

Based on the discussion with @jakobbotsch @jkotas. Let me know if the wording needs to be improved.

PS: "multiple levels of byref fields" is not something that can be represented today in C#, but we leave a room for it.

Open questions:

• If we call it UB, should we make warning CS9085: This ref-assigns 'field' to 'p' but 'field' has a narrower escape scope than 'p'. an error? UPD: given it requires unsafe and there are, possibly, valid scenarios it's fine to leave it as is.

[Copilot]

Pull request overview

This PR adds a new specification section III.1.7.7 "Managed pointers exposing parameters outside of the method scope" to the Ecma-335 augments document. The goal is to formalize rules for when byrefs derived from method parameters can escape a function's scope, enabling JIT compiler optimizations such as boxing optimizations and inter-procedural escape analysis.

Key changes:

• Defines two permitted ways for byrefs from parameters to escape: explicit returns and writes through byrefs to byref-like types
• Declares undefined behavior for any other escape attempts
• Provides examples to clarify when parameters can and cannot expose values

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[jkotas]

What does "derived" mean exactly? The existing uses of "derived" in the ECMA spec are about inheritance.

[EgorBo]

When we take a byref from a parameter directly or from any of its fields or array elements.

Byrefs obtained from parameters directly or indirectly (byrefs to their fields or array elements)

is it better?

[jkotas]

array elements

Marshal.UnsafeAddrOfPinnedArrayElement does exactly that. I think this needs to be more limited.

[jkotas]

If we call it UB, should we make warning CS9085: This ref-assigns 'field' to 'p' but 'field' has a narrower escape scope than 'p'. an error?

Does this warning have false positives - does it fire on correct (unsafe) code?

[EgorBo]

If we call it UB, should we make warning CS9085: This ref-assigns 'field' to 'p' but 'field' has a narrower escape scope than 'p'. an error?

Does this warning have false positives - does it fire on correct (unsafe) code?

Hard to tell, here is the minimal repro for that warning to show up:

unsafe void M(ref int x)
{
    int y = 0;
    x = ref y; // warning CS9085: This ref-assigns 'y' to 'x' but 'y' has a narrower escape scope than 'x'.
}

given it already requires unsafe keyword here, it's probably fine to keep it as is?

[jkotas]

Hard to tell, here is the minimal repro for that warning to show up:

It shows up for a code like this that is not obviously invalid:

struct MyStruct
{
    int f;

    unsafe ref int M(ref int x)
    {
        x = ref f;
        return ref x;
    }
}

given it already requires unsafe keyword here, it's probably fine to keep it as is?

Right, my guess it was the idea for keeping it as a warning - to have an escape hatch for false positives.

[tfenise]

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/Unsafe.cs

Lines 698 to 709 in ccd875b

	/// <summary>
	/// Writes a value of type <typeparamref name="T"/> to the given location.
	/// </summary>
	[Intrinsic]
	[NonVersionable]
	[CLSCompliant(false)]
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	public static void Write<T>(void* destination, T value)
	where T : allows ref struct
	{
	*(T*)destination = value;
	}

So now Unsafe.Write<Span<byte>> is unconditionally undefined behavior, as the Span<byte> escapes in a disallowed way? Unsafe.Write<T> shouldn't allow T : allows ref struct?

[EgorBo]

So now Unsafe.Write<Span<byte>> is unconditionally undefined behavior, as the Span<byte> escapes in a disallowed way? Unsafe.Write<T> shouldn't allow T : allows ref struct?

Interesting question. I'm not sure why the where T : allows ref struct constraint was added for Write specifically in the first place, probably just because it was added elsewhere too, at this point removing it is a breaking change. There are probably valid use-cases for it being used with ref structs (esp since ref structs aren't required to have ref fields).

Perhaps, the UB part needs to be re-phrased. "If you use unsafe and you know what you do - it's fine" 🙂

[jkotas]

Perhaps, the UB part needs to be re-phrased. "If you use unsafe and you know what you do - it's fine"

We need better description of what is valid unsafe code vs. invalid unsafe code than this. If we are not able to come up with one, I would rather revert the problematic JIT optimization.

[AndyAyersMS]

revert the problematic JIT optimization.

We never added the optimization... it was partially implemented in #112543 which we gave up on.

[jkotas]

We never added the optimization

I believe that the issue described in #122099 that motivated this discussion is in the product currently.

[EgorBo]

Right, @AndyAyersMS we talk about the existing optimization in impDevirtualizeCall e.g.

public void Foo(MyStruct ms)
{
    ((IMyStruct)ms).DoWork();
    //
    // mov      r11, 0x7FFDB60500E8
    // cmp      dword ptr [rcx], ecx
    // tail.jmp [r11]IMyStruct:DoWork():this
}


public interface IMyStruct
{
    void DoWork();
}

public class MyStruct : IMyStruct
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    public void DoWork()
    {
        // escapes itself in some odd way
    }
}

today we happily remove the (IMyStruct)ms boxing and call DoWork on an unboxed entry. But it might not be legal if we don't properly define the behavior.

[AndyAyersMS]

Sounds like either way we must defer the box removal to escape analysis.

I think that should be relatively straightforward: in impDevirtualizeCall we can modify the code to always follow the !optimizedTheBox path. Once we inline, escape analysis can determine if the box can be allocated on the stack, and physical promotion should kick in and forward the box payload references to their original sources, and we should end up with more or less the same codegen.

I can put up a PR for this.

[EgorBo]

Sounds like either way we must defer the box removal to escape analysis.

I think that should be relatively straightforward: in impDevirtualizeCall we can modify the code to always follow the !optimizedTheBox path. Once we inline, escape analysis can determine if the box can be allocated on the stack, and physical promotion should kick in and forward the box payload references to their original sources, and we should end up with more or less the same codegen.

I can put up a PR for this.

It's definitely better than not doing anything, although, I expect some noticeable regressions from cases where we can't inline (e.g. too big) or EA gives up due to some non-inlined call etc.

I still would prefer defining the behavior and allow ourselves to make assumptions based on call signature alone. We've had this behavior in impDevirtualize for a while and it raised no issues except that one that is very likely synthetic.

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[jkotas]

If we don't set the rules, the optimization that the JIT does is only valid when the method over the boxed struct is inlined and JIT's escape analysis clearly sees that nothing escapes anywhere. If we conservatively make it so, it will be a noticeable performance regression over previous releases.

I do not think the latest revision is going to allow these boxing optimizations. Could you please update the PR description if you agree?

[EgorBo]

I do not think the latest revision is going to allow these boxing optimizations. Could you please update the PR description if you agree?

I am probably misreading it, but can you clarify why we can't possibly do it for:

static void Foo()
{
    MyStruct ms = new();
    ((IDisposable)ms).Dispose(); // can we legally remove this boxing without inlining or inter-procedural analysis?
}

public struct MyStruct : IDisposable
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    public void Dispose()
    {
        // non-UB code
    }
}

[jkotas]

Ah ok. "Obtained from method parameters" needs further clarification. For example:

static PinnedGCHandle<StrongBox<int>> s;

void* M(StrongBox<int> x)
{
    s = new PinnedGCHandle(x);
    return Unsafe.AsPointer(ref x.Value);
}

Is this UB with this rule or not? (I think it should remain valid - it looks like a real-world code one can write.)