Incorrect IND(ADDR(LCL_VAR)) folding when types do not match

[mikedn]

I was looking at reimplementing this kind of folding in LocalAddressVisitor and ran into this morph code:

runtime/src/coreclr/src/jit/morph.cpp

Lines 12859 to 12870 in f621f44

	// If the type of the IND (typ) is a "small int", and the type of the local has the
	// same width, then we can reduce to just the local variable -- it will be
	// correctly normalized, and signed/unsigned differences won't matter.
	//
	// The below transformation cannot be applied if the local var needs to be normalized on load.
	else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) &&
	!lvaTable[lclNum].lvNormalizeOnLoad())
	{
	tree->gtType = typ = temp->TypeGet();
	foldAndReturnTemp = true;
	}
	else if (!varTypeIsStruct(typ) && (lvaTable[lclNum].lvType == typ) &&

I don't think that "signed/unsigned differences won't matter":

static void Main() => Test(-1, 1);

[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe void Test(short a, int b)
{
    short c = (short)(a * 2);
    int d = *((ushort*)&c) / b;
    Console.WriteLine(d);
}

generates

G_M41262_IG01:
       4883EC28             sub      rsp, 40
       448BC2               mov      r8d, edx
G_M41262_IG02:
       480FBFC1             movsx    rax, cx
       D1E0                 shl      eax, 1
       480FBFC0             movsx    rax, ax
       99                   cdq
       41F7F8               idiv     edx:eax, r8d
       8BC8                 mov      ecx, eax
       E8A4FCFFFF           call     System.Console:WriteLine(int)
       90                   nop
G_M41262_IG03:
       4883C428             add      rsp, 40
       C3                   ret

and prints [edit] -2. The result is obviously incorrect, ushort / 1 can't ever be negative.

category:correctness
theme:morph
skill-level:intermediate
cost:medium

[sandreenko]

@dotnet/jit-contrib

[mikedn]

LHS version of the above:

static unsafe void Test(short a, int b)
{
    short c = (short)(a * 2);
    *((ushort*)&c) = (ushort)(a * b);
     Console.WriteLine(c);
}

generates:

480FBFC9             movsx    rcx, cx
0FAFCA               imul     ecx, edx
0FB7C9               movzx    rcx, cx
E8ADFCFFFF           call     System.Console:WriteLine(int)

and prints 65535. This is clearly incorrect as the value of a short variable can't ever be greater than 32767.

Despite the similarity with the RHS version of the bug, this might require a different fix. The pre-morph tree is:

[000013] -A-XG-------              *  ASG       short 
[000012] *------N----              +--*  IND       short 
[000007] ------------              |  \--*  ADDR      long  
[000006] ------------              |     \--*  LCL_VAR   int    V02 loc0         
[000011] ------------              \--*  CAST      int <- ushort <- int
[000010] ------------                 \--*  MUL       int   
[000008] ------------                    +--*  LCL_VAR   short  V00 arg0         
[000009] ------------                    \--*  LCL_VAR   int    V01 arg1         

Note that if the LHS was a LCL_VAR the pre-order morphing of ASG would have inserted a cast to short node on the RHS for "normalize on store". But it's not yet a LCL_VAR and there's also a pre-existing cast to ushort that was imported from IL (BTW why on earth is Roslyn insisting on inserting a narrowing cast before a store that truncates anyway?!)

Post-morph the tree becomes:

[000013] -A--G+------              *  ASG       short 
[000006] D----+-N----              +--*  LCL_VAR   int    V02 loc0         
[000011] -----+------              \--*  CAST      int <- ushort <- int
[000010] -----+------                 \--*  MUL       int   
[000018] -----+------                    +--*  CAST      int <- short <- int
[000008] -----+------                    |  \--*  LCL_VAR   int    V00 arg0         
[000009] -----+------                    \--*  LCL_VAR   int    V01 arg1         

So the problem is the missing normalize on store cast, that would have to be somehow added if we want to continue folding the LHS.

And the fun part is that in this case the signed/unsigned difference the comment talks about can't even be detected - the LHS indir has type short, not ushort, because there are no unsigned stind instructions. In other words, this can't be fixed by changing (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) to the stronger (lvaTable[lclNum].lvType == typ). A fix will likely need to figure out that the indir is LHS or RHS. Talk about getting rid of ASG...

[mikedn]

But it's not yet a LCL_VAR and there's also a pre-existing cast to ushort that was imported from IL (BTW why on earth is Roslyn insisting on inserting a narrowing cast before a store that truncates anyway?!)

And if Roslyn wouldn't generate that narrowing cast then you could make this print ever larger values because the narrowing effect of the stind.i2 is effectively lost:

fgMorphTree BB01, STMT00001 (after)
               [000012] -A--G+------              *  ASG       short 
               [000006] D----+-N----              +--*  LCL_VAR   int    V02 loc0         
               [000010] -----+------              \--*  MUL       int   
               [000017] -----+------                 +--*  CAST      int <- short <- int
               [000008] -----+------                 |  \--*  LCL_VAR   int    V00 arg0         
               [000009] -----+------                 \--*  LCL_VAR   int    V01 arg1         

So Test(32767, 32767); would print 1073676289. Who would have thought that you could stuff 1 billion in a short :)

[mikedn]

Unrelated to this bug but since it's in the same area I'll included this here for completness:
Based on a cursory look at the existing IND(ADD(local)) morphing code I would expect the following to lead to an assert:

unsafe struct Arr
{
    public int len;
    public fixed byte a1[65536];
}

[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe void Test(short a, int b)
{
    Arr r = default;
    Console.WriteLine(r.a1[65535]);
}

The tree is:

[000014] --CXG+------              *  CALL      void   System.Console.WriteLine
[000013] *--XG+------ arg0 in rcx  \--*  IND       ubyte 
[000012] ----G+------                 \--*  ADD       byref 
[000009] ----G+------                    +--*  ADDR      byref 
[000004] ----G+-N----                    |  \--*  LCL_FLD   ubyte  V02 loc0         [+4] Fseq[a1, FixedElementField]
[000011] -----+------                    \--*  CNS_INT   long   0xFFFF

Morph code checks if the ADD offset fits in 16 bit:

runtime/src/coreclr/src/jit/morph.cpp

Line 12944 in f621f44

if (ival1 != (unsigned short)ival1)

but then fails to check if the sum of the ADD offset and LCL_FLD also fits in 16 bit:

runtime/src/coreclr/src/jit/morph.cpp

Line 13010 in f621f44

lclFld->SetLclOffs(lclFld->GetLclOffs() + static_cast<unsigned>(ival1));

So why doesn't the assert in SetLclOffs fire? Because folding doesn't actually take place due to another piece of broken code:

runtime/src/coreclr/src/jit/morph.cpp

Lines 12989 to 12991 in f621f44

	const var_types tempTyp = temp->TypeGet();
	const bool useExactSize = varTypeIsStruct(tempTyp) || (tempTyp == TYP_BLK) || (tempTyp == TYP_LCLBLK);
	const unsigned varSize = useExactSize ? varDsc->lvExactSize : genTypeSize(temp);

This tries to use the indir type to determine the size of the lclvar and thus wrongly concludes that the var size is 1 (= sizeof(byte)) and wrongly concludes that the access is out of bounds.

[mikedn]

Another case:

[MethodImpl(MethodImplOptions.NoInlining)]
static unsafe int Test(short a, int b)
{
    short c = (short)(a * 2);
    Unsafe.As<short, Bytes>(ref c).hi = (byte)b;
    Console.WriteLine(c);
    return c;
}

Prints -65026 which again is out of short's range.

The original tree is:

fgMorphTree BB01, STMT00002 (before)
[000013] -ACXG-------              *  ASG       ubyte 
[000012] --CXG--N----              +--*  FIELD     ubyte  hi
[000018] ------------              |  \--*  ADDR      byref 
[000019] ------------              |     \--*  LCL_VAR   int    V02 loc0         
[000011] ------------              \--*  CAST      int <- ubyte <- int
[000010] ------------                 \--*  LCL_VAR   int    V01 arg1         

and it becomes:

fgMorphTree BB01, STMT00002 (after)
[000013] -A--G+------              *  ASG       ubyte 
[000019] U----+-N----              +--*  LCL_FLD   ubyte  V02 loc0         [+1] Fseq[hi]
[000011] -----+------              \--*  CAST      int <- ubyte <- int
[000010] -----+------                 \--*  LCL_VAR   int    V01 arg1         

The problem is that the variable is "normalize on store" but no normalization is done when such a partial update occurs. We'll probably need to mark the variable "address exposed" just to force it to be "normalize on load". Not ideal but then this is such a corner case that it's unlikely to matter.

[briansull]

Fixed by #40535