Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not encode safe points with -1 offset. #110845

Merged
merged 6 commits into from
Dec 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 12 additions & 40 deletions src/coreclr/gcinfo/gcinfoencoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -494,8 +494,6 @@ GcInfoEncoder::GcInfoEncoder(
// If the compiler doesn't set the GCInfo, report RT_Unset.
// This is used for compatibility with JITs that aren't updated to use the new API.
m_ReturnKind = RT_Unset;
#else
m_ReturnKind = RT_Illegal;
#endif // TARGET_X86
m_CodeLength = 0;
#ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA
Expand Down Expand Up @@ -776,12 +774,14 @@ void GcInfoEncoder::SetReversePInvokeFrameSlot(INT32 spOffset)
m_ReversePInvokeFrameSlot = spOffset;
}

#ifndef TARGET_X86
void GcInfoEncoder::SetReturnKind(ReturnKind returnKind)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used on Linux x86. Is there something fundamental that prevents Linux x86 to be switched to the same plan as the other GC encoder platforms? It would make the change simpler.

I know that there is no testing for Linux x86. I would not be worried about it. Since there is no testing, Linux x86 is likely broken anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I actually wanted to make this "ifdef", not "ifndef". How does this even pass tests?

x86 GC info is not supposed to be changed.

Copy link
Member Author

@VSadov VSadov Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. This is still the non-x86 GC encoder. The confusion was because this is a part where non-x86 encoder had an ifdef for TARGET_X86, so I assumed it is a shared path between two encoders.
It is not. The reason for original ifdef was just because X86 would have a different default state for the return kind.

Regardless x86 or not, m_ReturnKind is not going to be used by the non-legacy GC encoder. The non-legacy GC info is expressive enough to not require encoding return registers in an additional/special way.
This does not need to be ifdef'd. It should be removed. There is indeed nothing special about X86 here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my lack of knowledge about GC encoder history shows here.

{
_ASSERTE(IsValidReturnKind(returnKind));

m_ReturnKind = returnKind;
}
#endif

struct GcSlotDescAndId
{
Expand Down Expand Up @@ -1045,16 +1045,15 @@ void GcInfoEncoder::Build()
BOOL slimHeader = (!m_IsVarArg && !hasGSCookie && (m_PSPSymStackSlot == NO_PSP_SYM) &&
!hasContextParamType && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame &&
((m_StackBaseRegister == NO_STACK_BASE_REGISTER) || (NORMALIZE_STACK_BASE_REGISTER(m_StackBaseRegister) == 0))) &&
(m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) &&
#ifdef TARGET_AMD64
!m_WantsReportOnlyLeaf &&
#elif defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
!m_HasTailCalls &&
#endif // TARGET_AMD64
!IsStructReturnKind(m_ReturnKind);
(m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA);

// All new code is generated for the latest GCINFO_VERSION.
// So, always encode RetunrKind and encode ReversePInvokeFrameSlot where applicable.
// So, always encode ReversePInvokeFrameSlot where applicable.
if (slimHeader)
{
// Slim encoding means nothing special, partially interruptible, maybe a default frame register
Expand All @@ -1065,8 +1064,6 @@ void GcInfoEncoder::Build()
assert(m_StackBaseRegister == 8 || 2 == m_StackBaseRegister);
#endif
GCINFO_WRITE(m_Info1, (m_StackBaseRegister == NO_STACK_BASE_REGISTER) ? 0 : 1, 1, FlagsSize);

GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_SLIM_HEADER, RetKindSize);
}
else
{
Expand All @@ -1089,8 +1086,6 @@ void GcInfoEncoder::Build()
#endif // TARGET_AMD64
GCINFO_WRITE(m_Info1, ((m_SizeOfEditAndContinuePreservedArea != NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) ? 1 : 0), 1, FlagsSize);
GCINFO_WRITE(m_Info1, (hasReversePInvokeFrame ? 1 : 0), 1, FlagsSize);

GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_FAT_HEADER, RetKindSize);
}

_ASSERTE( m_CodeLength > 0 );
Expand Down Expand Up @@ -1217,42 +1212,19 @@ void GcInfoEncoder::Build()

///////////////////////////////////////////////////////////////////////
// Normalize call sites
// Eliminate call sites that fall inside interruptible ranges
///////////////////////////////////////////////////////////////////////

_ASSERTE(m_NumCallSites == 0 || numInterruptibleRanges == 0);

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

_ASSERTE(DENORMALIZE_CODE_OFFSET(NORMALIZE_CODE_OFFSET(callSite)) == callSite);
UINT32 normOffset = NORMALIZE_CODE_OFFSET(callSite);

BOOL keepIt = TRUE;

for(UINT32 intRangeIndex = 0; intRangeIndex < numInterruptibleRanges; intRangeIndex++)
{
InterruptibleRange *pRange = &pRanges[intRangeIndex];
if(pRange->NormStopOffset > normOffset)
{
if(pRange->NormStartOffset <= normOffset)
{
keepIt = FALSE;
}
break;
}
}

if(keepIt)
m_pCallSites[numCallSites++] = normOffset;
m_pCallSites[numCallSites++] = normOffset;
}

GCINFO_WRITE_VARL_U(m_Info1, NORMALIZE_NUM_SAFE_POINTS(numCallSites), NUM_SAFE_POINTS_ENCBASE, NumCallSitesSize);
Expand Down Expand Up @@ -1419,7 +1391,7 @@ void GcInfoEncoder::Build()

for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
{
if(pCurrent->CodeOffset > callSite)
if(pCurrent->CodeOffset >= callSite)
{
couldBeLive |= liveState;

Expand Down Expand Up @@ -1774,7 +1746,7 @@ void GcInfoEncoder::Build()
{
for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
{
if(pCurrent->CodeOffset > callSite)
if(pCurrent->CodeOffset >= callSite)
{
// Time to record the call site

Expand Down Expand Up @@ -1873,7 +1845,7 @@ void GcInfoEncoder::Build()

for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
{
if(pCurrent->CodeOffset > callSite)
if(pCurrent->CodeOffset >= callSite)
{
// Time to encode the call site

Expand Down Expand Up @@ -1920,7 +1892,7 @@ void GcInfoEncoder::Build()

for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
{
if(pCurrent->CodeOffset > callSite)
if(pCurrent->CodeOffset >= callSite)
{
// Time to encode the call site
GCINFO_WRITE_VECTOR(m_Info1, liveState, CallSiteStateSize);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/inc/gcinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const unsigned this_OFFSET_FLAG = 0x2; // the offset is "this"
// The current GCInfo Version
//-----------------------------------------------------------------------------

#define GCINFO_VERSION 3
#define GCINFO_VERSION 4

//-----------------------------------------------------------------------------
// GCInfoToken: A wrapper that contains the GcInfo data and version number.
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/inc/gcinfoencoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -409,12 +409,13 @@ class GcInfoEncoder
GcSlotState slotState
);


#ifndef TARGET_X86
//------------------------------------------------------------------------
// ReturnKind
//------------------------------------------------------------------------

void SetReturnKind(ReturnKind returnKind);
#endif

//------------------------------------------------------------------------
// Miscellaneous method information
Expand Down Expand Up @@ -509,7 +510,9 @@ class GcInfoEncoder
INT32 m_PSPSymStackSlot;
INT32 m_GenericsInstContextStackSlot;
GENERIC_CONTEXTPARAM_TYPE m_contextParamType;
#ifndef TARGET_X86
ReturnKind m_ReturnKind;
#endif
UINT32 m_CodeLength;
UINT32 m_StackBaseRegister;
UINT32 m_SizeOfEditAndContinuePreservedArea;
Expand Down
34 changes: 11 additions & 23 deletions src/coreclr/inc/gcinfotypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -637,8 +637,6 @@ void FASTCALL decodeCallPattern(int pattern,
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
#define GS_COOKIE_STACK_SLOT_ENCBASE 6
#define CODE_LENGTH_ENCBASE 8
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 4
#define STACK_BASE_REGISTER_ENCBASE 3
#define SIZE_OF_STACK_AREA_ENCBASE 3
#define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 4
Expand Down Expand Up @@ -679,8 +677,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,
#define DENORMALIZE_CODE_OFFSET(x) (x) // but the safe-point offsets are encoded with a -1 adjustment.
#define NORMALIZE_CODE_OFFSET(x) ((x)>>1) // Instructions are 2/4 bytes long in Thumb/ARM states,
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<1)
#define NORMALIZE_REGISTER(x) (x)
#define DENORMALIZE_REGISTER(x) (x)
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
Expand All @@ -695,8 +693,6 @@ void FASTCALL decodeCallPattern(int pattern,
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 5
#define GS_COOKIE_STACK_SLOT_ENCBASE 5
#define CODE_LENGTH_ENCBASE 7
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 2
#define STACK_BASE_REGISTER_ENCBASE 1
#define SIZE_OF_STACK_AREA_ENCBASE 3
#define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 3
Expand Down Expand Up @@ -735,9 +731,9 @@ void FASTCALL decodeCallPattern(int pattern,
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x)^29)
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
#define CODE_OFFSETS_NEED_NORMALIZATION 0
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
#define CODE_OFFSETS_NEED_NORMALIZATION 1
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
#define NORMALIZE_REGISTER(x) (x)
#define DENORMALIZE_REGISTER(x) (x)
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
Expand All @@ -750,8 +746,6 @@ void FASTCALL decodeCallPattern(int pattern,
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
#define GS_COOKIE_STACK_SLOT_ENCBASE 6
#define CODE_LENGTH_ENCBASE 8
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 4
#define STACK_BASE_REGISTER_ENCBASE 2 // FP encoded as 0, SP as 2.
#define SIZE_OF_STACK_AREA_ENCBASE 3
#define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 4
Expand Down Expand Up @@ -790,9 +784,9 @@ void FASTCALL decodeCallPattern(int pattern,
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 22 : 3)
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
#define CODE_OFFSETS_NEED_NORMALIZATION 0
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
#define CODE_OFFSETS_NEED_NORMALIZATION 1
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
#define NORMALIZE_REGISTER(x) (x)
#define DENORMALIZE_REGISTER(x) (x)
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
Expand All @@ -805,8 +799,6 @@ void FASTCALL decodeCallPattern(int pattern,
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
#define GS_COOKIE_STACK_SLOT_ENCBASE 6
#define CODE_LENGTH_ENCBASE 8
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 4
// FP/SP encoded as 0 or 1.
#define STACK_BASE_REGISTER_ENCBASE 2
#define SIZE_OF_STACK_AREA_ENCBASE 3
Expand Down Expand Up @@ -845,9 +837,9 @@ void FASTCALL decodeCallPattern(int pattern,
#define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 8 : 2)
#define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3)
#define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3)
#define CODE_OFFSETS_NEED_NORMALIZATION 0
#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point
#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment.
#define CODE_OFFSETS_NEED_NORMALIZATION 1
#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long
#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2)
#define NORMALIZE_REGISTER(x) (x)
#define DENORMALIZE_REGISTER(x) (x)
#define NORMALIZE_NUM_SAFE_POINTS(x) (x)
Expand All @@ -860,8 +852,6 @@ void FASTCALL decodeCallPattern(int pattern,
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
#define GS_COOKIE_STACK_SLOT_ENCBASE 6
#define CODE_LENGTH_ENCBASE 8
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 4
#define STACK_BASE_REGISTER_ENCBASE 2
// FP encoded as 0, SP as 1
#define SIZE_OF_STACK_AREA_ENCBASE 3
Expand Down Expand Up @@ -924,8 +914,6 @@ PORTABILITY_WARNING("Please specialize these definitions for your platform!")
#define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
#define GS_COOKIE_STACK_SLOT_ENCBASE 6
#define CODE_LENGTH_ENCBASE 6
#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER 2
#define STACK_BASE_REGISTER_ENCBASE 3
#define SIZE_OF_STACK_AREA_ENCBASE 6
#define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 3
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/inc/readytorun.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
// src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h
// If you update this, ensure you run `git grep MINIMUM_READYTORUN_MAJOR_VERSION`
// and handle pending work.
#define READYTORUN_MAJOR_VERSION 10
#define READYTORUN_MINOR_VERSION 0x0001
#define READYTORUN_MAJOR_VERSION 11
#define READYTORUN_MINOR_VERSION 0x0000

#define MINIMUM_READYTORUN_MAJOR_VERSION 10
#define MINIMUM_READYTORUN_MAJOR_VERSION 11

// R2R Version 2.1 adds the InliningInfo section
// R2R Version 2.2 adds the ProfileDataInfo section
Expand All @@ -38,6 +38,7 @@
// uses GCInfo v3, which makes safe points in partially interruptible code interruptible.
// R2R Version 10.0 adds support for the statics being allocated on a per type basis instead of on a per module basis, disable support for LogMethodEnter helper
// R2R Version 10.1 adds Unbox_TypeTest helper
// R2R Version 11 uses GCInfo v4, which encodes safe points without -1 offset and does not track return kinds in GCInfo

struct READYTORUN_CORE_HEADER
{
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/gcencode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3709,6 +3709,7 @@ class GcInfoEncoderWithLogging
}
}

#ifndef TARGET_X86
void SetReturnKind(ReturnKind returnKind)
{
m_gcInfoEncoder->SetReturnKind(returnKind);
Expand All @@ -3717,6 +3718,7 @@ class GcInfoEncoderWithLogging
printf("Set ReturnKind to %s.\n", ReturnKindToString(returnKind));
}
}
#endif

void SetStackBaseRegister(UINT32 registerNumber)
{
Expand Down Expand Up @@ -3832,7 +3834,9 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz

gcInfoEncoderWithLog->SetCodeLength(methodSize);

#ifndef TARGET_X86
gcInfoEncoderWithLog->SetReturnKind(getReturnKind());
#endif

if (compiler->isFramePointerUsed())
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ struct ReadyToRunHeaderConstants
{
static const uint32_t Signature = 0x00525452; // 'RTR'

static const uint32_t CurrentMajorVersion = 10;
static const uint32_t CurrentMinorVersion = 1;
static const uint32_t CurrentMajorVersion = 11;
static const uint32_t CurrentMinorVersion = 0;
};

struct ReadyToRunHeader
Expand Down
30 changes: 2 additions & 28 deletions src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,44 +213,18 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo,

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

bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;

if (!isActiveStackFrame && !executionAborted)
{
// the reasons for this adjustment are explained in EECodeManager::EnumGcRefs
codeOffset--;
}

GcInfoDecoder decoder(
GCInfoToken(gcInfo),
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
codeOffset
);

if (isActiveStackFrame)
{
// CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here.
// Or, better yet, maybe we should change the decoder to not require this adjustment.
// The scenario that adjustment tries to handle (fallthrough into BB with random liveness)
// does not seem possible.
if (!decoder.HasInterruptibleRanges())
{
decoder = GcInfoDecoder(
GCInfoToken(gcInfo),
GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG),
codeOffset - 1
);

assert(decoder.IsSafePoint());
}
}

ICodeManagerFlags flags = (ICodeManagerFlags)0;
bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;
if (executionAborted)
flags = ICodeManagerFlags::ExecutionAborted;

Expand Down
Loading
Loading