Incorrect NRE during operations on structures in method marked with AgressiveOptimization

[nsentinel]

Description

Testing our code on .NET 8 we encountered getting NRE where it is not supposed to be. I managed to write a minimal sample for the test (below). The described behavior is repeated on Preview.7, RC.1, RC.2 (haven't tried other versions, but it works fine on .NET 7)

Apparently, the problem is in the generated JIT code, because with optimization enabled, the method generates an explicit reference to address 0.

; Method MutableStructs.Program:TestMethod() (FullOpts)
G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+0x30], rax
       mov      qword ptr [rsp+0x28], rax

G_M000_IG02:                ;; offset=0x0010
       mov      qword ptr [rsp+0x30], 420
       mov      qword ptr [rsp+0x28], 42
       mov      ecx, 3
       call     [System.TimeSpan:op_UnaryNegation(System.TimeSpan):System.TimeSpan]
       mov      rcx, qword ptr [rsp+0x30]
       cmp      rcx, qword ptr [rsp+0x28]
       jl       SHORT G_M000_IG04
       align    [7 bytes for IG03]

G_M000_IG03:                ;; offset=0x0040
       add      qword ptr [0x0000], rax ;; <- NRE
       mov      rcx, qword ptr [rsp+0x30]
       cmp      rcx, qword ptr [rsp+0x28]
       jge      SHORT G_M000_IG03

G_M000_IG04:                ;; offset=0x0054
       add      rsp, 56
       ret      
; Total bytes of code: 89

Reproduction Steps

using System.Runtime.CompilerServices;

namespace MutableStructs;

public struct MutableStruct
{
    private long _internalValue;

    public long InternalValue
    {
        get => Volatile.Read(ref _internalValue);
        private set => Volatile.Write(ref _internalValue, value);
    }

    public void Add(long value) => AddInternal(value);
    private void AddInternal(long value) => InternalValue += value;
    public MutableStruct(long value) => InternalValue = value;
}

internal static class Program
{
    private static void Main() => TestMethod();

    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    private static void TestMethod()
    {
        var test = new MutableStruct(420);
        var from = new MutableStruct(42);

        var wrapper = -new TimeSpan(3);

        while (test.InternalValue >= from.InternalValue)
        {
            test.Add(wrapper.Ticks);  // <- NRE
        }
    }
}

Expected behavior

The code is successfully executed

Actual behavior

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. at MutableStructs.Program.TestMethod()

Regression?

At least since .NET 7

Known Workarounds

The behavior described is quite sensitive to a combination of factors. Small changes will normalize the situation: you can remove Volatile, or make InternalValue an auto property, or not wrap Add/AddInternal calls, or use your own structure instead of TimeSpan - all of the above will remove the issue

Configuration

.NET 8 RC.2
Windows 10 Pro 22H2 x64
12th Gen Intel(R) Core(TM) i9-12900K

Other information

No response

[ghost]

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

Issue Details

---

Description

Testing our code on .NET 8 we encountered getting NRE where it is not supposed to be. I managed to write a minimal sample for the test (below). The described behavior is repeated on Preview.7, RC.1, RC.2 (haven't tried other versions, but it works fine on .NET 7)

Apparently, the problem is in the generated JIT code, because with optimization enabled, the method generates an explicit reference to address 0.

; Method MutableStructs.Program:TestMethod() (FullOpts)
G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+0x30], rax
       mov      qword ptr [rsp+0x28], rax

G_M000_IG02:                ;; offset=0x0010
       mov      qword ptr [rsp+0x30], 420
       mov      qword ptr [rsp+0x28], 42
       mov      ecx, 3
       call     [System.TimeSpan:op_UnaryNegation(System.TimeSpan):System.TimeSpan]
       mov      rcx, qword ptr [rsp+0x30]
       cmp      rcx, qword ptr [rsp+0x28]
       jl       SHORT G_M000_IG04
       align    [7 bytes for IG03]

G_M000_IG03:                ;; offset=0x0040
       add      qword ptr [0x0000], rax ;; <- NRE
       mov      rcx, qword ptr [rsp+0x30]
       cmp      rcx, qword ptr [rsp+0x28]
       jge      SHORT G_M000_IG03

G_M000_IG04:                ;; offset=0x0054
       add      rsp, 56
       ret      
; Total bytes of code: 89

Reproduction Steps

using System.Runtime.CompilerServices;

namespace MutableStructs;

public struct MutableStruct
{
    private long _internalValue;

    public long InternalValue
    {
        get => Volatile.Read(ref _internalValue);
        private set => Volatile.Write(ref _internalValue, value);
    }

    public void Add(long value) => AddInternal(value);
    private void AddInternal(long value) => InternalValue += value;
    public MutableStruct(long value) => InternalValue = value;
}

internal static class Program
{
    private static void Main() => TestMethod();

    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    private static void TestMethod()
    {
        var test = new MutableStruct(420);
        var from = new MutableStruct(42);

        var wrapper = -new TimeSpan(3);

        while (test.InternalValue >= from.InternalValue)
        {
            test.Add(wrapper.Ticks);  // <- NRE
        }
    }
}

Expected behavior

The code is successfully executed

Actual behavior

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. at MutableStructs.Program.TestMethod()

Regression?

At least since .NET 7

Known Workarounds

The behavior described is quite sensitive to a combination of factors. Small changes will normalize the situation: you can remove Volatile, or make InternalValue an auto property, or not wrap Add/AddInternal calls, or use your own structure instead of TimeSpan - all of the above will remove the issue

Configuration

.NET 8 RC.2
Windows 10 Pro 22H2 x64
12th Gen Intel(R) Core(TM) i9-12900K

Other information

No response

Author:	nsentinel
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

It hits an assert on Checked JIT:

Assert failure(PID 78378 [0x0001322a], Thread: 1274202 [0x13715a]): Assertion failed '!memBase->isContained()' in 'MutableStructs.Program:TestMethod()' during 'Generate code' (IL size 69; hash 0x2b7f56af; FullOpts)

    File: /Users/egorbo/prj/runtime/src/coreclr/jit/emitxarch.cpp Line: 4708
    Image: /Users/egorbo/prj/runtime/artifacts/bin/coreclr/osx.x64.Checked/corerun

zsh: abort      /Users/egorbo/prj/runtime/artifacts/bin/coreclr/osx.x64.Checked/./corerun

cc @dotnet/jit-contrib

[BruceForstall]

@AndyAyersMS PTAL

[AndyAyersMS]

We have an RWM memory op

N001 (  3,  3) [000085] -c---------                   t85 =    LCL_ADDR  byref  V08 tmp5         [+0] $140
                                                            /--*  t85    byref  
N002 (  6,  5) [000086] Vc--GO-----                   t86 = *  IND       long   $c3
N003 (  1,  1) [000070] -----------                   t70 =    LCL_VAR   long   V10 tmp7         u:2 $1c0
                                                            /--*  t86    long   
                                                            +--*  t70    long   
N004 (  8,  7) [000081] -c--GO-----                   t81 = *  ADD       long   $1c1
N005 (  3,  3) [000089] -c---------                   t89 =    LCL_ADDR  byref  V08 tmp5         [+0] $140
                                                            /--*  t89    byref  
                                                            +--*  t81    long   
N006 ( 15, 13) [000091] VA--GO-----                         *  STOREIND  long   $VN.Void

This should produce something like

add [rsp + offset(V09)], reg(V10)

but this case of emitHandleMemOp does not expect to see a contained [000089].

[AndyAyersMS]

Seems like xarch's emitInsRMW just does not handle the case where the address is a local and the value to add is not immediate. Adding the obvious bit

diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp
index ba599c9e465..c1d7e2df5d4 100644
--- a/src/coreclr/jit/emitxarch.cpp
+++ b/src/coreclr/jit/emitxarch.cpp
@@ -5485,6 +5485,13 @@ void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTreeStoreInd* storeI
     {
         assert(!src->isContained()); // there must be one non-contained src

+        if (addr->isContained() && addr->OperIs(GT_LCL_ADDR))
+        {
+            GenTreeLclVarCommon* lclVar = addr->AsLclVarCommon();
+            emitIns_S_R(ins, attr, src->GetRegNum(), lclVar->GetLclNum(), lclVar->GetLclOffs());
+            return;
+        }
+
         // ind, reg
         id = emitNewInstrAmd(attr, offset);
         emitHandleMemOp(storeInd, id, emitInsModeFormat(ins, IF_ARD_RRD), ins);

fixes the test:

       add      qword ptr [rsp+0x30], rax

Since this is regression, let me look at what happens in .NET 7.

[AndyAyersMS]

In .NET 7 we had

N001 (  3,  2) [000134] -c-----N---                  t134 =    LCL_VAR_ADDR byref  V08 tmp5         
                                                            /--*  t134   byref  
N003 (  6,  5) [000137] Vc--GO-----                  t137 = *  IND       long   $147
N004 (  1,  1) [000108] -----------                  t108 =    LCL_VAR   long   V10 tmp7         u:2 $144
                                                            /--*  t137   long   
                                                            +--*  t108   long   
N005 (  8,  7) [000123] ----GO-----                  t123 = *  ADD       long   $240
                                                            /--*  t123   long   
N007 (  8,  7) [000146] DA--GO-----                         *  STORE_LCL_VAR long   V07 tmp4         d:2

so it was not driven off of STOREIND. Emitting was done via emitInsStoreLcl which handles the S_R case.

[AndyAyersMS]

@nsentinel thank you for reporting this bug.

It should be fixed in the official .NET8 RC2 release which will be available around Oct 10th. The fix should also appear in interim .NET 8 RC2 and .NET 9 builds with versions higher than 23469 (may take a few days for one of these to show up).

[nsentinel]

@AndyAyersMS Thank you very much for the quick fix!

I will check it once it will be available in the daily builds of .NET 8 RC.2

[AndyAyersMS]

@nsentinel a fix should be in the recent RC2 builds, let me know if you get chance to try one out.

[nsentinel]

@AndyAyersMS I apologize for the delay in checking. Didn't have a chance to check it right away as I was going to, although I managed to put daily build on the same day we were discussing.

Checked with SDK 8.0.100-rtm.23479.3-win-x64 today with the same source code.

The issue was not repeated neither in automatic tests, nor in manual testing, so I think the fix covers all the nuances at least in our code.

Thanks again for the quick fix.