Test failure in CI: Assertion failed in System.Tests.ValueTupleTests:LongTuplesWithNull()

[ahsonkhan]

See https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_arm64_altjit_checked_windows_nt_corefx_baseline_prtest/25/consoleFull#62222254079fe3b83-f408-404c-b9e7-9207d232e5fc from dotnet/coreclr#16223

17:51:12   Discovered:  System.ValueTuple.Tests
17:51:13   Starting:    System.ValueTuple.Tests
17:51:19   
17:51:19 
  Assert failure(PID 9188 [0x000023e4], Thread: 3328 [0x0d00]): Assertion failed 'varDsc->lvOnFrame && !varDsc->lvRegister' in 'System.Tests.ValueTupleTests:LongTuplesWithNull()' (IL size 741)
17:51:19   
17:51:19       File: d:\j\workspace\x64_arm64_alt---e876825d\src\jit\codegenarmarch.cpp Line: 753
17:51:19       Image: D:\j\workspace\x64_arm64_alt---e876825d\_\fx\bin\testhost\netcoreapp-Windows_NT-Release-x64\dotnet.exe
17:51:19   
17:51:19   Finished running tests.  End time=17:51:18.86, Exit code = 123456789
17:51:19 D:\j\workspace\x64_arm64_alt---e876825d\_\fx\Tools\tests.targets(492,5): warning MSB3073: The command "D:\j\workspace\x64_arm64_alt---e876825d\_\fx\bin/tests/System.ValueTuple.Tests/netcoreapp-Windows_NT-Release-x64//RunTests.cmd D:\j\workspace\x64_arm64_alt---e876825d\_\fx\bin/testhost/netcoreapp-Windows_NT-Release-x64/" exited with code 123456789. [D:\j\workspace\x64_arm64_alt---e876825d\_\fx\src\System.ValueTuple\tests\System.ValueTuple.Tests.csproj]

cc @stephentoub, @jcouv, @tarekgh

[AndyAyersMS]

This is a jit assert...

cc @dotnet/arm64-contrib

[AndyAyersMS]

Ran into dotnet/coreclr#17099 while trying to repro this via an altjit. Can workaround for now by restricting COMPlus_AltJit to just be the method that hits the assert.

I have extracted the CoreFX test into a standalone case which repros the assert.

using System;

class X
{
    public static ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLong<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) where TRest : struct
    {
        return new ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>(item1, item2, item3, item4, item5, item6, item7, rest);
    }
    
    public static Tuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLongRef<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest)
    {
        return new Tuple<T1, T2, T3, T4, T5, T6, T7, TRest>(item1, item2, item3, item4, item5, item6, item7, rest);
    }

    public static void AssertEqual(string s1, string s2)
    {
        if (!s1.Equals(s2))
        {
            throw new Exception();
        }
    }

    static void Test()
    {
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string>(null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string>(null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string>(null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string>(null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string>(null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string>(null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string, string>(null, null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string, string>(null, null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string, string, string>(null, null, null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string, string, string>(null, null, null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , , , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string, string, string, string>(null, null, null, null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string, string, string, string>(null, null, null, null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , , , , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string, string, string, string, string>(null, null, null, null, null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string, string, string, string, string>(null, null, null, null, null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , , , , , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
        
        {
            var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string, string, string, string, string, string>(null, null, null, null, null, null, null));
            var tupleWithNull = CreateLongRef(1, 2, 3, 4, 5, 6, 7, new Tuple<string, string, string, string, string, string, string>(null, null, null, null, null, null, null));
            AssertEqual("(1, 2, 3, 4, 5, 6, 7, , , , , , , )", vtWithNull.ToString());
            AssertEqual(tupleWithNull.ToString(), vtWithNull.ToString());
        }
    }
    
    public static int Main()
    {
        int result = 0;
        try
        {
            Test();
            result = 100;
        }
        catch (Exception)
        {
            result = -1;
        }

        return result;
    }
}

[sdmaclea]

Running System.ValueTuple.Tests one test case at a time only one test case is failing in release.
System.Tests.ValueTupleTests.LongTuplesWithNull. It is however failing with runtime termination.

[sdmaclea]

I split the above test into 8 pieces. Of the 8 pieces, 7 passed. The third piece failed. The third piece was trimed to the following and still failed.

    static void Test()
    {
        var vtWithNull = CreateLong(1, 2, 3, 4, 5, 6, 7, new ValueTuple<string, string>(null, null));
        AssertEqual("(1, 2, 3, 4, 5, 6, 7, , )", vtWithNull.ToString());
    }

Most of the other cases are passing.

[sdmaclea]

The assert is firing while putting the eighth argument, a 16-byte struct. This must be passed on the stack per the ABI. However since there are only 7 register arguments consumed there could be an off by one error in this special case with a two register struct.

The tmp allocated by the new ValueTuple<string, string>(null, null) is marked do not enregister address exposed. The tmp has its fields promoted. They need to be spilled to the stack before the call.

@dotnet/jit-contrib How is this supposed to work? Should the call argument tmp fields never be promoted. Is something supposed to generate a node to spill? Is the code PUTSTACK_ARG supposed to handle the enregistered struct and spill?

[sdmaclea]

@CarolEidt Since you are looking at morph args et al. Perhaps you can answer my question above?

[CarolEidt]

I would have to look more carefully at this to speak definitively on this, but it should be the case that any promoted lclVar can be used as a call argument. However, it it's promoted it should be marked do not enregister, which you indicate that it is. I would suspect that it is the 7 register arguments, with one register left over that's causing the issue - though, again, I'd have to look at it further to be sure.
In a jitdump, you should see a dump of the arg table that looks something like this:

fgArgTabEntry[arg 8 262.OBJ, numSlots=2, slotNum=2, align=1, lateArgInx=0, tmpNum=V12, isTmp, processed, isStruct]
fgArgTabEntry[arg 9 59.LCL_VAR, numSlots=1, slotNum=4, align=1, processed]
fgArgTabEntry[arg 10 66.LCL_VAR, numSlots=1, slotNum=5, align=1, processed, isStruct]
fgArgTabEntry[arg 11 67.LCL_VAR, numSlots=1, slotNum=6, align=1, processed]
fgArgTabEntry[arg 0 30.CNS_INT, 1 regs: rdi, align=1, lateArgInx=1, processed]
fgArgTabEntry[arg 1 31.CNS_INT, 1 regs: rsi, align=1, lateArgInx=2, processed]
fgArgTabEntry[arg 2 32.CNS_INT, 1 regs: rdx, align=1, lateArgInx=3, processed]
fgArgTabEntry[arg 3 33.CNS_INT, 1 regs: rcx, align=1, lateArgInx=4, processed]
fgArgTabEntry[arg 4 35.CNS_INT, 1 regs: r8, align=1, lateArgInx=5, processed]
fgArgTabEntry[arg 5 37.CNS_INT, 1 regs: r9, align=1, lateArgInx=6, processed]
fgArgTabEntry[arg 6 38.CNS_INT, numSlots=1, slotNum=0, align=1, processed]
fgArgTabEntry[arg 7 39.CNS_INT, numSlots=1, slotNum=1, align=1, processed]

If it includes numSlots then it is on the stack, and if it includes <n> regs then it is passed in registers. I think that on arm32 it displays both for split args, but I'm not certain. You should look at what it shows for the arg in question.

[sdmaclea]

OK I finally got back to this.

Argument table looks like this.

fgArgTabEntry[arg 9 83.OBJ, numSlots=2, slotNum=1, align=1, processed]
fgArgTabEntry[arg 0 78.ADDR, x0, regs=1, align=1, lateArgInx=0, processed]
fgArgTabEntry[arg 1 80.CNS_INT, x1, regs=1, align=1, lateArgInx=1, processed]
fgArgTabEntry[arg 2 65.CNS_INT, x2, regs=1, align=1, lateArgInx=2, processed]
fgArgTabEntry[arg 3 66.CNS_INT, x3, regs=1, align=1, lateArgInx=3, processed]
fgArgTabEntry[arg 4 67.CNS_INT, x4, regs=1, align=1, lateArgInx=4, processed]
fgArgTabEntry[arg 5 68.CNS_INT, x5, regs=1, align=1, lateArgInx=5, processed]
fgArgTabEntry[arg 6 69.CNS_INT, x6, regs=1, align=1, lateArgInx=6, processed]
fgArgTabEntry[arg 7 70.CNS_INT, x7, regs=1, align=1, lateArgInx=7, processed]
fgArgTabEntry[arg 8 71.CNS_INT, numSlots=1, slotNum=0, align=1, processed]

I was surprised to see the struct constructor takes a this and a template class argument. Which moves the rest argument and the 7 into the stack

[sdmaclea]

The tmp allocated by the new ValueTuple<string, string>(null, null) is marked do not enregister address exposed. The tmp has its fields promoted. They need to be spilled to the stack before the call.

My comment above was inaccurate. I was looking at the destination of the GT_PUTARG_STK, not the source which is what is triggering the assert.

So in the failing case

  N035.    LCL_VAR_ADDR V03 tmp2          NA
                 ref    V03.Item1 (offs=0x00) -> V08 tmp7          x19 (last use)
                 ref    V03.Item2 (offs=0x08) -> V09 tmp8          x19 (last use) REG NA
  N037.    OBJ(16)  
  N039.    PUTARG_STK [+0x08]

V03 is

lvOnFrame = 0
lvAddrExposed = 0
lvDoNotEnregister = 0
lvPromoted = 1

lvaTable after fgPromoteStructs
; Initial local variable assignments
;
;  V00 loc0           struct (48)  ld-addr-op
;  V01 OutArgs        lclBlk (na) 
;  V02 tmp1           struct (16) 
;  V03 tmp2           struct (16) 
;  V04 tmp3           struct (48) 
;  V05 tmp4              ref  class-hnd
;  V06 tmp5              ref  V02.Item1(offs=0x00) P-INDEP
;  V07 tmp6              ref  V02.Item2(offs=0x08) P-INDEP
;  V08 tmp7              ref  V03.Item1(offs=0x00) P-INDEP
;  V09 tmp8              ref  V03.Item2(offs=0x08) P-INDEP

[sdmaclea]

@CarolEidt help
It seems either:

• V03 should be marked lvOnFrame = 1, lvAddrExposed = 1, lvDoNotEnregister = 1
• genPutArgStk() should handle putting a promoted struct
• LCL_VAR_ADDR should become GT_FIELD_LIST when the struct is promoted.

[sdmaclea]

@CarolEidt Attaching jitdump
jitdump.zip

[briansull]

The JIT isn't seeing the uses of V08 and V09 properly here.
The defs for each of these are assigning x19 for both V08 and V09
The use looks like it only sees the address taken struct V03, which feeds into the PUTARG_STK.
But if it is address taken then V08/V09 should not be fully promoted into registers

[briansull]

The callsite after fgMorphTree:

fgMorphTree BB01, stmt 5 (after)
               [000079] --CXG+------              *  CALL      void   System.ValueTuple`8[Int32,Int32,Int32,Int32,Int32,Int32,Int32,ValueTuple`2][System.Int32,System.Int32,System.Int32,System.Int32,System.Int32,System.Int32,System.Int32,System.ValueTuple`2[System.__Canon,System.__Canon]]..ctor
               [000071] -----+------ arg8 out+00  +--*  CNS_INT   int    7
     (  9,  7) [000083] x----------- arg9 out+08  +--*  OBJ(16)   struct
     (  3,  3) [000082] L-----------              |  \--*  ADDR      byref 
     (  3,  2) [000072] -------N----              |     \--*  LCL_VAR   struct(P) V03 tmp2         
                                                  |     \--*    ref    V03.Item1 (offs=0x00) -> V08 tmp7         
                                                  |     \--*    ref    V03.Item2 (offs=0x08) -> V09 tmp8         
     (  3,  3) [000078] L----------- this in x0   +--*  ADDR      byref 
     (  3,  2) [000077] ----G--N----              |  \--*  LCL_VAR   struct(AX) V04 tmp3         
               [000080] -----+------ arg1 in x1   +--*  CNS_INT(h) long   0xffff030d5ae8 class
               [000065] -----+------ arg2 in x2   +--*  CNS_INT   int    1
               [000066] -----+------ arg3 in x3   +--*  CNS_INT   int    2
               [000067] -----+------ arg4 in x4   +--*  CNS_INT   int    3
               [000068] -----+------ arg5 in x5   +--*  CNS_INT   int    4
               [000069] -----+------ arg6 in x6   +--*  CNS_INT   int    5
               [000070] -----+------ arg7 in x7   \--*  CNS_INT   int    6

[sdmaclea]

Arm64 is the only platform with FEATURE_MULTIREG_STRUCT_PROMOTE enabled. This modifies code in lvaCanPromoteStructVar() and lvaShouldPromoteStructVar().

I am tempted to just disable it. We could just mark do not enregister in these cases.

[sdmaclea]

@briansull Looks like you enabled FEATURE_MULTIREG_STRUCT_PROMOTE on arm64 in b324afc. Do you think we just need to handle this special case?