Handle getting the address of an RVA static correctly in a composite image #55301

davidwrighton · 2021-07-07T23:18:07Z

Since composite images have the original RVA statics in the original files, use the value from there
This required re-enabling the ability of getFieldAddress to return an indirection
And adding some new processing for the fixup as needed

Fixes composite mode compilation of src/tests/Directed/rvastatics/RVAOrderingTest.ilproj

davidwrighton · 2021-07-07T23:18:57Z

@dotnet/crossgen-contrib @dotnet/jit-contrib I'd like a review from both the codegen and crossgen groups.

EgorBo · 2021-07-07T23:54:23Z

src/coreclr/jit/importer.cpp

+                    // where the actual field does not move from the original file
+                    assert(!varTypeIsGC(lclTyp));
+                    op1 = gtNewOperNode(GT_IND, TYP_I_IMPL, op1);
+                    op1->gtFlags |= GTF_IND_INVARIANT | GTF_IND_NONFAULTING;


there is also GTF_IND_NONNULL that can be used for indirects which return something non-null(0) always

trylek

Looks great to me, thanks for fixing this!

davidwrighton · 2021-07-08T23:29:43Z

@EgorBo The build fails due to an assert in the JIT when targeting some platforms. I've looked in particular at the failure on Windows x86, and it appears to be what I think is a benign assert, but I'm not sure what the appropriate tweak should be to silence it.

With a Windows x86 Debug build, run a command like C:\git\runtime\.dotnet\dotnet.exe "C:\git\runtime\artifacts\bin\coreclr\windows.x86.Debug\\x64\crossgen2\crossgen2.dll" --composite --out C:\temp\spccomposite.dll C:\git\runtime\artifacts\bin\coreclr\windows.x86.Debug\il\System.Private.CoreLib.dll --targetarch x86 --singlemethodtypename "System.Globalization.CharUnicodeInfo" --singlemethodname "GetNumericGraphemeTableOffsetNoBoundsChecks" --singlemethodindex 1 --codegenopt:NGenDump=* -O and it will fail with an assert Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'System.Globalization.CharUnicodeInfo:GetNumericGraphemeTableOffsetNoBoundsChecks(int):int' during 'Emit code' (IL size 117). From what I can see the handling of inlining MemoryMarshal.GetReference doesn't quite work right.

Could you give me some advice?

BruceForstall · 2021-07-08T23:33:40Z

This looks like #51728 and #54007. Basically, recent libraries changes are using so many Unsafe.* constructs, the JIT is getting confused about gc/byref.

davidwrighton · 2021-07-09T00:55:43Z

@BruceForstall I can't disable the test to shut up the issue, as it prevents the build from succeeding. Do you have advice on what to do with the actual jit to silence the assert, or do I just need to wait for a bit to get this fixed? Basically, is the PR that @tannergooding wrote going to fix this soon, or is this yet another issue to chase?

tannergooding · 2021-07-09T02:05:12Z

is the PR that @tannergooding wrote going to fix this soon

No. There ended up being issues with the PR that I'm not going to have time to investigate before 6.0 wraps up.

I think there might be a deeper issue as emitting all moves (that is actually generating code for all moves, where-as my PR tried to only preserve liveness updates for a subset of moves) results in crashes as well. I would expect this shouldn't happen since we shouldn't depend on move elision for the JIT to be correct, only to have more optimal codegen. It wasn't immediately clear if this issue is exclusively related to byref to pointer conversions (or vice-versa) or if its something more in depth and also impacting gcrefs.

BruceForstall · 2021-07-09T16:09:32Z

@davidwrighton Does the failure only exist with this change, or does it also exist in the baseline?

I'm afraid you might just have to wait, and the JIT team might need to pick up @tannergooding 's change and push if further.

AndyAyersMS · 2021-07-09T17:47:17Z

I've said it before but will repeat myself: I think we should just disable this assert. I can't remember it ever catching an actual bug.

BruceForstall · 2021-07-09T20:41:29Z

Maybe that's the best option we have today?

Note there are two similar assertions, both causing issues today:

runtime/src/coreclr/jit/emitxarch.cpp

Lines 12192 to 12200 in ac53e65

    
           #ifdef DEBUG 
        
                                   regMaskTP regMask; 
        
                                   regMask = genRegMask(reg1) | genRegMask(reg2); 
        
                                   // r1/r2 could have been a GCREF as GCREF + int=BYREF 
        
                                   //                            or BYREF+/-int=BYREF 
        
                                   assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) || 
        
                                          ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))); 
        
           #endif

and:

runtime/src/coreclr/jit/emitxarch.cpp

Lines 11884 to 11887 in ac53e65

    
           // The reg must currently be holding either a gcref or a byref 
        
           // and the instruction must be inc or dec 
        
           assert(((emitThisGCrefRegs | emitThisByrefRegs) & regMask) && 
        
                  (ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l));

davidwrighton · 2021-07-14T17:21:34Z

@BruceForstall this failure is new with my change. It tweaks the code to have an extra indirection, and produces the new error. I'm going to update my PR to comment out the assert, with a reference to this PR.

…image - Since composite images have the original RVA statics in the original files, use the value from there - This required re-enabling the ability of getFieldAddress to return an indirection - And adding some new processing for the fixup as needed

…mposite image (dotnet#55301)" This reverts commit e3d319b. It breaks the crossgen2 outerloop runs

davidwrighton

Reviewing positively, to allow a revert

…mposite image (#55301)" (#55713) This reverts commit e3d319b. It breaks the crossgen2 outerloop runs

… in a composite image (dotnet#55301)" (dotnet#55713)" This reverts commit 87fcf6e.

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Jul 7, 2021

davidwrighton requested a review from mangod9 July 7, 2021 23:18

davidwrighton added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 7, 2021

BruceForstall approved these changes Jul 7, 2021

View reviewed changes

EgorBo reviewed Jul 7, 2021

View reviewed changes

trylek approved these changes Jul 8, 2021

View reviewed changes

mangod9 approved these changes Jul 8, 2021

View reviewed changes

davidwrighton added 2 commits July 14, 2021 10:23

Silence pointless warning

2323cf7

davidwrighton force-pushed the rva_ordering branch from 8d112ac to 2323cf7 Compare July 14, 2021 18:01

jit formatting

cf39bb8

davidwrighton merged commit e3d319b into dotnet:main Jul 15, 2021

davidwrighton added a commit to davidwrighton/runtime that referenced this pull request Jul 15, 2021

Revert "Handle getting the address of an RVA static correctly in a co…

c158c3b

…mposite image (dotnet#55301)" This reverts commit e3d319b. It breaks the crossgen2 outerloop runs

davidwrighton commented Jul 15, 2021

View reviewed changes

davidwrighton added a commit that referenced this pull request Jul 15, 2021

Revert "Handle getting the address of an RVA static correctly in a co…

87fcf6e

…mposite image (#55301)" (#55713) This reverts commit e3d319b. It breaks the crossgen2 outerloop runs

davidwrighton added a commit to davidwrighton/runtime that referenced this pull request Jul 16, 2021

Revert "Revert "Handle getting the address of an RVA static correctly…

5929172

… in a composite image (dotnet#55301)" (dotnet#55713)" This reverts commit 87fcf6e.

ManickaP mentioned this pull request Jul 20, 2021

[QUIC] Remove AppContext switch from S.N.Quic #56027

Merged

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

davidwrighton deleted the rva_ordering branch April 13, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle getting the address of an RVA static correctly in a composite image #55301

Handle getting the address of an RVA static correctly in a composite image #55301

davidwrighton commented Jul 7, 2021

davidwrighton commented Jul 7, 2021

EgorBo Jul 7, 2021 •

edited

Loading

trylek left a comment

davidwrighton commented Jul 8, 2021

BruceForstall commented Jul 8, 2021

davidwrighton commented Jul 9, 2021

tannergooding commented Jul 9, 2021

BruceForstall commented Jul 9, 2021

AndyAyersMS commented Jul 9, 2021

BruceForstall commented Jul 9, 2021

davidwrighton commented Jul 14, 2021

davidwrighton left a comment

Handle getting the address of an RVA static correctly in a composite image #55301

Handle getting the address of an RVA static correctly in a composite image #55301

Conversation

davidwrighton commented Jul 7, 2021

davidwrighton commented Jul 7, 2021

EgorBo Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

trylek left a comment

Choose a reason for hiding this comment

davidwrighton commented Jul 8, 2021

BruceForstall commented Jul 8, 2021

davidwrighton commented Jul 9, 2021

tannergooding commented Jul 9, 2021

BruceForstall commented Jul 9, 2021

AndyAyersMS commented Jul 9, 2021

BruceForstall commented Jul 9, 2021

davidwrighton commented Jul 14, 2021

davidwrighton left a comment

Choose a reason for hiding this comment

EgorBo Jul 7, 2021 •

edited

Loading