Skip to content

Commit

Permalink
Remove MODULE_READY_FOR_TYPELOAD flag on Module (dotnet#103990)
Browse files Browse the repository at this point in the history
- `MODULE_READY_FOR_TYPELOAD` flag on `Module` was only checked in an assert under a condition that would never be hit. I think this is left from ngen.
- Remove Module filter parameter on DebuggerMethodInfo::IterateAllDJIs - always `NULL`
  • Loading branch information
elinor-fung committed Jun 26, 2024
1 parent 4a8fb53 commit e4caa8d
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 61 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/debug/ee/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1930,7 +1930,7 @@ BOOL DebuggerController::AddILPatch(AppDomain * pAppDomain, Module *module,
// Iterate through every existing NativeCodeBlob (with the same EnC version).
// This includes generics + prejitted code.
DebuggerMethodInfo::DJIIterator it;
dmi->IterateAllDJIs(pAppDomain, NULL /* module filter */, pMethodDescFilter, &it);
dmi->IterateAllDJIs(pAppDomain, pMethodDescFilter, &it);

if (it.IsAtEnd())
{
Expand Down
9 changes: 2 additions & 7 deletions src/coreclr/debug/ee/debugger.h
Original file line number Diff line number Diff line change
Expand Up @@ -1028,33 +1028,28 @@ class DebuggerMethodInfo
friend class DebuggerMethodInfo;

DebuggerJitInfo* m_pCurrent;
Module* m_pLoaderModuleFilter;
MethodDesc* m_pMethodDescFilter;
public:
DJIIterator();

bool IsAtEnd();
DebuggerJitInfo * Current();
void Next(BOOL fFirst = FALSE);

};

// Ensure the DJI cache is completely up to date. (This can be an expensive call, but
// much less so if pMethodDescFilter is used).
void CreateDJIsForNativeBlobs(AppDomain * pAppDomain, Module * pModuleFilter, MethodDesc * pMethodDescFilter);
void CreateDJIsForNativeBlobs(AppDomain * pAppDomain, MethodDesc * pMethodDescFilter);

// Ensure the DJI cache is up to date for a particular closed method desc
void CreateDJIsForMethodDesc(MethodDesc * pMethodDesc);

// Get an iterator for all native blobs (accounts for Generics, Enc, + Prejiiting).
// Must be stopped when we do this. This could be heavy weight.
// This will call CreateDJIsForNativeBlobs() to ensure we have all DJIs available.
// You may optionally pass pLoaderModuleFilter to restrict the DJIs iterated to
// exist only on MethodDescs whose loader module matches the filter (pass NULL not
// to filter by loader module).
// You may optionally pass pMethodDescFilter to restrict the DJIs iterated to only
// a single generic instantiation.
void IterateAllDJIs(AppDomain * pAppDomain, Module * pLoaderModuleFilter, MethodDesc * pMethodDescFilter, DJIIterator * pEnum);
void IterateAllDJIs(AppDomain * pAppDomain, MethodDesc * pMethodDescFilter, DJIIterator * pEnum);

private:
// The linked list of JIT's of this version of the method. This will ALWAYS
Expand Down
27 changes: 5 additions & 22 deletions src/coreclr/debug/ee/functioninfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1815,7 +1815,6 @@ DebuggerMethodInfo::DJIIterator::DJIIterator()
LIMITED_METHOD_CONTRACT;

m_pCurrent = NULL;
m_pLoaderModuleFilter = NULL;
}

bool DebuggerMethodInfo::DJIIterator::IsAtEnd()
Expand Down Expand Up @@ -1855,10 +1854,6 @@ void DebuggerMethodInfo::DJIIterator::Next(BOOL fFirst /*=FALSE*/)
{
Module * pLoaderModule = m_pCurrent->m_pLoaderModule;

// Obey the module filter if it's provided
if ((m_pLoaderModuleFilter != NULL) && (m_pLoaderModuleFilter != pLoaderModule))
continue;

//Obey the methodDesc filter if it is provided
if ((m_pMethodDescFilter != NULL) && (m_pMethodDescFilter != m_pCurrent->m_nativeCodeVersion.GetMethodDesc()))
continue;
Expand Down Expand Up @@ -1971,7 +1966,7 @@ void DebuggerMethodInfo::SetJMCStatus(bool fStatus)
// Get an iterator that will go through ALL native code-blobs (DJI) in the specified
// AppDomain, optionally filtered by loader module (if pLoaderModuleFilter != NULL).
// This is EnC/ Generics / Prejit aware.
void DebuggerMethodInfo::IterateAllDJIs(AppDomain * pAppDomain, Module * pLoaderModuleFilter, MethodDesc * pMethodDescFilter, DebuggerMethodInfo::DJIIterator * pEnum)
void DebuggerMethodInfo::IterateAllDJIs(AppDomain * pAppDomain, MethodDesc * pMethodDescFilter, DebuggerMethodInfo::DJIIterator * pEnum)
{
CONTRACTL
{
Expand All @@ -1984,10 +1979,9 @@ void DebuggerMethodInfo::IterateAllDJIs(AppDomain * pAppDomain, Module * pLoader
_ASSERTE(pAppDomain != NULL || pMethodDescFilter != NULL);

// Ensure we have DJIs for everything.
CreateDJIsForNativeBlobs(pAppDomain, pLoaderModuleFilter, pMethodDescFilter);
CreateDJIsForNativeBlobs(pAppDomain, pMethodDescFilter);

pEnum->m_pCurrent = m_latestJitInfo;
pEnum->m_pLoaderModuleFilter = pLoaderModuleFilter;
pEnum->m_pMethodDescFilter = pMethodDescFilter;

// Advance to the first DJI that passes the filter
Expand All @@ -2000,14 +1994,10 @@ void DebuggerMethodInfo::IterateAllDJIs(AppDomain * pAppDomain, Module * pLoader
//
// Arguments:
// * pAppDomain - Create DJIs only for this AppDomain
// * pLoaderModuleFilter - If non-NULL, create DJIs only for MethodDescs whose
// loader module matches this one. (This can be different from m_module in the
// case of generics defined in one module and instantiated in another). If
// non-NULL, create DJIs for all modules in pAppDomain.
// * pMethodDescFilter - If non-NULL, create DJIs only for this single MethodDesc.
//

void DebuggerMethodInfo::CreateDJIsForNativeBlobs(AppDomain * pAppDomain, Module * pLoaderModuleFilter, MethodDesc* pMethodDescFilter)
void DebuggerMethodInfo::CreateDJIsForNativeBlobs(AppDomain * pAppDomain, MethodDesc* pMethodDescFilter)
{
CONTRACTL
{
Expand All @@ -2016,11 +2006,8 @@ void DebuggerMethodInfo::CreateDJIsForNativeBlobs(AppDomain * pAppDomain, Module
}
CONTRACTL_END;

// If we're not stopped and the module we're iterating over allows types to load,
// then it's possible new native blobs are being created underneath us.
_ASSERTE(g_pDebugger->IsStopped() ||
((pLoaderModuleFilter != NULL) && !pLoaderModuleFilter->IsReadyForTypeLoad()) ||
pMethodDescFilter != NULL);
// If we're not stopped, it's possible new native blobs are being created underneath us.
_ASSERTE(g_pDebugger->IsStopped() || pMethodDescFilter != NULL);

if (pMethodDescFilter != NULL)
{
Expand All @@ -2047,10 +2034,6 @@ void DebuggerMethodInfo::CreateDJIsForNativeBlobs(AppDomain * pAppDomain, Module

Module * pLoaderModule = pDesc->GetLoaderModule();

// Obey the module filter if it's provided
if ((pLoaderModuleFilter != NULL) && (pLoaderModuleFilter != pLoaderModule))
continue;

// Skip modules that are unloaded, but still hanging around. Note that we can't use DebuggerModule for this check
// because of it is deleted pretty early during unloading, and we do not want to recreate it.
if (pLoaderModule->GetLoaderAllocator()->IsUnloaded())
Expand Down
37 changes: 10 additions & 27 deletions src/coreclr/vm/ceeload.h
Original file line number Diff line number Diff line change
Expand Up @@ -616,11 +616,9 @@ class Module : public ModuleBase
IS_ETW_NOTIFIED = 0x00000020,

//
// Note: the order of these must match the order defined in
// cordbpriv.h for DebuggerAssemblyControlFlags. The three
// values below should match the values defined in
// DebuggerAssemblyControlFlags when shifted right
// DEBUGGER_INFO_SHIFT bits.
// Note: The values below must match the ones defined in
// cordbpriv.h for DebuggerAssemblyControlFlags when shifted
// right DEBUGGER_INFO_SHIFT bits.
//
DEBUGGER_USER_OVERRIDE_PRIV = 0x00000400,
DEBUGGER_ALLOW_JIT_OPTS_PRIV= 0x00000800,
Expand All @@ -634,12 +632,15 @@ class Module : public ModuleBase
// Used to indicate that this module has had it's IJW fixups properly installed.
IS_IJW_FIXED_UP = 0x00080000,
IS_BEING_UNLOADED = 0x00100000,

// Used to indicate that the module is loaded sufficiently for generic candidate instantiations to work
MODULE_READY_FOR_TYPELOAD = 0x00200000,

};

static_assert_no_msg(DEBUGGER_USER_OVERRIDE_PRIV >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_USER_OVERRIDE);
static_assert_no_msg(DEBUGGER_ALLOW_JIT_OPTS_PRIV >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_ALLOW_JIT_OPTS);
static_assert_no_msg(DEBUGGER_TRACK_JIT_INFO_PRIV >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_OBSOLETE_TRACK_JIT_INFO);
static_assert_no_msg(DEBUGGER_ENC_ENABLED_PRIV >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_ENC_ENABLED);
static_assert_no_msg(DEBUGGER_PDBS_COPIED >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_PDBS_COPIED);
static_assert_no_msg(DEBUGGER_IGNORE_PDBS >> DEBUGGER_INFO_SHIFT_PRIV == DebuggerAssemblyControlFlags::DACF_IGNORE_PDBS);

enum {
// These are the values set in m_dwPersistedFlags.
// unused = 0x00000001,
Expand Down Expand Up @@ -930,24 +931,6 @@ class Module : public ModuleBase
}
#endif // !DACCESS_COMPILE


// This means the module has been sufficiently fixed up/security checked
// that type loads can occur in domains. This is not sufficient to indicate
// that domain-specific types can be loaded when applied to domain-neutral modules
BOOL IsReadyForTypeLoad()
{
LIMITED_METHOD_CONTRACT;
return m_dwTransientFlags & MODULE_READY_FOR_TYPELOAD;
}

#ifndef DACCESS_COMPILE
VOID SetIsReadyForTypeLoad()
{
LIMITED_METHOD_CONTRACT;
InterlockedOr((LONG*)&m_dwTransientFlags, MODULE_READY_FOR_TYPELOAD);
}
#endif

#ifndef DACCESS_COMPILE
VOID EnsureActive();
VOID EnsureAllocated();
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/vm/domainassembly.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -550,10 +550,6 @@ void DomainAssembly::FinishLoad()

// Now the DAC can find this module by enumerating assemblies in a domain.
DACNotify::DoModuleLoadNotification(m_pModule);

// Set a bit to indicate that the module has been loaded in some domain, and therefore
// typeloads can involve types from this module. (Used for candidate instantiations.)
GetModule()->SetIsReadyForTypeLoad();
}

void DomainAssembly::Activate()
Expand Down

0 comments on commit e4caa8d

Please sign in to comment.