Skip to content

Commit

Permalink
SPMI: Fix a couple of replay/record bugs (#89443)
Browse files Browse the repository at this point in the history
* Avoid mutating constrained token in CEEInfo::getCallInfo

For certain calls the EE would modify the constrained token passed to
getCallInfo. This token is an in-argument, and SPMI does not expect it
to be modified. Fix the EE to make a local copy when it needs to mutate
it.

* SPMI: Fix treatment of CORINFO_FLG_BAD_INLINEE during replay

- Save union of method attributes set by the JIT from
  recSetMethodAttribs calls
- Fix bit handling in repGetMethodAttribs
- Add the same method attribs handling to repGetCallInfo that
  repGetMethodAttribs has

This fixes an issue where we could have recorded an old result of
getCallInfo without CORINFO_FLG_DONT_INLINE set. If the JIT then calls
repSetMethodAttribs(CORINFO_FLG_BAD_INLINEE) this would not be reflected
in subsequent calls to getCallInfo, causing possible misses.
  • Loading branch information
jakobbotsch authored Jul 26, 2023
1 parent d7a52b2 commit 8479eab
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 9 deletions.
8 changes: 4 additions & 4 deletions src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -3175,16 +3175,16 @@ class ICorDynamicInfo : public ICorStaticInfo

// Returns instructions on how to make the call. See code:CORINFO_CALL_INFO for possible return values.
virtual void getCallInfo(
// Token info
// Token info (in)
CORINFO_RESOLVED_TOKEN * pResolvedToken,

// Generics info
// Generics info (in)
CORINFO_RESOLVED_TOKEN * pConstrainedResolvedToken,

// Security info
// Security info (in)
CORINFO_METHOD_HANDLE callerHandle,

// Jit info
// Jit info (in)
CORINFO_CALLINFO_FLAGS flags,

// out params
Expand Down
20 changes: 17 additions & 3 deletions src/coreclr/tools/superpmi/superpmi-shared/compileresult.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -576,17 +576,31 @@ void CompileResult::recSetMethodAttribs(CORINFO_METHOD_HANDLE ftn, CorInfoMethod
if (SetMethodAttribs == nullptr)
SetMethodAttribs = new LightWeightMap<DWORDLONG, DWORD>();

SetMethodAttribs->Add(CastHandle(ftn), (DWORD)attribs);
int index = SetMethodAttribs->GetIndex(CastHandle(ftn));
if (index == -1)
{
SetMethodAttribs->Add(CastHandle(ftn), (DWORD)attribs);
}
else
{
DWORD existingAttribs = SetMethodAttribs->GetItem(index);
SetMethodAttribs->Update(index, existingAttribs | (DWORD)attribs);
}
}
void CompileResult::dmpSetMethodAttribs(DWORDLONG key, DWORD value)
{
printf("SetMethodAttribs key ftn-%016" PRIX64 ", value attr-%08X", key, value);
}
CorInfoMethodRuntimeFlags CompileResult::repSetMethodAttribs(CORINFO_METHOD_HANDLE ftn)
{
if ((SetMethodAttribs == nullptr) || (SetMethodAttribs->GetIndex(CastHandle(ftn)) == -1))
if (SetMethodAttribs == nullptr)
return (CorInfoMethodRuntimeFlags)0;

int index = SetMethodAttribs->GetIndex(CastHandle(ftn));
if (index == -1)
return (CorInfoMethodRuntimeFlags)0;
CorInfoMethodRuntimeFlags result = (CorInfoMethodRuntimeFlags)SetMethodAttribs->Get(CastHandle(ftn));

CorInfoMethodRuntimeFlags result = (CorInfoMethodRuntimeFlags)SetMethodAttribs->GetItem(index);
return result;
}

Expand Down
17 changes: 15 additions & 2 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -789,8 +789,8 @@ DWORD MethodContext::repGetMethodAttribs(CORINFO_METHOD_HANDLE methodHandle)

DEBUG_REP(dmpGetMethodAttribs(key, value));

if (cr->repSetMethodAttribs(methodHandle) == CORINFO_FLG_BAD_INLINEE)
value ^= CORINFO_FLG_DONT_INLINE;
if ((cr->repSetMethodAttribs(methodHandle) & CORINFO_FLG_BAD_INLINEE) != 0)
value |= CORINFO_FLG_DONT_INLINE;
return value;
}

Expand Down Expand Up @@ -1536,6 +1536,19 @@ void MethodContext::repGetCallInfo(CORINFO_RESOLVED_TOKEN* pResolvedToken,
{
pResult->hMethod = (CORINFO_METHOD_HANDLE)value.hMethod;
pResult->methodFlags = (unsigned)value.methodFlags;
// We could have stored getCallInfo from a previous call in this
// context without the CORINFO_FLG_DONT_INLINE bit. If the JIT later
// realized that this will be an always no-inline then it would call
// setMethodAttribs with that information, but a later getCallInfo call
// would not update the recorded SPMI entry. In that case let's mimic
// the behavior on the runtime side by calculating this particular
// dynamic flag here (the same logic is present in getMethodAttribs).
//
// This doesn't handle all cases (e.g. parallelism could still cause
// us to record these flags without CORINFO_FLG_DONT_INLINE).
if ((cr->repSetMethodAttribs(pResult->hMethod) & CORINFO_FLG_BAD_INLINEE) != 0)
pResult->methodFlags |= CORINFO_FLG_DONT_INLINE;

pResult->classFlags = (unsigned)value.classFlags;
pResult->sig = SpmiRecordsHelper::Restore_CORINFO_SIG_INFO(value.sig, GetCallInfo, SigInstHandleMap);
pResult->accessAllowed = (CorInfoIsAccessAllowedResult)value.accessAllowed;
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5071,6 +5071,10 @@ void CEEInfo::getCallInfo(
BOOL fForceUseRuntimeLookup = FALSE;
BOOL fAbstractSVM = FALSE;

// The below may need to mutate the constrained token, in which case we
// switch to this local copy to avoid mutating the in argument.
CORINFO_RESOLVED_TOKEN constrainedResolvedTokenCopy;

MethodDesc * pMDAfterConstraintResolution = pMD;
if (constrainedType.IsNull())
{
Expand Down Expand Up @@ -5115,6 +5119,9 @@ void CEEInfo::getCallInfo(
// Pretend this was a "constrained. UnderlyingType" instruction prefix
constrainedType = TypeHandle(CoreLibBinder::GetElementType(constrainedType.GetVerifierCorElementType()));

constrainedResolvedTokenCopy = *pConstrainedResolvedToken;
pConstrainedResolvedToken = &constrainedResolvedTokenCopy;

// Native image signature encoder will use this field. It needs to match that pretended type, a bogus signature
// would be produced otherwise.
pConstrainedResolvedToken->hClass = (CORINFO_CLASS_HANDLE)constrainedType.AsPtr();
Expand Down

0 comments on commit 8479eab

Please sign in to comment.