Put HasNativeCodeReJITAware into GetFunctionAddress #90049
Conversation
|
Fixes #89475. |
| @@ -1577,6 +1576,14 @@ DebuggerJitInfo *DebuggerMethodInfo::FindOrCreateInitAndAddJitInfo(MethodDesc* f | |||
| if (startAddr == NULL) | |||
| { | |||
| startAddr = g_pEEInterface->GetFunctionAddress(fd); | |||
| if (startAddr == NULL) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No more scenarios after calling GetFunctionAddress, I put an Assert in the code and the test still passes.
| { | ||
| CodeVersionManager *pCodeVersionManager = GetCodeVersionManager(); | ||
| CodeVersionManager::LockHolder codeVersioningLockHolder; | ||
| ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(PTR_MethodDesc(this)); | ||
| if (!ilVersion.IsDefaultVersion()) | ||
| ILCodeVersionCollection ilVersionCollection = pCodeVersionManager->GetILCodeVersions(PTR_MethodDesc(this)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ghost
locked as resolved and limited conversation to collaborators
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6317993923 |
Went through many branches of the call tree to understand which changes needed to be made to ensure safe behavior when updating GetFunctionAddress to return possibly arbitrary nativeCode.