Skip to content

Commit af8c618

Browse files
jakobbotschjakobbotsch
committed
Revert "Do not encode safe points with -1 offset. (dotnet#104336)"
This reverts commit fa77959.
1 parent 7c3286f commit af8c618

File tree

13 files changed

+234
-99
lines changed

13 files changed

+234
-99
lines changed

‎src/coreclr/gcinfo/gcinfoencoder.cpp

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,19 +1216,42 @@ void GcInfoEncoder::Build()
12161216

12171217
///////////////////////////////////////////////////////////////////////
12181218
// Normalize call sites
1219+
// Eliminate call sites that fall inside interruptible ranges
12191220
///////////////////////////////////////////////////////////////////////
12201221

1221-
_ASSERTE(m_NumCallSites == 0 || numInterruptibleRanges == 0);
1222-
12231222
UINT32 numCallSites = 0;
12241223
for(UINT32 callSiteIndex = 0; callSiteIndex < m_NumCallSites; callSiteIndex++)
12251224
{
12261225
UINT32 callSite = m_pCallSites[callSiteIndex];
1227-
callSite += m_pCallSiteSizes[callSiteIndex];
1226+
// There's a contract with the EE that says for non-leaf stack frames, where the
1227+
// method is stopped at a call site, the EE will not query with the return PC, but
1228+
// rather the return PC *minus 1*.
1229+
// The reason is that variable/register liveness may change at the instruction immediately after the
1230+
// call, so we want such frames to appear as if they are "within" the call.
1231+
// Since we use "callSite" as the "key" when we search for the matching descriptor, also subtract 1 here
1232+
// (after, of course, adding the size of the call instruction to get the return PC).
1233+
callSite += m_pCallSiteSizes[callSiteIndex] - 1;
12281234

12291235
_ASSERTE(DENORMALIZE_CODE_OFFSET(NORMALIZE_CODE_OFFSET(callSite)) == callSite);
12301236
UINT32 normOffset = NORMALIZE_CODE_OFFSET(callSite);
1231-
m_pCallSites[numCallSites++] = normOffset;
1237+
1238+
BOOL keepIt = TRUE;
1239+
1240+
for(UINT32 intRangeIndex = 0; intRangeIndex < numInterruptibleRanges; intRangeIndex++)
1241+
{
1242+
InterruptibleRange *pRange = &pRanges[intRangeIndex];
1243+
if(pRange->NormStopOffset > normOffset)
1244+
{
1245+
if(pRange->NormStartOffset <= normOffset)
1246+
{
1247+
keepIt = FALSE;
1248+
}
1249+
break;
1250+
}
1251+
}
1252+
1253+
if(keepIt)
1254+
m_pCallSites[numCallSites++] = normOffset;
12321255
}
12331256

12341257
GCINFO_WRITE_VARL_U(m_Info1, NORMALIZE_NUM_SAFE_POINTS(numCallSites), NUM_SAFE_POINTS_ENCBASE, NumCallSitesSize);
@@ -1395,7 +1418,7 @@ void GcInfoEncoder::Build()
13951418

13961419
for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
13971420
{
1398-
if(pCurrent->CodeOffset >= callSite)
1421+
if(pCurrent->CodeOffset > callSite)
13991422
{
14001423
couldBeLive |= liveState;
14011424

@@ -1750,7 +1773,7 @@ void GcInfoEncoder::Build()
17501773
{
17511774
for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
17521775
{
1753-
if(pCurrent->CodeOffset >= callSite)
1776+
if(pCurrent->CodeOffset > callSite)
17541777
{
17551778
// Time to record the call site
17561779

@@ -1849,7 +1872,7 @@ void GcInfoEncoder::Build()
18491872

18501873
for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
18511874
{
1852-
if(pCurrent->CodeOffset >= callSite)
1875+
if(pCurrent->CodeOffset > callSite)
18531876
{
18541877
// Time to encode the call site
18551878

@@ -1896,7 +1919,7 @@ void GcInfoEncoder::Build()
18961919

18971920
for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
18981921
{
1899-
if(pCurrent->CodeOffset >= callSite)
1922+
if(pCurrent->CodeOffset > callSite)
19001923
{
19011924
// Time to encode the call site
19021925
GCINFO_WRITE_VECTOR(m_Info1, liveState, CallSiteStateSize);

‎src/coreclr/inc/gcinfotypes.h

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -678,8 +678,8 @@ void FASTCALL decodeCallPattern(int pattern,
678678
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>2)
679679
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<2)
680680
#define CODE_OFFSETS_NEED_NORMALIZATION 1
681-
#define NORMALIZE_CODE_OFFSET(x) ((x)>>1) // Instructions are 2/4 bytes long in Thumb/ARM states,
682-
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<1)
681+
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 2/4 bytes long in Thumb/ARM states,
682+
#define DENORMALIZE_CODE_OFFSET(x) (x) // but the safe-point offsets are encoded with a -1 adjustment.
683683
#define NORMALIZE_REGISTER(x) (x)
684684
#define DENORMALIZE_REGISTER(x) (x)
685685
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
@@ -734,9 +734,9 @@ void FASTCALL decodeCallPattern(int pattern,
734734
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x)^29)
735735
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
736736
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
737-
#define CODE_OFFSETS_NEED_NORMALIZATION 1
738-
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
739-
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
737+
#define CODE_OFFSETS_NEED_NORMALIZATION 0
738+
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
739+
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
740740
#define NORMALIZE_REGISTER(x) (x)
741741
#define DENORMALIZE_REGISTER(x) (x)
742742
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
@@ -789,9 +789,9 @@ void FASTCALL decodeCallPattern(int pattern,
789789
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 22 : 3)
790790
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
791791
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
792-
#define CODE_OFFSETS_NEED_NORMALIZATION 1
793-
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
794-
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
792+
#define CODE_OFFSETS_NEED_NORMALIZATION 0
793+
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
794+
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
795795
#define NORMALIZE_REGISTER(x) (x)
796796
#define DENORMALIZE_REGISTER(x) (x)
797797
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
@@ -844,9 +844,9 @@ void FASTCALL decodeCallPattern(int pattern,
844844
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 8 : 2)
845845
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
846846
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
847-
#define CODE_OFFSETS_NEED_NORMALIZATION 1
848-
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
849-
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
847+
#define CODE_OFFSETS_NEED_NORMALIZATION 0
848+
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
849+
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
850850
#define NORMALIZE_REGISTER(x) (x)
851851
#define DENORMALIZE_REGISTER(x) (x)
852852
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)

‎src/coreclr/jit/codegencommon.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1517,6 +1517,29 @@ void CodeGen::genExitCode(BasicBlock* block)
15171517
if (compiler->getNeedsGSSecurityCookie())
15181518
{
15191519
genEmitGSCookieCheck(jmpEpilog);
1520+
1521+
if (jmpEpilog)
1522+
{
1523+
// Dev10 642944 -
1524+
// The GS cookie check created a temp label that has no live
1525+
// incoming GC registers, we need to fix that
1526+
1527+
unsigned varNum;
1528+
LclVarDsc* varDsc;
1529+
1530+
/* Figure out which register parameters hold pointers */
1531+
1532+
for (varNum = 0, varDsc = compiler->lvaTable; varNum < compiler->lvaCount && varDsc->lvIsRegArg;
1533+
varNum++, varDsc++)
1534+
{
1535+
noway_assert(varDsc->lvIsParam);
1536+
1537+
gcInfo.gcMarkRegPtrVal(varDsc->GetArgReg(), varDsc->TypeGet());
1538+
}
1539+
1540+
GetEmitter()->emitThisGCrefRegs = GetEmitter()->emitInitGCrefRegs = gcInfo.gcRegGCrefSetCur;
1541+
GetEmitter()->emitThisByrefRegs = GetEmitter()->emitInitByrefRegs = gcInfo.gcRegByrefSetCur;
1542+
}
15201543
}
15211544

15221545
genReserveEpilog(block);

‎src/coreclr/jit/emit.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10427,9 +10427,9 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper)
1042710427
// of the last instruction in the region makes GC safe again.
1042810428
// In particular - once the IP is on the first instruction, but not executed it yet,
1042910429
// it is still safe to do GC.
10430-
// The only special case is when NoGC region is used for prologs.
10431-
// In such case the GC info could be incorrect until the prolog completes, so the first
10432-
// instruction cannot have GC.
10430+
// The only special case is when NoGC region is used for prologs/epilogs.
10431+
// In such case the GC info could be incorrect until the prolog completes and epilogs
10432+
// may have unwindability restrictions, so the first instruction cannot have GC.
1043310433

1043410434
void emitter::emitDisableGC()
1043510435
{

‎src/coreclr/jit/emitinl.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,8 @@ bool emitter::emitGenNoGCLst(Callback& cb)
594594
emitter::instrDesc* id = emitFirstInstrDesc(ig->igData);
595595
assert(id != nullptr);
596596
assert(id->idCodeSize() > 0);
597-
if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), ig->igFlags & (IGF_FUNCLET_PROLOG)))
597+
if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(),
598+
ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG)))
598599
{
599600
return false;
600601
}

‎src/coreclr/jit/gcencode.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4027,7 +4027,8 @@ class InterruptibleRangeReporter
40274027
// Report everything between the previous region and the current
40284028
// region as interruptible.
40294029

4030-
bool operator()(unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog)
4030+
bool operator()(
4031+
unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInPrologOrEpilog)
40314032
{
40324033
if (igOffs < m_uninterruptibleEnd)
40334034
{
@@ -4043,7 +4044,7 @@ class InterruptibleRangeReporter
40434044
// Once the first instruction in IG executes, we cannot have GC.
40444045
// But it is ok to have GC while the IP is on the first instruction, unless we are in prolog/epilog.
40454046
unsigned interruptibleEnd = igOffs;
4046-
if (!isInProlog)
4047+
if (!isInPrologOrEpilog)
40474048
{
40484049
interruptibleEnd += firstInstrSize;
40494050
}

‎src/coreclr/nativeaot/Runtime/thread.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -675,16 +675,10 @@ void Thread::HijackCallback(NATIVE_CONTEXT* pThreadContext, void* pThreadToHijac
675675
if (runtime->IsConservativeStackReportingEnabled() ||
676676
codeManager->IsSafePoint(pvAddress))
677677
{
678-
// IsUnwindable is precise on arm64, but can give false negatives on other architectures.
679-
// (when IP is on the first instruction of an epilog, we still can unwind,
680-
// but we can tell if the instruction is the first only if we can navigate instructions backwards and check)
681-
// The preciseness of IsUnwindable is tracked in https://github.com/dotnet/runtime/issues/101932
682-
#if defined(TARGET_ARM64)
683678
// we may not be able to unwind in some locations, such as epilogs.
684679
// such locations should not contain safe points.
685680
// when scanning conservatively we do not need to unwind
686681
ASSERT(codeManager->IsUnwindable(pvAddress) || runtime->IsConservativeStackReportingEnabled());
687-
#endif
688682

689683
// if we are not given a thread to hijack
690684
// perform in-line wait on the current thread

‎src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,18 +213,44 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
213213

214214
#ifdef TARGET_ARM
215215
// Ensure that code offset doesn't have the Thumb bit set. We need
216-
// it to be aligned to instruction start
216+
// it to be aligned to instruction start to make the !isActiveStackFrame
217+
// branch below work.
217218
ASSERT(((uintptr_t)codeOffset & 1) == 0);
218219
#endif
219220

221+
bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;
222+
223+
if (!isActiveStackFrame && !executionAborted)
224+
{
225+
// the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
226+
codeOffset--;
227+
}
228+
220229
GcInfoDecoder decoder(
221230
GCInfoToken(gcInfo),
222231
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
223232
codeOffset
224233
);
225234

235+
if (isActiveStackFrame)
236+
{
237+
// CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here.
238+
// Or, better yet, maybe we should change the decoder to not require this adjustment.
239+
// The scenario that adjustment tries to handle (fallthrough into BB with random liveness)
240+
// does not seem possible.
241+
if (!decoder.HasInterruptibleRanges())
242+
{
243+
decoder = GcInfoDecoder(
244+
GCInfoToken(gcInfo),
245+
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
246+
codeOffset - 1
247+
);
248+
249+
assert(decoder.IsInterruptibleSafePoint());
250+
}
251+
}
252+
226253
ICodeManagerFlags flags = (ICodeManagerFlags)0;
227-
bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;
228254
if (executionAborted)
229255
flags = ICodeManagerFlags::ExecutionAborted;
230256

‎src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,8 +440,9 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
440440
PTR_uint8_t gcInfo;
441441
uint32_t codeOffset = GetCodeOffset(pMethodInfo, safePointAddress, &gcInfo);
442442

443-
ICodeManagerFlags flags = (ICodeManagerFlags)0;
444443
bool executionAborted = ((CoffNativeMethodInfo *)pMethodInfo)->executionAborted;
444+
445+
ICodeManagerFlags flags = (ICodeManagerFlags)0;
445446
if (executionAborted)
446447
flags = ICodeManagerFlags::ExecutionAborted;
447448

@@ -452,13 +453,36 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,
452453
flags = (ICodeManagerFlags)(flags | ICodeManagerFlags::ActiveStackFrame);
453454

454455
#ifdef USE_GC_INFO_DECODER
456+
if (!isActiveStackFrame && !executionAborted)
457+
{
458+
// the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
459+
codeOffset--;
460+
}
455461

456462
GcInfoDecoder decoder(
457463
GCInfoToken(gcInfo),
458464
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
459465
codeOffset
460466
);
461467

468+
if (isActiveStackFrame)
469+
{
470+
// CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here.
471+
// Or, better yet, maybe we should change the decoder to not require this adjustment.
472+
// The scenario that adjustment tries to handle (fallthrough into BB with random liveness)
473+
// does not seem possible.
474+
if (!decoder.HasInterruptibleRanges())
475+
{
476+
decoder = GcInfoDecoder(
477+
GCInfoToken(gcInfo),
478+
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
479+
codeOffset - 1
480+
);
481+
482+
assert(decoder.IsInterruptibleSafePoint());
483+
}
484+
}
485+
462486
if (!decoder.EnumerateLiveSlots(
463487
pRegisterSet,
464488
isActiveStackFrame /* reportScratchSlots */,

0 commit comments

Comments
 (0)