Do not encode safe points with -1 offset. #104336
Conversation
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
These are timing out currently in other PRs as well. The rest is green. Including |
|
CC: @dotnet/jit-contrib |
| { | ||
| noway_assert(varDsc->lvIsParam); | ||
|
|
||
| gcInfo.gcMarkRegPtrVal(varDsc->GetArgReg(), varDsc->TypeGet()); |
There was a problem hiding this comment.
We should have correct GC info at the label after GS cookie check as cookie check has no effect on GC liveness. (maybe it did in the past?)
This was just noop or was messing things up by forcing argument registers to be GC-alive when they would not have GC references in them. Perhaps it was compensating for some behavior that is long gone.
There was a problem hiding this comment.
This was hidden because we did not allow interrupting thread on this instruction in a leaf frame (but we can and should) and in nonleaf this would not show up because this is a tailcall.
| @@ -594,8 +594,7 @@ bool emitter::emitGenNoGCLst(Callback& cb) | |||
| emitter::instrDesc* id = emitFirstInstrDesc(ig->igData); | |||
| assert(id != nullptr); | |||
| assert(id->idCodeSize() > 0); | |||
| if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), | |||
| ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG))) | |||
There was a problem hiding this comment.
I realized that including epilogs in this set was just compensating for the bug related to GS cookies and for overly conservative asserts in NativeAOT hijacker.
It is fairly common on ARM64 to see:
bl <somewhere>
ldp <some regs> // <-- this IP is certainly unwindable, how else can we unwind from the call above?
ldp <more regs>
ldp fp, lr, [sp], #0x60The first instruction of an epilog is no different from a first instruction in a regular no-GC region.
We have not executed any instructions yet that destroy unwindability or GC-safeness.
| @@ -678,8 +678,8 @@ void FASTCALL decodeCallPattern(int pattern, | |||
| #define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>2) | |||
| #define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<2) | |||
| #define CODE_OFFSETS_NEED_NORMALIZATION 1 | |||
| #define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 2/4 bytes long in Thumb/ARM states, | |||
There was a problem hiding this comment.
Matching change will need to be made in https://github.com/dotnet/diagnostics/tree/main/src/shared/gcdump so that GC info dumping in SOS works.
There is a challenge though: SOS wants to support all runtimes, so the SOS copy will need to work for both old and new GC info encoding - we will need to switch based on the current runtime version somehow.
There was a problem hiding this comment.
Also, same problem exists for https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64
There was a problem hiding this comment.
Also, same problem exists for https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64
There is no corresponding location for arm[64]. Do we use Amd64 for all cases?
It is the RISC arches that are more interesting, since the encoding size changes.
Figured that:
// Arm, Arm64, LoongArch64 and RISCV64 use the same GcInfo format as Amd64The arch specific behavior is done via consulting with _readyToRunReader.Machine`
There was a problem hiding this comment.
I think there are no changes needed on x64 for the uses of GcInfo that do enumeration. The offsets are different, but that is a matter of FindSafePoint(offset) kind of use, as the offset would need to no longer do -1 adjustment in nonleaf cases. I do not see such use in ReadyToRun thing.
Basically - for the use like foreach(safePoint in ...) {foreach(slot in SlotsAt(safePoint)} , on x64 everything should work as before.
There was a problem hiding this comment.
The RISC is more interesting as GcInfo deserializer would need to denormalize code offsets based on GcInfo version. Not just the offsets used in safepoints, there are other places.
I would expect that GcInfo deserializer that wants to support different runtimes already needs to figure the version of GCInfo. This is not the first time it changes.
There was a problem hiding this comment.
The diagnostics/SOS GcInfo deserializer looks like just a copy of the one in the runtime. It is version-aware (can handle v1 and v2) and gets the version from the supplied token.
We may just need to copy/paste the new version once this change goes in.
Logged a follow-up issue for that: dotnet/diagnostics#4795
|
Thanks!! |
This reverts commit fa77959.
* Partial re-revert of #104336. Only JIT fixes are included. * fix for stress issues * comments and some cleanup * JIT format * keep existing code for x86
When the code position after a call site is a safe point, its GC info is now applicable regardless of how instruction pointer has arrived at this position - by executing a call (and unwinding), returning from a call or jumping around the call,...
That holds in both fully and partially interruptible code.
Therefore leaf/nonleaf/faulted-in-partial/faulted-in-interruptible scenarios all can work the same and we no longer need to resort to various disambiguation schemes.
In particular we do not need to pre-adjust safe point positions in partially interruptible code by
-1, into the middle of a previous instruction, and then carefully compensate for that in bunch of places.An additional benefit is that code offsets in GC info are now all instruction-aligned, thus on RISC architectures fewer bits could be used to encode those.
Fixes: #5677
======== Size impact of this change on arm64:
Using NativeAOT compiled
System.Collections.Concurrent.Tests.exefor Win-arm64 as an exampleBefore the change:
21,920,768 bytes
After the change:
21,779,456 bytes
executable size reduction is roughly
141K(0.6%)NB: the change does not affect the legacy X86 GC info