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 1 commit
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
4 changes: 2 additions & 2 deletions src/coreclr/debug/daccess/task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5225,7 +5225,7 @@ EnumMethodInstances::Next(ClrDataAccess* dac,
}
}

if (!(g_pEEInterface->GetFunctionAddress(m_methodIter.Current()) != NULL))
if (!m_methodIter.Current()->HasNativeCodeReJITAware())
noahfalk marked this conversation as resolved.
Show resolved Hide resolved
{
goto NextMethod;
}
Expand All @@ -5243,7 +5243,7 @@ EnumMethodInstances::CdStart(MethodDesc* methodDesc,
CLRDATA_ENUM* handle)
{
if (!methodDesc->HasClassOrMethodInstantiation() &&
!(g_pEEInterface->GetFunctionAddress(methodDesc) != NULL))
!(methodDesc->HasNativeCodeReJITAware()))
noahfalk marked this conversation as resolved.
Show resolved Hide resolved
{
*handle = 0;
return S_FALSE;
Expand Down
1 change: 1 addition & 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,6 +211,7 @@ HRESULT CordbFunctionBreakpoint::Activate(BOOL fActivate)
if (codeIsIL)
{
pEvent->BreakpointData.nativeCodeMethodDescToken = pEvent->BreakpointData.nativeCodeMethodDescToken.NullPtr();
pEvent->BreakpointData.codeStartAddress = 0;
}
else
{
Expand Down
25 changes: 3 additions & 22 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,27 +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)
{
_ASSERTE(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 @@ -8656,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
1 change: 1 addition & 0 deletions src/coreclr/debug/ee/debugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13982,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
11 changes: 9 additions & 2 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 @@ -1577,6 +1576,14 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f
if (startAddr == NULL)
{
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.

{
startAddr = fd->GetNativeCodeReJITAware();
if (startAddr == NULL)
{
return NULL;
}
}
}
else
{
Expand Down Expand Up @@ -1639,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
28 changes: 1 addition & 27 deletions src/coreclr/vm/eedbginterfaceimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,33 +630,7 @@ PCODE EEDbgInterfaceImpl::GetFunctionAddress(MethodDesc *pFD)
SUPPORTS_DAC;
}
CONTRACTL_END;

//return pFD->GetNativeCode();
WRAPPER_NO_CONTRACT;
SUPPORTS_DAC;

PCODE pDefaultCode = pFD->GetNativeCode();
if (pDefaultCode != NULL)
{
return pDefaultCode;
}

{
Module *pModule = pFD->GetModule();
CodeVersionManager *pCodeVersionManager = pModule->GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(PTR_MethodDesc(pFD));
if (!ilVersion.IsDefaultVersion())
{
NativeCodeVersion activeNativeCodeVersion = ilVersion.GetActiveNativeCodeVersion(PTR_MethodDesc(pFD));
if (!activeNativeCodeVersion.IsNull())
{
return activeNativeCodeVersion.GetNativeCode();
}
}

return NULL;
}
return pFD->GetNativeCode();
}

#ifndef DACCESS_COMPILE
Expand Down
33 changes: 32 additions & 1 deletion 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 All @@ -935,6 +934,38 @@ PCODE MethodDesc::GetNativeCode()
return GetStableEntryPoint();
}

PCODE MethodDesc::GetNativeCodeReJITAware()
{
WRAPPER_NO_CONTRACT;
SUPPORTS_DAC;

PCODE pDefaultCode = GetNativeCode();
if (pDefaultCode != NULL)
{
return pDefaultCode;
}

else
{
CodeVersionManager *pCodeVersionManager = GetCodeVersionManager();
CodeVersionManager::LockHolder codeVersioningLockHolder;
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++)
{
NativeCodeVersionCollection nativeCollection = curIL->GetNativeCodeVersions(PTR_MethodDesc(this));
for (NativeCodeVersionIterator curNative = nativeCollection.Begin(), endNative = nativeCollection.End(); curNative != endNative; curNative++)
{
PCODE native = curNative->GetNativeCode();
if(native != NULL)
{
return native;
}
}
}
return NULL;
}
}

//*******************************************************************************
PTR_PCODE MethodDesc::GetAddrOfNativeCodeSlot()
{
Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/vm/method.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,14 @@ class MethodDesc
return GetNativeCode() != NULL;
}

// Perf warning: takes the CodeVersionManagerLock on every call
BOOL HasNativeCodeReJITAware()
{
LIMITED_METHOD_DAC_CONTRACT;

return GetNativeCodeReJITAware() != NULL;
}

BOOL SetNativeCodeInterlocked(PCODE addr, PCODE pExpected = NULL);

PTR_PCODE GetAddrOfNativeCodeSlot();
Expand Down Expand Up @@ -1428,6 +1436,11 @@ class MethodDesc
// Returns the address of the native code.
PCODE GetNativeCode();

// Returns GetNativeCode() if it exists, but also checks to see if there
// is a non-default IL code version and returns that.
mikelle-rogers marked this conversation as resolved.
Show resolved Hide resolved
// Perf warning: takes the CodeVersionManagerLock on every call
PCODE GetNativeCodeReJITAware();

#if defined(FEATURE_JIT_PITCHING)
bool IsPitchable();
void PitchNativeCode();
Expand Down