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

[davidwrighton]

• 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]

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

[EgorBo]

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]

@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]

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]

@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]

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]

@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]

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]

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]

@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.

[davidwrighton]

Reviewing positively, to allow a revert