Change OperIsBlkOp test to check for store block operators

[yowl]

This PR removes the check for a Blk operation to have Data() != nullptr i.e. (gtOp2 != nullptr). If a GT_LCL_VAR is converted to a GT_OBJ gtOp2 will not be set and when dumping the tree a null reference is encountered. This comes from this question over in the runtimelab repo: dotnet/runtimelab#1673 where you can see the stack and context.

Instead the check on the right side of the || is just OperIsStoreBlk()

[ghost]

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

Issue Details

---

This PR removes the check for a Blk operation to have Data() != nullptr i.e. (gtOp2 != nullptr). If a GT_LCL_VAR is converted to a GT_OBJ gtOp2 will not be set and when dumping the tree a null reference is encountered. This comes from this question over in the runtimelab repo: dotnet/runtimelab#1673 where you can see the stack and context.

Instead the check on the right side of the || is just OperIsStoreBlk()

Author:	yowl
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[yowl]

This failed on what could be an intermittent problem #25979

[AndyAyersMS]

cc @dotnet/jit-contrib

[yowl]

Going to close and reopen. The crypto failure mentioned above is apparently very rare, so if there's nothing wrong with this actual change it should pass....

[JulieLeeMSFT]

@echesakovMSFT PTAL the community PR related to store block.

[echesakov]

@yowl Do you have a sample code where the issue can be reproduced?

[JulieLeeMSFT]

@yowl, @echesakovMSFT needs more info from you.

Do you have a sample code where the issue can be reproduced?

[yowl]

Yes, sorry for the delay. Attempt to compile https://github.com/dotnet/runtimelab/blob/dd8c43a825c41ccfc21a8ceefa79e232adbe02a1/src/libraries/System.Private.CoreLib/src/System/Span.cs#L117 after LLVM lowering and dump the tree. So in the rsp for the project add

--codegenopt:NgenDump=*

And make sure the code will exercise this ctor. I'm using the NativeAOT generics project for this. https://github.com/dotnet/runtimelab/blob/feature/NativeAOT-LLVM/src/tests/nativeaot/SmokeTests/Generics/Generics.csproj

Then after LLVM lowering it will dump the tree and fail with the dump stopping here at the GT_OBJ:

Trees after LLVM Shadow Stack Setup

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [???..???)                                     keep i internal LIR
BB02 [0001]  1       BB01                  1       [000..022)        (return)                     i hascall gcsafe LIR
-----------------------------------------------------------------------------------------------------------------------------------------

------------ BB01 [???..???), preds={} succs={BB02}
N001 (  0,  0) [000000] ------------                 NOP       void

------------ BB02 [000..022) (return), preds={BB01} succs={}
               [000031] ------------                 IL_OFFSET void   IL offset: 0x0
N001 (  1,  1) [000001] ------------                 NO_OP     void
               [000032] ------------                 IL_OFFSET void   IL offset: 0x1
N002 (  3,  2) [000002] ------------         t2 =    LCL_VAR   int    V02 arg2
N003 (  1,  1) [000003] ------------         t3 =    CNS_INT   int    0
                                                  /--*  t2     int
                                                  +--*  t3     int
N004 (  8,  4) [000004] ------------         t4 = *  GE        int
               [000039] ------------        t39 =    LCL_VAR   int    V05 tmp2
               [000040] ------------        t40 =    CNS_INT   int    12
                                                  /--*  t39    int
                                                  +--*  t40    int
               [000041] ------------        t41 = *  ADD       int
                                                  /--*  t41    int
               [000042] ------------        t42 = *  PUTARG_TYPE int
                                                  /--*  t4     int
               [000043] ------------        t43 = *  PUTARG_TYPE bool
                                                  /--*  t42    int    arg-1
                                                  +--*  t43    bool   arg0
N005 ( 22,  7) [000007] --CXG-------              *  CALL      void   System.Diagnostics.Debug.Assert
               [000033] ------------                 IL_OFFSET void   IL offset: 0xd
N001 (  1,  1) [000009] ------------                 NO_OP     void
               [000034] ------------                 IL_OFFSET void   IL offset: 0xe
               [000044] ------------        t44 =    CNS_INT   int    4
               [000045] ------------        t45 =    LCL_VAR   int    V05 tmp2
                                                  /--*  t45    int
                                                  +--*  t44    int
               [000046] ------------        t46 = *  ADD       int
                                                  /--*  t46    int
N001 (  3,  2) [000011] ------------        t11 = *  IND       byref
                                                  /--*  t11    byref
N003 (  7,  7) [000015] DA----------              *  STORE_LCL_FLD byref  V04 tmp1         [+0] Fseq[_value]
               [000035] ------------                 IL_OFFSET void   IL offset: 0x15
               [000047] ------------        t47 =    CNS_INT   int    0
               [000048] ------------        t48 =    LCL_VAR   int    V05 tmp2
                                                  /--*  t48    int
                                                  +--*  t47    int
               [000049] ------------        t49 = *  ADD       int
                                                  /--*  t49    int
N001 (  3,  2) [000010] ------------        t10 = *  IND       byref  Zero Fseq[_pointer]
               [000050] ------------        t50 =    CNS_INT   int    8
               [000051] ------------        t51 =    LCL_VAR   int    V05 tmp2
                                                  /--*  t51    int
                                                  +--*  t50    int
               [000052] ------------        t52 = *  ADD       int
                                                  /--*  t52    int
N003 (  3,  2) [000016] -------N----        t16 = *  OBJ       struct<System.ByReference`1[System.Char], 4>

Because Data is not set on the GT_OBJ, it may not be nullptr, OperIsBlkOp returns true and it then tries to indirect Data() at AsBlk()->Data()->gtSkipReloadOrCopy() which fails (as the Data() is garbage). GT_OBJ has no gtOp2. This change ensure that no attempt is made to Data() for ops that don't have a gtOp2.

[echesakov]

@yowl Thanks for the snippet - I will take a look.

[yowl]

Thanks for looking.
Just to be clear, you need the NativeAOT-LLVM branch for this. Its experimental but where a change is found that is considered potentially beneficial upstream, i.e. here, then a PR is created for feedback. I don't think you can reproduce this with the main branch of clrjit as you need to change the operation. NativeAOT-LLVM does that with this

        switch (node->OperGet())
        {
            case GT_STORE_LCL_VAR:
                indirOper = lclVar->TypeIs(TYP_STRUCT) ? GT_STORE_OBJ : GT_STOREIND;
                storedValue = node->AsOp()->gtGetOp1();
                break;
            case GT_LCL_VAR:
                indirOper = lclVar->TypeIs(TYP_STRUCT) ? GT_OBJ : GT_IND;
                break;
            default:
                unreached();
        }
        node->ChangeOper(indirOper);

[JulieLeeMSFT]

Ping @echesakovMSFT for further review.

[echesakov]

LGTM