From 6bc83f1b4e1f6f03cfd4a4548ac299364313b290 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 23 Jun 2022 20:12:16 -0700 Subject: [PATCH 1/6] JIT: fix double reporting of some GC frame slots If there is a gc struct local that is dependently promoted, the struct local may be untracked while the promoted gc fields of the struct are tracked. If so, the jit will double report the stack offset for the gc field, first as an untracked slot, and then as a tracked slot. Detect this case and report the slot as tracked only. Closes #71005. --- src/coreclr/jit/gcencode.cpp | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index f9a5e65b6015e7..45e6ba5c075faf 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4226,7 +4226,30 @@ void GCInfo::gcMakeRegPtrTable( continue; } - int offset = varDsc->GetStackOffset() + i * TARGET_POINTER_SIZE; + const unsigned int fieldOffset = i * TARGET_POINTER_SIZE; + + if (varDsc->lvPromoted) + { + assert(compiler->lvaGetPromotionType(varDsc) == Compiler::PROMOTION_TYPE_DEPENDENT); + + // See if this untracked dependently promoted struct has + // a tracked field at this offset. + // + unsigned const fieldLclNum = compiler->lvaGetFieldLocal(varDsc, fieldOffset); + LclVarDsc* const fieldVarDsc = compiler->lvaGetDesc(fieldLclNum); + assert(varTypeIsGC(fieldVarDsc->TypeGet())); + + if (fieldVarDsc->lvTracked) + { + JITDUMP("Untracked GC struct V%02 has tracked gc ref at + 0x%x (P-DEP promoted V%02u); will " + "report slot as tracked", + varNum, fieldOffset, fieldLclNum); + continue; + } + } + + int const offset = varDsc->GetStackOffset() + fieldOffset; + #if DOUBLE_ALIGN // For genDoubleAlign(), locals are addressed relative to ESP and // arguments are addressed relative to EBP. From fbb69f57ba9fa2eb87abd1af0504395154365fc0 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Jun 2022 14:57:55 -0700 Subject: [PATCH 2/6] revise --- src/coreclr/jit/emit.h | 5 +++++ src/coreclr/jit/gcencode.cpp | 25 ++++++++++++++----------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index d44fd1bd572eea..62fd1d4d972f01 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2368,6 +2368,11 @@ class emitter void emitSetFrameRangeLcls(int offsLo, int offsHi); void emitSetFrameRangeArgs(int offsLo, int offsHi); + bool emitIsWithinFrameRangeGCRs(int offs) + { + return (offs >= emitGCrFrameOffsMin) && (offs <= emitGCrFrameOffsMax); + } + static instruction emitJumpKindToIns(emitJumpKind jumpKind); static emitJumpKind emitInsToJumpKind(instruction ins); static emitJumpKind emitReverseJumpKind(emitJumpKind jumpKind); diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 45e6ba5c075faf..b2f298834dae80 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4226,30 +4226,33 @@ void GCInfo::gcMakeRegPtrTable( continue; } - const unsigned int fieldOffset = i * TARGET_POINTER_SIZE; + unsigned const fieldOffset = i * TARGET_POINTER_SIZE; + int const offset = varDsc->GetStackOffset() + fieldOffset; if (varDsc->lvPromoted) { assert(compiler->lvaGetPromotionType(varDsc) == Compiler::PROMOTION_TYPE_DEPENDENT); - // See if this untracked dependently promoted struct has - // a tracked field at this offset. + // For OSR, a dependently promoted tracked gc local can end up in the gc tracked + // frame range, because OSR always can't fully honor frame layout constraints. // - unsigned const fieldLclNum = compiler->lvaGetFieldLocal(varDsc, fieldOffset); + // Detect this case and report the slot as tracked. + // + unsigned const fieldLclNum = compiler->lvaGetFieldLocal(varDsc, fieldOffset); + assert(fieldLclNum != BAD_VAR_NUM); LclVarDsc* const fieldVarDsc = compiler->lvaGetDesc(fieldLclNum); - assert(varTypeIsGC(fieldVarDsc->TypeGet())); - if (fieldVarDsc->lvTracked) + if (compiler->lvaIsGCTracked(fieldVarDsc) && + compiler->GetEmitter()->emitIsWithinFrameRangeGCRs(offset)) { - JITDUMP("Untracked GC struct V%02 has tracked gc ref at + 0x%x (P-DEP promoted V%02u); will " - "report slot as tracked", - varNum, fieldOffset, fieldLclNum); + assert(compiler->opts.IsOSR()); + JITDUMP("Untracked GC struct slot V%02u+%u (P-DEP promoted V%02u) is at frame offset %d within " + "tracked ref range.\nWill report slot as tracked\n", + varNum, fieldOffset, fieldLclNum, offset); continue; } } - int const offset = varDsc->GetStackOffset() + fieldOffset; - #if DOUBLE_ALIGN // For genDoubleAlign(), locals are addressed relative to ESP and // arguments are addressed relative to EBP. From 5a9c94074d2d6977832ebc4d1b9084c2036cac1d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Jun 2022 16:34:59 -0700 Subject: [PATCH 3/6] exclude upper end of range --- src/coreclr/jit/emit.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 62fd1d4d972f01..fbcfe117a01641 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2370,7 +2370,7 @@ class emitter bool emitIsWithinFrameRangeGCRs(int offs) { - return (offs >= emitGCrFrameOffsMin) && (offs <= emitGCrFrameOffsMax); + return (offs >= emitGCrFrameOffsMin) && (offs < emitGCrFrameOffsMax); } static instruction emitJumpKindToIns(emitJumpKind jumpKind); From f183702d606add7868e65bfdccd1661403dc4d8b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 24 Jun 2022 18:54:14 -0700 Subject: [PATCH 4/6] restrict to OSR only --- src/coreclr/jit/gcencode.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index b2f298834dae80..6d9076eb22cde7 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4242,10 +4242,9 @@ void GCInfo::gcMakeRegPtrTable( assert(fieldLclNum != BAD_VAR_NUM); LclVarDsc* const fieldVarDsc = compiler->lvaGetDesc(fieldLclNum); - if (compiler->lvaIsGCTracked(fieldVarDsc) && + if (compiler->opts.IsOSR() && compiler->lvaIsGCTracked(fieldVarDsc) && compiler->GetEmitter()->emitIsWithinFrameRangeGCRs(offset)) { - assert(compiler->opts.IsOSR()); JITDUMP("Untracked GC struct slot V%02u+%u (P-DEP promoted V%02u) is at frame offset %d within " "tracked ref range.\nWill report slot as tracked\n", varNum, fieldOffset, fieldLclNum, offset); From 1bf699e65e03123f08f9585f532122dbb0e51f47 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 27 Jun 2022 11:50:09 -0700 Subject: [PATCH 5/6] revise and report as untracked instead of tracked --- src/coreclr/jit/compiler.hpp | 4 ++-- src/coreclr/jit/gcencode.cpp | 13 +++++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index f6fa0e2d946b64..5f26565b6ae003 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4005,7 +4005,7 @@ inline bool Compiler::lvaIsFieldOfDependentlyPromotedStruct(const LclVarDsc* var // This is because struct variables are never tracked as a whole for GC purposes. // It is up to the caller to ensure that the fields of struct variables are // correctly tracked. -// On Amd64, we never GC-track fields of dependently promoted structs, even +// On Amd64/Arm64, we never GC-track fields of dependently promoted structs, even // though they may be tracked for optimization purposes. // It seems that on x86 and arm, we simply don't track these // fields, though I have not verified that. I attempted to make these GC-tracked, @@ -4021,7 +4021,7 @@ inline bool Compiler::lvaIsGCTracked(const LclVarDsc* varDsc) #ifdef TARGET_AMD64 return !isStackParam && !lvaIsFieldOfDependentlyPromotedStruct(varDsc); #else // !TARGET_AMD64 - return !isStackParam; + return !isStackParam && !lvaIsFieldOfDependentlyPromotedStruct(varDsc); #endif // !TARGET_AMD64 } else diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 6d9076eb22cde7..fbc3fb6a7e1c9b 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4233,22 +4233,19 @@ void GCInfo::gcMakeRegPtrTable( { assert(compiler->lvaGetPromotionType(varDsc) == Compiler::PROMOTION_TYPE_DEPENDENT); - // For OSR, a dependently promoted tracked gc local can end up in the gc tracked - // frame range, because OSR always can't fully honor frame layout constraints. - // - // Detect this case and report the slot as tracked. + // A dependently promoted tracked gc local can end up in the gc tracked + // frame range. If so it should be excluded from tracking via lvaIsGCTracked. // unsigned const fieldLclNum = compiler->lvaGetFieldLocal(varDsc, fieldOffset); assert(fieldLclNum != BAD_VAR_NUM); LclVarDsc* const fieldVarDsc = compiler->lvaGetDesc(fieldLclNum); - if (compiler->opts.IsOSR() && compiler->lvaIsGCTracked(fieldVarDsc) && - compiler->GetEmitter()->emitIsWithinFrameRangeGCRs(offset)) + if (compiler->GetEmitter()->emitIsWithinFrameRangeGCRs(offset)) { + assert(!compiler->lvaIsGCTracked(fieldVarDsc)); JITDUMP("Untracked GC struct slot V%02u+%u (P-DEP promoted V%02u) is at frame offset %d within " - "tracked ref range.\nWill report slot as tracked\n", + "tracked ref range; will report slot as untracked\n", varNum, fieldOffset, fieldLclNum, offset); - continue; } } From c869f41eaa385f6fa3c9833db07192af3b99fa3c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 27 Jun 2022 12:22:51 -0700 Subject: [PATCH 6/6] revise per feedback --- src/coreclr/jit/compiler.hpp | 13 +++---------- src/coreclr/jit/gcencode.cpp | 2 ++ 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 5f26565b6ae003..41624dc24867eb 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4005,24 +4005,17 @@ inline bool Compiler::lvaIsFieldOfDependentlyPromotedStruct(const LclVarDsc* var // This is because struct variables are never tracked as a whole for GC purposes. // It is up to the caller to ensure that the fields of struct variables are // correctly tracked. -// On Amd64/Arm64, we never GC-track fields of dependently promoted structs, even +// +// We never GC-track fields of dependently promoted structs, even // though they may be tracked for optimization purposes. -// It seems that on x86 and arm, we simply don't track these -// fields, though I have not verified that. I attempted to make these GC-tracked, -// but there was too much logic that depends on these being untracked, so changing -// this would require non-trivial effort. - +// inline bool Compiler::lvaIsGCTracked(const LclVarDsc* varDsc) { if (varDsc->lvTracked && (varDsc->lvType == TYP_REF || varDsc->lvType == TYP_BYREF)) { // Stack parameters are always untracked w.r.t. GC reportings const bool isStackParam = varDsc->lvIsParam && !varDsc->lvIsRegArg; -#ifdef TARGET_AMD64 return !isStackParam && !lvaIsFieldOfDependentlyPromotedStruct(varDsc); -#else // !TARGET_AMD64 - return !isStackParam && !lvaIsFieldOfDependentlyPromotedStruct(varDsc); -#endif // !TARGET_AMD64 } else { diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index fbc3fb6a7e1c9b..e85066b64ed59f 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4229,6 +4229,7 @@ void GCInfo::gcMakeRegPtrTable( unsigned const fieldOffset = i * TARGET_POINTER_SIZE; int const offset = varDsc->GetStackOffset() + fieldOffset; +#ifdef DEBUG if (varDsc->lvPromoted) { assert(compiler->lvaGetPromotionType(varDsc) == Compiler::PROMOTION_TYPE_DEPENDENT); @@ -4248,6 +4249,7 @@ void GCInfo::gcMakeRegPtrTable( varNum, fieldOffset, fieldLclNum, offset); } } +#endif #if DOUBLE_ALIGN // For genDoubleAlign(), locals are addressed relative to ESP and