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.

Sign up for GitHub

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

[cDAC] Set RejitFlags to use a known underlying type #109935

Merged
merged 2 commits into from
Nov 19, 2024
+70 −71
Merged

[cDAC] Set RejitFlags to use a known underlying type #109935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0a128
convert enum to enum class
Nov 18, 2024
f011147
pull enum out of ILCodeVersion
Nov 19, 2024
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
6 changes: 3 additions & 3 deletions src/coreclr/debug/daccess/dacdbiimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7284,11 +7284,11 @@ HRESULT DacDbiInterfaceImpl::GetActiveRejitILCodeVersionNode(VMPTR_Module vmModu
// Be careful, there are two different definitions of 'active' being used here
// For the CodeVersionManager, the active IL version is whatever one should be used in the next invocation of the method
// 'rejit active' narrows that to only include rejit IL bodies where the profiler has already provided the definition
// for the new IL (ilCodeVersion.GetRejitState()==ILCodeVersion::kStateActive). It is possible that the code version
// for the new IL (ilCodeVersion.GetRejitState()==ILCodeVersion::RejitFlags::kStateActive). It is possible that the code version
// manager's active IL version hasn't yet asked the profiler for the IL body to use, in which case we want to filter it
// out from the return in this method.
ILCodeVersion activeILVersion = pCodeVersionManager->GetActiveILCodeVersion(pModule, methodTk);
if (activeILVersion.IsNull() || activeILVersion.IsDefaultVersion() || activeILVersion.GetRejitState() != ILCodeVersion::kStateActive)
if (activeILVersion.IsNull() || activeILVersion.IsDefaultVersion() || activeILVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive)
{
pVmILCodeVersionNode->SetDacTargetPtr(0);
}
Expand Down Expand Up @@ -7390,7 +7390,7 @@ HRESULT DacDbiInterfaceImpl::GetILCodeVersionNodeData(VMPTR_ILCodeVersionNode vm
DD_ENTER_MAY_THROW;
#ifdef FEATURE_REJIT
ILCodeVersion ilCode(vmILCodeVersionNode.GetDacPtr());
pData->m_state = ilCode.GetRejitState();
pData->m_state = static_cast<DWORD>(ilCode.GetRejitState());
pData->m_pbIL = PTR_TO_CORDB_ADDRESS(dac_cast<TADDR>(ilCode.GetIL()));
pData->m_dwCodegenFlags = ilCode.GetJitFlags();
const InstrumentedILOffsetMapping* pMapping = ilCode.GetInstrumentedILMap();
Expand Down
14 changes: 7 additions & 7 deletions src/coreclr/debug/daccess/request.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,11 +921,11 @@ void CopyNativeCodeVersionToReJitData(NativeCodeVersion nativeCodeVersion, Nativ
pReJitData->flags = DacpReJitData::kUnknown;
break;

case ILCodeVersion::kStateRequested:
case ILCodeVersion::RejitFlags::kStateRequested:
pReJitData->flags = DacpReJitData::kRequested;
break;

case ILCodeVersion::kStateActive:
case ILCodeVersion::RejitFlags::kStateActive:
pReJitData->flags = DacpReJitData::kActive;
break;
}
Expand Down Expand Up @@ -4618,7 +4618,7 @@ HRESULT ClrDataAccess::GetPendingReJITID(CLRDATA_ADDRESS methodDesc, int *pRejit
{
hr = E_INVALIDARG;
}
else if (ilVersion.GetRejitState() == ILCodeVersion::kStateRequested)
else if (ilVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateRequested)
{
*pRejitId = (int)ilVersion.GetVersionId();
}
Expand Down Expand Up @@ -4661,11 +4661,11 @@ HRESULT ClrDataAccess::GetReJITInformation(CLRDATA_ADDRESS methodDesc, int rejit
pReJitData->flags = DacpReJitData2::kUnknown;
break;

case ILCodeVersion::kStateRequested:
case ILCodeVersion::RejitFlags::kStateRequested:
pReJitData->flags = DacpReJitData2::kRequested;
break;

case ILCodeVersion::kStateActive:
case ILCodeVersion::RejitFlags::kStateActive:
pReJitData->flags = DacpReJitData2::kActive;
break;
}
Expand Down Expand Up @@ -4698,7 +4698,7 @@ HRESULT ClrDataAccess::GetProfilerModifiedILInformation(CLRDATA_ADDRESS methodDe
CodeVersionManager* pCodeVersionManager = pMD->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.GetRejitState() != ILCodeVersion::kStateActive || !ilVersion.HasDefaultIL())
if (ilVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive || !ilVersion.HasDefaultIL())
{
pILData->type = DacpProfilerILData::ReJITModified;
pILData->rejitID = static_cast<ULONG>(pCodeVersionManager->GetActiveILCodeVersion(pMD).GetVersionId());
Expand Down Expand Up @@ -4748,7 +4748,7 @@ HRESULT ClrDataAccess::GetMethodsWithProfilerModifiedIL(CLRDATA_ADDRESS mod, CLR

TADDR pDynamicIL = pModule->GetDynamicIL(pMD->GetMemberDef());
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.GetRejitState() != ILCodeVersion::kStateActive || !ilVersion.HasDefaultIL() || pDynamicIL != (TADDR)NULL)
if (ilVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive || !ilVersion.HasDefaultIL() || pDynamicIL != (TADDR)NULL)
{
methodDescs[*pcMethodDescs] = PTR_CDADDR(pMD);
++(*pcMethodDescs);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12251,7 +12251,7 @@ HRESULT Debugger::DeoptimizeMethodHelper(Module* pModule, mdMethodDef methodDef)
// call back in to anything so set it all here to match the original IL and debug codegen flags
ilCodeVersion.SetIL(ILCodeVersion(pModule, methodDef).GetIL());
ilCodeVersion.SetJitFlags(COR_PRF_CODEGEN_DISABLE_ALL_OPTIMIZATIONS | COR_PRF_CODEGEN_DEBUG_INFO);
ilCodeVersion.SetRejitState(ILCodeVersion::kStateActive);
ilCodeVersion.SetRejitState(ILCodeVersion::RejitFlags::kStateActive);
ilCodeVersion.SetEnableReJITCallback(false);
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/functioninfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,7 +1058,7 @@ void DebuggerJitInfo::SetBoundaries(ULONG32 cMap, ICorDebugInfo::OffsetMapping *
{
// Did the current rejit provide a map?
const InstrumentedILOffsetMapping *pReJitMap = NULL;
if (ilVersion.GetRejitState() == ILCodeVersion::kStateActive)
if (ilVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateActive)
{
pReJitMap = ilVersion.GetInstrumentedILMap();
}
Expand Down
17 changes: 8 additions & 9 deletions src/coreclr/vm/codeversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ ILCodeVersionNode::ILCodeVersionNode() :
m_methodDef(0),
m_rejitId(0),
m_pNextILVersionNode(dac_cast<PTR_ILCodeVersionNode>(nullptr)),
m_rejitState(ILCodeVersion::kStateRequested),
m_rejitState(ILCodeVersion::RejitFlags::kStateRequested),
m_pIL(),
m_jitFlags(0),
m_deoptimized(FALSE)
Expand All @@ -563,7 +563,7 @@ ILCodeVersionNode::ILCodeVersionNode(Module* pModule, mdMethodDef methodDef, ReJ
m_methodDef(methodDef),
m_rejitId(id),
m_pNextILVersionNode(dac_cast<PTR_ILCodeVersionNode>(nullptr)),
m_rejitState(ILCodeVersion::kStateRequested),
m_rejitState(ILCodeVersion::RejitFlags::kStateRequested),
m_pIL(nullptr),
m_jitFlags(0),
m_deoptimized(isDeoptimized)
Expand Down Expand Up @@ -591,14 +591,14 @@ ReJITID ILCodeVersionNode::GetVersionId() const
ILCodeVersion::RejitFlags ILCodeVersionNode::GetRejitState() const
{
LIMITED_METHOD_DAC_CONTRACT;
return static_cast<ILCodeVersion::RejitFlags>(m_rejitState.Load() & ILCodeVersion::kStateMask);
return m_rejitState.Load() & ILCodeVersion::RejitFlags::kStateMask;
}

BOOL ILCodeVersionNode::GetEnableReJITCallback() const
{
LIMITED_METHOD_DAC_CONTRACT;

return (m_rejitState.Load() & ILCodeVersion::kSuppressParams) == ILCodeVersion::kSuppressParams;
return (m_rejitState.Load() & ILCodeVersion::RejitFlags::kSuppressParams) == ILCodeVersion::RejitFlags::kSuppressParams;
}

PTR_COR_ILMETHOD ILCodeVersionNode::GetIL() const
Expand Down Expand Up @@ -638,8 +638,7 @@ void ILCodeVersionNode::SetRejitState(ILCodeVersion::RejitFlags newState)
// We're doing a non thread safe modification to m_rejitState
_ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread());

ILCodeVersion::RejitFlags oldNonMaskFlags =
static_cast<ILCodeVersion::RejitFlags>(m_rejitState.Load() & ~ILCodeVersion::kStateMask);
ILCodeVersion::RejitFlags oldNonMaskFlags = m_rejitState.Load() & ~ILCodeVersion::RejitFlags::kStateMask;
m_rejitState.Store(static_cast<ILCodeVersion::RejitFlags>(newState | oldNonMaskFlags));
}

Expand All @@ -652,11 +651,11 @@ void ILCodeVersionNode::SetEnableReJITCallback(BOOL state)
ILCodeVersion::RejitFlags oldFlags = m_rejitState.Load();
if (state)
{
m_rejitState.Store(static_cast<ILCodeVersion::RejitFlags>(oldFlags | ILCodeVersion::kSuppressParams));
m_rejitState.Store(oldFlags | ILCodeVersion::RejitFlags::kSuppressParams);
}
else
{
m_rejitState.Store(static_cast<ILCodeVersion::RejitFlags>(oldFlags & ~ILCodeVersion::kSuppressParams));
m_rejitState.Store(oldFlags & ~ILCodeVersion::RejitFlags::kSuppressParams);
}
}

Expand Down Expand Up @@ -859,7 +858,7 @@ ILCodeVersion::RejitFlags ILCodeVersion::GetRejitState() const
}
else
{
return ILCodeVersion::kStateActive;
return ILCodeVersion::RejitFlags::kStateActive;
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/vm/codeversion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class ILCodeVersion
HRESULT SetActiveNativeCodeVersion(NativeCodeVersion activeNativeCodeVersion);
#endif //DACCESS_COMPILE

enum RejitFlags
enum class RejitFlags : uint32_t
Copy link
Member

Choose a reason for hiding this comment

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

Could we do some C++ to make us not have to write out ILCodeVersion::RejitFlags:: every time? I think we could put using enum RejitFlags in the ILCodeVersion class definition so the usage is the same. It feels too long to type out all of it every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is possible to do for enum class's in C++14. It looks like using enum is only available in C++20 and higher.

I see some approaches online manually dumping each enum value, but that feels worse than referencing them directly. https://stackoverflow.com/a/48932002

Copy link
Member

Choose a reason for hiding this comment

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

It is a bit unfortunate. Sometimes in the past I've simply elevated the enum to no longer be contained within the class when I do this conversion. Then I can just use RejitFlags::BLAH wherever it needs to be used. You still have to touch all of the places where the enum is touched, but the major reason to put an enum within ILCodeVersion was probably to give its members a namespace to live in, and enum class does that all by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled the enum out of ILCodeVersion. Now it is referenced with RejitState::<enum_value> instead of ILCodeVersion::<enum_value>.

{
// The profiler has requested a ReJit, so we've allocated stuff, but we haven't
// called back to the profiler to get any info or indicate that the ReJit has
Expand All @@ -208,7 +208,9 @@ class ILCodeVersion
// Indicates that the method being ReJITted is an inliner of the actual
// ReJIT request and we should not issue the GetReJITParameters for this
// method.
kSuppressParams = 0x80000000
kSuppressParams = 0x80000000,

support_use_as_flags // Enable the template functions in enum_class_flags.h
};

RejitFlags GetRejitState() const;
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7928,7 +7928,7 @@ CorInfoInline CEEInfo::canInline (CORINFO_METHOD_HANDLE hCaller,
CodeVersionManager* pCodeVersionManager = pCallee->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pCallee);
if (ilVersion.GetRejitState() != ILCodeVersion::kStateActive || !ilVersion.HasDefaultIL())
if (ilVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive || !ilVersion.HasDefaultIL())
{
result = INLINE_FAIL;
szFailReason = "ReJIT methods cannot be inlined.";
Expand Down Expand Up @@ -8131,7 +8131,7 @@ void CEEInfo::reportInliningDecision (CORINFO_METHOD_HANDLE inlinerHnd,
CodeVersionManager* pCodeVersionManager = pCallee->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pCallee);
if (ilVersion.GetRejitState() != ILCodeVersion::kStateActive || !ilVersion.HasDefaultIL())
if (ilVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive || !ilVersion.HasDefaultIL())
{
shouldCallReJIT = TRUE;
modId = reinterpret_cast<ModuleID>(pCaller->GetModule());
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/prestub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2113,11 +2113,11 @@ HRESULT VersionedPrepareCodeConfig::FinishConfiguration()
// Any code build stages that do just in time configuration should
// be configured now
#ifdef FEATURE_REJIT
if (m_ilCodeVersion.GetRejitState() != ILCodeVersion::kStateActive)
if (m_ilCodeVersion.GetRejitState() != ILCodeVersion::RejitFlags::kStateActive)
{
ReJitManager::ConfigureILCodeVersion(m_ilCodeVersion);
}
_ASSERTE(m_ilCodeVersion.GetRejitState() == ILCodeVersion::kStateActive);
_ASSERTE(m_ilCodeVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateActive);
#endif

return S_OK;
Expand Down
20 changes: 10 additions & 10 deletions src/coreclr/vm/rejit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ HRESULT ReJitManager::BindILVersion(
ILCodeVersion ilCodeVersion = pCodeVersionManager->GetActiveILCodeVersion(pModule, methodDef);
BOOL fDoCallback = (flags & COR_PRF_REJIT_INLINING_CALLBACKS) == COR_PRF_REJIT_INLINING_CALLBACKS;

if (ilCodeVersion.GetRejitState() == ILCodeVersion::kStateRequested)
if (ilCodeVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateRequested)
{
// We can 'reuse' this instance because the profiler doesn't know about
// it yet. (This likely happened because a profiler called RequestReJIT
Expand Down Expand Up @@ -965,12 +965,12 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion)
CodeVersionManager::LockHolder codeVersioningLockHolder;
switch (ilCodeVersion.GetRejitState())
{
case ILCodeVersion::kStateRequested:
ilCodeVersion.SetRejitState(ILCodeVersion::kStateGettingReJITParameters);
case ILCodeVersion::RejitFlags::kStateRequested:
ilCodeVersion.SetRejitState(ILCodeVersion::RejitFlags::kStateGettingReJITParameters);
fNeedsParameters = TRUE;
break;

case ILCodeVersion::kStateGettingReJITParameters:
case ILCodeVersion::RejitFlags::kStateGettingReJITParameters:
fWaitForParameters = TRUE;
break;

Expand Down Expand Up @@ -1026,9 +1026,9 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion)
// This code path also happens if the GetReJITParameters callback was suppressed due to
// the method being ReJITted as an inliner by the runtime (instead of by the user).
CodeVersionManager::LockHolder codeVersioningLockHolder;
if (ilCodeVersion.GetRejitState() == ILCodeVersion::kStateGettingReJITParameters)
if (ilCodeVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateGettingReJITParameters)
{
ilCodeVersion.SetRejitState(ILCodeVersion::kStateActive);
ilCodeVersion.SetRejitState(ILCodeVersion::RejitFlags::kStateActive);
ilCodeVersion.SetIL(ILCodeVersion(pModule, methodDef).GetIL());
}
}
Expand All @@ -1045,7 +1045,7 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion)
_ASSERTE(pFuncControl != NULL);

CodeVersionManager::LockHolder codeVersioningLockHolder;
if (ilCodeVersion.GetRejitState() == ILCodeVersion::kStateGettingReJITParameters)
if (ilCodeVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateGettingReJITParameters)
{
// Inside the above call to ICorProfilerCallback4::GetReJITParameters, the profiler
// will have used the specified pFuncControl to provide its IL and codegen flags.
Expand All @@ -1055,7 +1055,7 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion)
// ilCodeVersion is now the owner of the memory for the IL buffer
ilCodeVersion.SetInstrumentedILMap(pFuncControl->GetInstrumentedMapEntryCount(),
pFuncControl->GetInstrumentedMapEntries());
ilCodeVersion.SetRejitState(ILCodeVersion::kStateActive);
ilCodeVersion.SetRejitState(ILCodeVersion::RejitFlags::kStateActive);
}
}
}
Expand Down Expand Up @@ -1086,7 +1086,7 @@ HRESULT ReJitManager::ConfigureILCodeVersion(ILCodeVersion ilCodeVersion)
{
{
CodeVersionManager::LockHolder codeVersioningLockHolder;
if (ilCodeVersion.GetRejitState() == ILCodeVersion::kStateActive)
if (ilCodeVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateActive)
{
break; // the other thread got the parameters successfully, go race to rejit
}
Expand Down Expand Up @@ -1188,7 +1188,7 @@ HRESULT ReJitManager::GetReJITIDs(PTR_MethodDesc pMD, ULONG cReJitIds, ULONG * p
{
ILCodeVersion curILVersion = *iter;

if (curILVersion.GetRejitState() == ILCodeVersion::kStateActive)
if (curILVersion.GetRejitState() == ILCodeVersion::RejitFlags::kStateActive)
{
if (cnt < cReJitIds)
{
Expand Down