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

Put HasNativeCodeReJITAware into GetFunctionAddress #90049

Merged
merged 9 commits into from
Aug 14, 2023
Merged

Put HasNativeCodeReJITAware into GetFunctionAddress #90049

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0872883
Put HasNativeCodeReJITAware into GetFunctionAddress
mikelle-rogers Aug 4, 2023
3fe7c02
get rid of unused code
mikelle-rogers Aug 4, 2023
8a9d0b1
reverting GetFunctionAddress to previous behavior
mikelle-rogers Aug 9, 2023
49697d0
removing if statement
mikelle-rogers Aug 10, 2023
d69d33d
clarify method names
mikelle-rogers Aug 10, 2023
86e84bd
add comments
mikelle-rogers Aug 11, 2023
aaa2f7b
use BreakpointData.codeStartAddress
mikelle-rogers Aug 11, 2023
31529d5
dac cast
mikelle-rogers Aug 11, 2023
e4d8bfe
address code review
mikelle-rogers Aug 11, 2023
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
11 changes: 0 additions & 11 deletions src/coreclr/debug/daccess/dacimpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,17 +1253,6 @@ class ClrDataAccess
/* [out] */ union STUB_BUF* outBuffer,
/* [out] */ ULONG32* outFlags);

DebuggerJitInfo* GetDebuggerJitInfo(MethodDesc* methodDesc,
TADDR addr)
{
if (g_pDebugger)
{
return g_pDebugger->GetJitInfo(methodDesc, (PBYTE)addr, NULL);
}

return NULL;
}

HRESULT GetMethodExtents(MethodDesc* methodDesc,
METH_EXTENTS** extents);
HRESULT GetMethodVarInfo(MethodDesc* methodDesc,
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/debug/daccess/task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5243,7 +5243,7 @@ EnumMethodInstances::CdStart(MethodDesc* methodDesc,
CLRDATA_ENUM* handle)
{
if (!methodDesc->HasClassOrMethodInstantiation() &&
!methodDesc->HasNativeCodeReJITAware())
!(methodDesc->HasNativeCodeReJITAware()))
noahfalk marked this conversation as resolved.
Show resolved Hide resolved
{
*handle = 0;
return S_FALSE;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/debug/di/breakpoint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,13 @@ HRESULT CordbFunctionBreakpoint::Activate(BOOL fActivate)
if (codeIsIL)
{
pEvent->BreakpointData.nativeCodeMethodDescToken = pEvent->BreakpointData.nativeCodeMethodDescToken.NullPtr();
pEvent->BreakpointData.codeStartAddress = 0;
}
else
{
pEvent->BreakpointData.nativeCodeMethodDescToken =
(m_code.GetValue()->AsNativeCode())->GetVMNativeCodeMethodDescToken().ToLsPtr();
pEvent->BreakpointData.codeStartAddress = (m_code.GetValue()->AsNativeCode())->GetAddress();
mikelle-rogers marked this conversation as resolved.
Show resolved Hide resolved
}

// Note: we're sending a two-way event, so it blocks here
Expand Down
24 changes: 3 additions & 21 deletions src/coreclr/debug/ee/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1247,26 +1247,8 @@ bool DebuggerController::BindPatch(DebuggerControllerPatch *patch,
startAddr = (CORDB_ADDRESS_TYPE *) CORDB_ADDRESS_TO_PTR(patch->GetDJI()->m_addrOfCode);
_ASSERTE(startAddr != NULL);
}
if (startAddr == NULL)
{
// Should not be trying to place patches on MethodDecs's for stubs.
// These stubs will never get jitted.
CONSISTENCY_CHECK_MSGF(!pMD->IsWrapperStub(), ("Can't place patch at stub md %p, %s::%s",
pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName));

startAddr = (CORDB_ADDRESS_TYPE *)g_pEEInterface->GetFunctionAddress(pMD);
//
// Code is not available yet to patch. The prestub should
// notify us when it is executed.
//
if (startAddr == NULL)
{
LOG((LF_CORDB, LL_INFO10000,
"DC::BP: Patch at 0x%zx not bindable yet.\n", patch->offset));

return false;
}
}
//We should never be calling this function with both a NULL startAddr and a DJI that doesn't have code.
mikelle-rogers marked this conversation as resolved.
Show resolved Hide resolved
_ASSERTE(startAddr != NULL);
}

_ASSERTE(!g_pEEInterface->IsStub((const BYTE *)startAddr));
Expand Down Expand Up @@ -8655,7 +8637,7 @@ bool DebuggerFuncEvalComplete::SendEvent(Thread *thread, bool fIpChanged)
// DebuggerEnCBreakpoint constructor - creates and activates a new EnC breakpoint
//
// Arguments:
// offset - native offset in the function to place the patch
// offset - IL offset in the function to place the patch
// jitInfo - identifies the function in which the breakpoint is being placed
// fTriggerType - breakpoint type: either REMAP_PENDING or REMAP_COMPLETE
// pAppDomain - the breakpoint applies to the specified AppDomain only
Expand Down
50 changes: 7 additions & 43 deletions src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2832,6 +2832,8 @@ HRESULT Debugger::GetILToNativeMapping(PCODE pNativeCodeStartAddress, ULONG32 cM
}
CONTRACTL_END;

_ASSERTE(pNativeCodeStartAddress != NULL);

#ifdef PROFILING_SUPPORTED
// At this point, we're pulling in the debugger.
if (!HasLazyData())
Expand Down Expand Up @@ -2998,6 +3000,7 @@ HRESULT Debugger::GetILToNativeMappingIntoArrays(
_ASSERTE(pcMap != NULL);
_ASSERTE(prguiILOffset != NULL);
_ASSERTE(prguiNativeOffset != NULL);
_ASSERTE(pNativeCodeStartAddress != NULL);

// Any caller of GetILToNativeMappingIntoArrays had better call
// InitializeLazyDataIfNecessary first!
Expand Down Expand Up @@ -5402,28 +5405,6 @@ void Debugger::ReleaseAllRuntimeThreads(AppDomain *pAppDomain)
g_pEEInterface->ResumeFromDebug(pAppDomain);
}

// Given a method, get's its EnC version number. 1 if the method is not EnCed.
// Note that MethodDescs are reused between versions so this will give us
// the most recent EnC number.
int Debugger::GetMethodEncNumber(MethodDesc * pMethod)
{
CONTRACTL
{
THROWS;
GC_NOTRIGGER;
}
CONTRACTL_END;

DebuggerJitInfo * dji = GetLatestJitInfoFromMethodDesc(pMethod);
if (dji == NULL)
{
// If there's no DJI, couldn't have been EnCed.
return 1;
}
return (int) dji->m_encVersion;
}


bool Debugger::IsJMCMethod(Module* pModule, mdMethodDef tkMethod)
{
CONTRACTL
Expand Down Expand Up @@ -6210,25 +6191,6 @@ void Debugger::LockAndSendEnCRemapCompleteEvent(MethodDesc *pMD)
Thread *thread = g_pEEInterface->GetThread();
// Note that the debugger lock is reentrant, so we may or may not hold it already.
SENDIPCEVENT_BEGIN(this, thread);

EX_TRY
{
// Ensure the DJI for the latest version of this method has been pre-created.
// It's not clear whether this is necessary or not, but it shouldn't hurt since
// we're going to need to create it anyway since we'll be debugging inside it.
DebuggerJitInfo *dji = g_pDebugger->GetLatestJitInfoFromMethodDesc(pMD);
(void)dji; //prevent "unused variable" error from GCC
_ASSERTE( dji != NULL );
}
EX_CATCH
{
// GetLatestJitInfo could throw on OOM, but the debugger isn't resiliant to OOM.
// I'm not aware of any other legitimate reason why it may throw, so we'll ASSERT
// if it fails.
_ASSERTE(!"Unexpected exception from Debugger::GetLatestJitInfoFromMethodDesc on EnC remap complete");
}
EX_END_CATCH(RethrowTerminalExceptions);

// Send an EnC remap complete event to the Right Side.
DebuggerIPCEvent* ipce = m_pRCThread->GetIPCEventSendBuffer();
InitIPCEvent(ipce,
Expand Down Expand Up @@ -7856,6 +7818,7 @@ void Debugger::FirstChanceManagedExceptionCatcherFound(Thread *pThread,
// Implements DebugInterface
// Call by EE/exception. Must be on managed thread
_ASSERTE(GetThreadNULLOk() != NULL);
_ASSERTE(pMethodAddr != NULL);

// Quick check.
if (!CORDebuggerAttached())
Expand Down Expand Up @@ -12616,7 +12579,7 @@ DWORD Debugger::GetThreadIdHelper(Thread *pThread)
// does not own the memory provided via vars outparameter.
//-----------------------------------------------------------------------------
void Debugger::GetVarInfo(MethodDesc * fd, // [IN] method of interest
void *DebuggerVersionToken, // [IN] which edit version
CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version
SIZE_T * cVars, // [OUT] size of 'vars'
const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored
)
Expand All @@ -12628,7 +12591,7 @@ void Debugger::GetVarInfo(MethodDesc * fd, // [IN] method of interest
}
CONTRACTL_END;

DebuggerJitInfo * ji = (DebuggerJitInfo *)DebuggerVersionToken;
DebuggerJitInfo * ji = g_pDebugger->GetJitInfo(fd, (const BYTE *)nativeCodeAddress);

// If we didn't supply a DJI, then we're asking for the most recent version.
if (ji == NULL)
Expand Down Expand Up @@ -14019,6 +13982,7 @@ bool Debugger::GetILOffsetFromNative (MethodDesc *pFunc, const BYTE *pbAddr,
DebuggerJitInfo *jitInfo = methodInfo->FindOrCreateInitAndAddJitInfo(pFunc, methodStartAddress);
if (jitInfo == NULL)
{

mikelle-rogers marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/debug/ee/debugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1933,8 +1933,6 @@ class Debugger : public DebugInterface

bool IsJMCMethod(Module* pModule, mdMethodDef tkMethod);

int GetMethodEncNumber(MethodDesc * pMethod);


bool FirstChanceManagedException(Thread *pThread, SIZE_T currentIP, SIZE_T currentSP);

Expand Down Expand Up @@ -1980,7 +1978,7 @@ class Debugger : public DebugInterface
#endif // EnC_SUPPORTED

void GetVarInfo(MethodDesc * fd, // [IN] method of interest
void *DebuggerVersionToken, // [IN] which edit version
CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version
SIZE_T * cVars, // [OUT] size of 'vars'
const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored
);
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/debug/ee/functioninfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1565,7 +1565,6 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f
GC_NOTRIGGER;
}
CONTRACTL_END;

_ASSERTE(fd != NULL);

// The debugger doesn't track Lightweight-codegen methods b/c they have no metadata.
Expand All @@ -1576,7 +1575,6 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f

if (startAddr == NULL)
{
// This will grab the start address for the current code version.
startAddr = g_pEEInterface->GetFunctionAddress(fd);
if (startAddr == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

Are there scenarios that are still calling into FindOrCreateInitAndAddJitInfo() with startAddr=NULL after the other changes we did? (I'm not pushing back or implying anything, just asking)

If there aren't any scenarios left then we should assert startAddr != NULL and remove the code that handles the NULL case.

If there are still scenarios where startAddr==NULL can we enumerate them in a comment? What I'd like to get recorded is whether we've explicitly determined that searching default + active code version only is the correct behavior, or we know it isn't fully correct but we are using it for expediency, or we don't know one way or the other.

If GetNativeCodeReJITAware() is just a heuristic that often works but isn't fully correct then we should comment it clearly so that other devs don't accidentally start using it and dig the hole any deeper than it already is.

Copy link
Member Author

Choose a reason for hiding this comment

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

No more scenarios after calling GetFunctionAddress, I put an Assert in the code and the test still passes.

{
Expand Down Expand Up @@ -1648,7 +1646,7 @@ DebuggerJitInfo *DebuggerMethodInfo::CreateInitAndAddJitInfo(NativeCodeVersion n

_ASSERTE(fd != NULL);

// May or may-not be jitted, that's why we passed in the start addr & size explicitly.
// May or may-not be jitted, that's why we passed in the start addr & size explicitly.
mikelle-rogers marked this conversation as resolved.
Show resolved Hide resolved
_ASSERTE(startAddr != NULL);

*jitInfoWasCreated = FALSE;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/debug/inc/dbgipcevents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,6 +2011,7 @@ struct MSLAYOUT DebuggerIPCEvent
SIZE_T offset;
SIZE_T encVersion;
LSPTR_METHODDESC nativeCodeMethodDescToken; // points to the MethodDesc if !isIL
CORDB_ADDRESS codeStartAddress;
} BreakpointData;

struct MSLAYOUT
Expand Down
7 changes: 1 addition & 6 deletions src/coreclr/vm/dbginterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ class DebugInterface

// Get debugger variable information for a specific version of a method
virtual void GetVarInfo(MethodDesc * fd, // [IN] method of interest
void *DebuggerVersionToken, // [IN] which edit version
CORDB_ADDRESS nativeCodeAddress, // [IN] which edit version
SIZE_T * cVars, // [OUT] size of 'vars'
const ICorDebugInfo::NativeVarInfo **vars // [OUT] map telling where local vars are stored
) = 0;
Expand Down Expand Up @@ -262,11 +262,6 @@ class DebugInterface

virtual bool IsJMCMethod(Module* pModule, mdMethodDef tkMethod) = 0;

// Given a method, get's its EnC version number. 1 if the method is not EnCed.
// Note that MethodDescs are reused between versions so this will give us
// the most recent EnC number.
virtual int GetMethodEncNumber(MethodDesc * pMethod) = 0;

virtual void SendLogSwitchSetting (int iLevel,
int iReason,
_In_z_ LPCWSTR pLogSwitchName,
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/eedbginterfaceimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,6 @@ PCODE EEDbgInterfaceImpl::GetFunctionAddress(MethodDesc *pFD)
SUPPORTS_DAC;
}
CONTRACTL_END;

return pFD->GetNativeCode();
}

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/encee.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,8 +804,8 @@ NOINLINE void EditAndContinueModule::FixContextAndResume(
// Get the var info which the codemanager will use for updating
// enregistered variables correctly, or variables whose lifetimes differ
// at the update point
g_pDebugInterface->GetVarInfo(pMD, oldDebuggerFuncHandle, &oldVarInfoCount, &pOldVarInfo);
g_pDebugInterface->GetVarInfo(pMD, NULL, &newVarInfoCount, &pNewVarInfo);
g_pDebugInterface->GetVarInfo(pMD, oldCodeInfo.GetCodeAddress(), &oldVarInfoCount, &pOldVarInfo);
g_pDebugInterface->GetVarInfo(pMD, newCodeInfo.GetCodeAddress(), &newVarInfoCount, &pNewVarInfo);

#ifdef TARGET_X86
// save the frame pointer as FixContextForEnC might step on it.
Expand Down
17 changes: 10 additions & 7 deletions src/coreclr/vm/method.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,6 @@ PCODE MethodDesc::GetNativeCode()
WRAPPER_NO_CONTRACT;
SUPPORTS_DAC;
_ASSERTE(!IsDefaultInterfaceMethod() || HasNativeCodeSlot());

if (HasNativeCodeSlot())
{
// When profiler is enabled, profiler may ask to rejit a code even though we
Expand Down Expand Up @@ -946,19 +945,23 @@ PCODE MethodDesc::GetNativeCodeReJITAware()
return pDefaultCode;
}

else
{
CodeVersionManager *pCodeVersionManager = GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(PTR_MethodDesc(this));
if (!ilVersion.IsDefaultVersion())
ILCodeVersionCollection ilVersionCollection = pCodeVersionManager->GetILCodeVersions(PTR_MethodDesc(this));
Copy link
Member

Choose a reason for hiding this comment

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

Before you were getting the current IL version's address. Is there any guarantee of what order you get this in? Does it return in order of IL version and descend until a native body is found? For things like deoptimization, is that what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no guarantee of order, however, the work has been done to look through everything that calls this code, and make adjustments to allow this behavior to be safe. GetNativeCodeAnyVersion is only used in HasNativeCodeAnyVersion, which is used in EnumMethodInstances::Next and CdStart. It is only used to see whether or not there is a native code body. The reason it was changed is with the addition of deoptimization, there is a possibility that the user asks the code to be deoptimized before it has a default and active version (before the method is jitted). Thus, the code could have a native code version even though it did not have an active nor default native code version. @davmason @noahfalk

for (ILCodeVersionIterator curIL = ilVersionCollection.Begin(), endIL = ilVersionCollection.End(); curIL != endIL; curIL++)
{
NativeCodeVersion activeNativeCodeVersion = ilVersion.GetActiveNativeCodeVersion(PTR_MethodDesc(this));
if (!activeNativeCodeVersion.IsNull())
NativeCodeVersionCollection nativeCollection = curIL->GetNativeCodeVersions(PTR_MethodDesc(this));
for (NativeCodeVersionIterator curNative = nativeCollection.Begin(), endNative = nativeCollection.End(); curNative != endNative; curNative++)
{
return activeNativeCodeVersion.GetNativeCode();
PCODE native = curNative->GetNativeCode();
if(native != NULL)
{
return native;
}
}
}

return NULL;
}
}
Expand Down