OSR failure in System.Text.Json.Tests

[AndyAyersMS]

x64 Ubuntu

export COMPlus_TieredCompilation=1
export COMPlus_TC_OnStackReplacement=1
export COMPlus_TC_QuickJitForLoops=1

  Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 2)
    System.Text.Json.Tests.Utf8JsonReaderTests.ReadJsonStringsWithCommentsAndTrailingCommas(jsonString: "{\"Property1\": {\"Property1.1\": 42} // comment\n"...) [FAIL]

Via COMPlus_JitEnablePatchpointRange I have isolated this down to

System.Text.Json.Tests.Utf8JsonReaderTests::ReadJsonStringsWithCommentsAndTrailingCommas, IL size = 399, hash=0xc1a3563b 
Tier1-OSR @0x159

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

x64 Ubuntu

export COMPlus_TieredCompilation=1
export COMPlus_TC_OnStackReplacement=1
export COMPlus_TC_QuickJitForLoops=1

  Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 2)
    System.Text.Json.Tests.Utf8JsonReaderTests.ReadJsonStringsWithCommentsAndTrailingCommas(jsonString: "{\"Property1\": {\"Property1.1\": 42} // comment\n"...) [FAIL]

Via COMPlus_JitEnablePatchpointRange I have isolated this down to

System.Text.Json.Tests.Utf8JsonReaderTests::ReadJsonStringsWithCommentsAndTrailingCommas, IL size = 399, hash=0xc1a3563b 
Tier1-OSR @0x159

Author:	AndyAyersMS
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[AndyAyersMS]

Ok, I think I know what's going wrong.

We have an independently promoted struct with some byte sized fields. OSR decides it should live on the Tier0 frame. But elseswhere in the jit (notably LSRA) we assume independently promoted fields are register sized.

;  V57 tmp44        [V57,T28] (  2,  66   )     int  ->  [rbp+168H]   tier0-frame V05._maxDepth(offs=0x00) P-INDEP "field V05._maxDepth (fldOffset=0x0)"
;  V58 tmp45        [V58,T24] (  2,  66   )   ubyte  ->  [rbp+16CH]   tier0-frame V05._commentHandling(offs=0x04) P-INDEP "field V05._commentHandling (fldOffset=0x4)"
;  V59 tmp46        [V59,T29] (  2,  66   )    bool  ->  [rbp+16DH]   tier0-frame V05.<AllowTrailingCommas>k__BackingField(offs=0x05) P-INDEP "field V05.<AllowTrailingCommas>k__BackingField (fldOffset=0x5)"

We end up having to spill V58 and in so doing we clobber V59, which happens to be the part of JsonOptions that controls whether trailing commas are ok.

Generating: N001 (  2,  2) [002377] -----------Z      t2377 =    LCL_VAR   ubyte  V58 tmp45         r13 REG r13
IN0207:        mov      dword ptr [V58 rbp+16CH], r13d

and this clobber causes the errors we see at runtime.

Possible options for a fix:
(1) teach LSRA (and maybe more) that memory homes for LSRA local struct fields are not resized actual sized
(2) teach OSR that independently promoted fields do not live on the Tier0 frame, and update the prolog generation to copy from the Tier0 to the Tier1 stack slot when the promoted field is live on entry and not initially assigned to a register.
(3) Bail on independent promotion of OSR locals completely
(4) Don't allocate small independently promoted OSR locals (see below)

[AndyAyersMS]

One possible workaround:

@@ -1536,6 +1536,13 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc)
         return false;
     }
 
+    // Don't enregister small promoted struct fields for OSR locals.
+    //
+    if (varTypeIsSmall(varDsc->lvType) && varDsc->lvIsStructField && compiler->lvaIsOSRLocal(lclNum))
+    {
+        return false;
+    }
+

[AndyAyersMS]

This bit of related code is confusing as the comment and code no longer match (consequence of #52039). We used to use the declared type for the local here, not the stack type. This widens the store to V58 from 8 to 32 bits, as seen above.

runtime/src/coreclr/jit/codegenlinear.cpp

Lines 888 to 893 in e961b10

	// In order for a lclVar to have been allocated to a register, it must not have been aliasable, and can
	// therefore be store-normalized (rather than load-normalized). In fact, not performing store normalization
	// can lead to problems on architectures where a lclVar may be allocated to a register that is not
	// addressable at the granularity of the lclVar's defined type (e.g. x86).
	var_types lclType = varDsc->GetActualRegisterType();
	emitAttr size = emitTypeSize(lclType);

[jtschuster]

Looks like this might have been hit again in rolling build 1874165, but it's on arm (net7.0-Linux-Release-arm-CoreCLR_checked-(Alpine.314.Arm32.Open)[email protected]/dotnet-buildtools/prereqs:alpine-3.14-helix-arm32v7-20210910135806-8a6f4f3).
Log

Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 4)
    System.Text.Json.Tests.Utf8JsonReaderTests.ReadJsonStringsWithComments(jsonString: "{\"Property1\": {\"Property1.1\": 42}, // comment\"...) [FAIL]
      System.InvalidOperationException : End position was not reached during enumeration.
      Stack Trace:
        /_/src/libraries/System.Memory/src/System/ThrowHelper.cs(43,0): at System.ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached()
        /_/src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs(325,0): at System.Text.Json.Utf8JsonReader.GetNextSpan()
        /_/src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs(1561,0): at System.Text.Json.Utf8JsonReader.ConsumeNextTokenMultiSegment(Byte marker)
        /_/src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs(213,0): at System.Text.Json.Utf8JsonReader.ReadMultiSegment()
        /_/src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs(285,0): at System.Text.Json.Utf8JsonReader.Read()
        /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Utf8JsonReaderTests.MultiSegment.cs(2096,0): at System.Text.Json.Tests.Utf8JsonReaderTests.ReadWithCommentsHelper(Utf8JsonReader& reader, Int32 expectedConsumed, Boolean validateThrows)
        /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Utf8JsonReaderTests.MultiSegment.cs(1989,0): at System.Text.Json.Tests.Utf8JsonReaderTests.ReadJsonStringsWithComments(String jsonString)
           at InvokeStub_Utf8JsonReaderTests.ReadJsonStringsWithComments(Object, Object, IntPtr*)
           at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

[AndyAyersMS]

There is no OSR for arm so it's likely a different issue.

[jtschuster]

There is no OSR for arm so it's likely a different issue.

Made a new issue: #72100