JIT optimizations can produce incorrect IL when dealing with byrefs

[GrabYourPitchforks]

(See also dotnet/coreclr#23783 (comment).)

Repro:

private delegate ref byte MyDelegate(ref byte theRef, IntPtr a, IntPtr b);

static void Main()
{
    DynamicMethod dm = new DynamicMethod("Sample",
        typeof(byte).MakeByRefType(),
        new Type[] { typeof(byte).MakeByRefType(), typeof(IntPtr), typeof(IntPtr) });

    var ilGen = dm.GetILGenerator();
    ilGen.Emit(OpCodes.Ldarg_0);
    ilGen.Emit(OpCodes.Ldc_I4_8);
    ilGen.Emit(OpCodes.Sub);
    ilGen.Emit(OpCodes.Ldarg_1);
    ilGen.Emit(OpCodes.Add);
    ilGen.Emit(OpCodes.Ldc_I4_8);
    ilGen.Emit(OpCodes.Sub);
    ilGen.Emit(OpCodes.Ldarg_2);
    ilGen.Emit(OpCodes.Add);
    ilGen.Emit(OpCodes.Ret);

    var del = (MyDelegate)dm.CreateDelegate(typeof(MyDelegate));
}

Codegen:

DynamicClass.Sample(Byte ByRef, IntPtr, IntPtr):
00007ffc`80220080 4803d1          add     rdx,rcx
00007ffc`80220083 4a8d4402f0      lea     rax,[rdx+r8-10h]
00007ffc`80220088 c3              ret

Here, rcx := theRef&, rdx := a, and r8 := b. The JIT has incorrectly rearranged the addition of a and the subtraction of 8. It's possible that the caller required the operations to be done specifically in that order to avoid a GC hole. The intermediate value ref byte temp = ref theRef + a could result in a temp that points beyond the bounds of the GC-tracked object.

Edit: the following codegen would have been acceptable, since it doesn't result in an intermediate byref.

add rdx, r8
lea rax, [rcx + rdx - 10h]
ret

[AndyAyersMS]

The jit is likely not sufficiently careful about re-association in address formation.

In normal usage, address computations are always adding positive values, and so it doesn't matter what order the adds are performed.

Fixing this in general without causing regression may not be easy -- for example, we may need to surface implicit assumptions about the non-negativity of some variable addends below guard checks.

I suspect this is a long-standing issue with jitted codegen in general. Will put in 3.0 for now but will likely move this to future once we've looked at it more deeply.

[AndyAyersMS]

Likely culprit is fgMoveOpsLeft. It has some byref paranoia, but perhaps not enough.

[erozenfeld]

I'll investigate.

[erozenfeld]

The culprit is this code in fgMorphSmpOpOptional:

https://github.com/dotnet/coreclr/blob/72d49127a0c25e4b931c81e621c2411bfb6633a5/src/jit/morph.cpp#L13991-L14035

I have a change that allows this transformation for byrefs only if the constant is not negative and the other operand is unsigned. With that I get this code for the issue example:

G_M8004_IG02:
       4883C1F8             add      rcx, -8
       4803D1               add      rdx, rcx
       4A8D4402F8           lea      rax, bword ptr [rdx+r8-8]

G_M8004_IG03:
       C3                   ret

[erozenfeld]

I'll check the diffs to see if this causes many regressions.