From 00830cda5b38f1001bbe2334b10fb2302148d537 Mon Sep 17 00:00:00 2001 From: Sean Gillespie Date: Tue, 21 Mar 2017 22:00:12 -0700 Subject: [PATCH] [Local GC] Forbid inclusion of gcscan.h from VM and DAC directories (dotnet/coreclr#10332) * Forbid inclusion of gcscan.h from VM and DAC directories * Address code review feedback - hoist IsGCHeapInitialized check to an assert, since the heap should definitely be initialized if we are validating objects that ostensibly came from it Commit migrated from https://github.com/dotnet/coreclr/commit/d5906ff3e0d0f864c1432f1f39d586ca3d79fbb6 --- src/coreclr/src/debug/daccess/enummem.cpp | 5 ++--- src/coreclr/src/debug/daccess/request.cpp | 1 + src/coreclr/src/debug/daccess/request_svr.cpp | 1 + src/coreclr/src/debug/ee/dactable.cpp | 8 -------- src/coreclr/src/gc/gcee.cpp | 5 +++++ src/coreclr/src/gc/gcimpl.h | 2 ++ src/coreclr/src/gc/gcinterface.h | 3 +++ src/coreclr/src/gc/gcscan.cpp | 17 +---------------- src/coreclr/src/gc/gcscan.h | 4 ---- src/coreclr/src/inc/dacvars.h | 2 -- src/coreclr/src/vm/object.cpp | 6 +++--- src/coreclr/src/vm/stubhelpers.cpp | 3 +-- src/coreclr/src/vm/syncblk.cpp | 3 +-- 13 files changed, 20 insertions(+), 40 deletions(-) diff --git a/src/coreclr/src/debug/daccess/enummem.cpp b/src/coreclr/src/debug/daccess/enummem.cpp index 4a217d0..0dc7a42 100644 --- a/src/coreclr/src/debug/daccess/enummem.cpp +++ b/src/coreclr/src/debug/daccess/enummem.cpp @@ -21,7 +21,6 @@ #include "ipcmanagerinterface.h" #include "binder.h" #include "win32threadpool.h" -#include "gcscan.h" #ifdef FEATURE_APPX #include "appxutil.h" @@ -407,7 +406,7 @@ HRESULT ClrDataAccess::DumpManagedObject(CLRDataEnumMemoryFlags flags, OBJECTREF return status; } - if (!GCScan::GetGcRuntimeStructuresValid ()) + if (*g_gcDacGlobals->gc_structures_invalid_cnt != 0) { // GC is in progress, don't dump this object return S_OK; @@ -465,7 +464,7 @@ HRESULT ClrDataAccess::DumpManagedExcepObject(CLRDataEnumMemoryFlags flags, OBJE return S_OK; } - if (!GCScan::GetGcRuntimeStructuresValid ()) + if (*g_gcDacGlobals->gc_structures_invalid_cnt != 0) { // GC is in progress, don't dump this object return S_OK; diff --git a/src/coreclr/src/debug/daccess/request.cpp b/src/coreclr/src/debug/daccess/request.cpp index 88efe22..c30501f 100644 --- a/src/coreclr/src/debug/daccess/request.cpp +++ b/src/coreclr/src/debug/daccess/request.cpp @@ -3827,6 +3827,7 @@ ClrDataAccess::EnumWksGlobalMemoryRegions(CLRDataEnumMemoryFlags flags) Dereference(g_gcDacGlobals->ephemeral_heap_segment).EnumMem(); g_gcDacGlobals->alloc_allocated.EnumMem(); + g_gcDacGlobals->gc_structures_invalid_cnt.EnumMem(); Dereference(g_gcDacGlobals->finalize_queue).EnumMem(); // Enumerate the entire generation table, which has variable size diff --git a/src/coreclr/src/debug/daccess/request_svr.cpp b/src/coreclr/src/debug/daccess/request_svr.cpp index 4a90ec1..930a35b 100644 --- a/src/coreclr/src/debug/daccess/request_svr.cpp +++ b/src/coreclr/src/debug/daccess/request_svr.cpp @@ -210,6 +210,7 @@ ClrDataAccess::EnumSvrGlobalMemoryRegions(CLRDataEnumMemoryFlags flags) DacEnumMemoryRegion(g_gcDacGlobals->g_heaps.GetAddr(), sizeof(TADDR) * *g_gcDacGlobals->n_heaps); + g_gcDacGlobals->gc_structures_invalid_cnt.EnumMem(); g_gcDacGlobals->g_heaps.EnumMem(); for (int i=0; i < *g_gcDacGlobals->n_heaps; i++) diff --git a/src/coreclr/src/debug/ee/dactable.cpp b/src/coreclr/src/debug/ee/dactable.cpp index a54fa75..af7de17 100644 --- a/src/coreclr/src/debug/ee/dactable.cpp +++ b/src/coreclr/src/debug/ee/dactable.cpp @@ -21,14 +21,6 @@ #include "../../vm/gcenv.h" #include "../../vm/ecall.h" #include "../../vm/rcwwalker.h" -#include "../../gc/gc.h" -#include "../../gc/gcscan.h" - -#undef SERVER_GC -namespace WKS { -#include "../../gc/gcimpl.h" -#include "../../gc/gcpriv.h" -} #ifdef DEBUGGING_SUPPORTED diff --git a/src/coreclr/src/gc/gcee.cpp b/src/coreclr/src/gc/gcee.cpp index c93cc91..6513fde 100644 --- a/src/coreclr/src/gc/gcee.cpp +++ b/src/coreclr/src/gc/gcee.cpp @@ -681,6 +681,11 @@ void GCHeap::UnregisterFrozenSegment(segment_handle seg) #endif // FEATURE_BASICFREEZE } +bool GCHeap::RuntimeStructuresValid() +{ + return GCScan::GetGcRuntimeStructuresValid(); +} + #endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/gc/gcimpl.h b/src/coreclr/src/gc/gcimpl.h index 67d84b9..7b4b2eb 100644 --- a/src/coreclr/src/gc/gcimpl.h +++ b/src/coreclr/src/gc/gcimpl.h @@ -91,6 +91,8 @@ public: void SetGCInProgress(BOOL fInProgress); + bool RuntimeStructuresValid(); + CLREvent * GetWaitForGCEvent(); HRESULT Initialize (); diff --git a/src/coreclr/src/gc/gcinterface.h b/src/coreclr/src/gc/gcinterface.h index 42cdcc0..c463ba7 100644 --- a/src/coreclr/src/gc/gcinterface.h +++ b/src/coreclr/src/gc/gcinterface.h @@ -581,6 +581,9 @@ public: // Sets whether or not a GC is in progress. virtual void SetGCInProgress(BOOL fInProgress) = 0; + // Gets whether or not the GC runtime structures are in a valid state for heap traversal. + virtual bool RuntimeStructuresValid() = 0; + /* ============================================================================ Add/RemoveMemoryPressure support routines. These are on the interface diff --git a/src/coreclr/src/gc/gcscan.cpp b/src/coreclr/src/gc/gcscan.cpp index b4e6352..edcb533 100644 --- a/src/coreclr/src/gc/gcscan.cpp +++ b/src/coreclr/src/gc/gcscan.cpp @@ -19,11 +19,7 @@ #include "gc.h" #include "objecthandle.h" -#ifdef DACCESS_COMPILE -SVAL_IMPL_INIT(int32_t, GCScan, m_GcStructuresInvalidCnt, 1); -#else //DACCESS_COMPILE VOLATILE(int32_t) GCScan::m_GcStructuresInvalidCnt = 1; -#endif //DACCESS_COMPILE bool GCScan::GetGcRuntimeStructuresValid () { @@ -33,18 +29,7 @@ bool GCScan::GetGcRuntimeStructuresValid () return (int32_t)m_GcStructuresInvalidCnt == 0; } -#ifdef DACCESS_COMPILE - -#ifndef FEATURE_REDHAWK -void -GCScan::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) -{ - UNREFERENCED_PARAMETER(flags); - m_GcStructuresInvalidCnt.EnumMem(); -} -#endif - -#else +#ifndef DACCESS_COMPILE // // Dependent handle promotion scan support diff --git a/src/coreclr/src/gc/gcscan.h b/src/coreclr/src/gc/gcscan.h index c17f0ce..306d5f2 100644 --- a/src/coreclr/src/gc/gcscan.h +++ b/src/coreclr/src/gc/gcscan.h @@ -89,11 +89,7 @@ class GCScan static void VerifyHandleTable(int condemned, int max_gen, ScanContext* sc); -#ifdef DACCESS_COMPILE - SVAL_DECL(int32_t, m_GcStructuresInvalidCnt); -#else static VOLATILE(int32_t) m_GcStructuresInvalidCnt; -#endif //DACCESS_COMPILE }; // These two functions are utilized to scan the heap if requested by ETW diff --git a/src/coreclr/src/inc/dacvars.h b/src/coreclr/src/inc/dacvars.h index 6180210..ab3bca2 100644 --- a/src/coreclr/src/inc/dacvars.h +++ b/src/coreclr/src/inc/dacvars.h @@ -138,8 +138,6 @@ DEFINE_DACVAR(ULONG, PTR_SharedDomain, SharedDomain__m_pSharedDomain, SharedDoma DEFINE_DACVAR(ULONG, DWORD, CExecutionEngine__TlsIndex, CExecutionEngine::TlsIndex) -DEFINE_DACVAR(ULONG, LONG, GCScan__m_GcStructuresInvalidCnt, GCScan::m_GcStructuresInvalidCnt) - #if defined(FEATURE_WINDOWSPHONE) DEFINE_DACVAR(ULONG, int, CCLRErrorReportingManager__g_ECustomDumpFlavor, CCLRErrorReportingManager::g_ECustomDumpFlavor) #endif diff --git a/src/coreclr/src/vm/object.cpp b/src/coreclr/src/vm/object.cpp index dc53757..c37aacc 100644 --- a/src/coreclr/src/vm/object.cpp +++ b/src/coreclr/src/vm/object.cpp @@ -19,7 +19,6 @@ #include "eeconfig.h" #include "gcheaputilities.h" #include "field.h" -#include "gcscan.h" #include "argdestination.h" @@ -1773,12 +1772,13 @@ VOID Object::ValidateInner(BOOL bDeep, BOOL bVerifyNextHeader, BOOL bVerifySyncB lastTest = 7; + _ASSERTE(GCHeapUtilities::IsGCHeapInitialized()); // try to validate next object's header if (bDeep && bVerifyNextHeader - && GCScan::GetGcRuntimeStructuresValid () + && GCHeapUtilities::GetGCHeap()->RuntimeStructuresValid() //NextObj could be very slow if concurrent GC is going on - && !(GCHeapUtilities::IsGCHeapInitialized() && GCHeapUtilities::GetGCHeap ()->IsConcurrentGCInProgress ())) + && !GCHeapUtilities::GetGCHeap ()->IsConcurrentGCInProgress ()) { Object * nextObj = GCHeapUtilities::GetGCHeap ()->NextObj (this); if ((nextObj != NULL) && diff --git a/src/coreclr/src/vm/stubhelpers.cpp b/src/coreclr/src/vm/stubhelpers.cpp index 899e88e..bf83c72 100644 --- a/src/coreclr/src/vm/stubhelpers.cpp +++ b/src/coreclr/src/vm/stubhelpers.cpp @@ -21,7 +21,6 @@ #include "comdatetime.h" #include "gcheaputilities.h" #include "interoputil.h" -#include "gcscan.h" #ifdef FEATURE_COMINTEROP #include @@ -58,7 +57,7 @@ void StubHelpers::ValidateObjectInternal(Object *pObjUNSAFE, BOOL fValidateNextO } CONTRACTL_END; - _ASSERTE(GCScan::GetGcRuntimeStructuresValid()); + _ASSERTE(GCHeapUtilities::GetGCHeap()->RuntimeStructuresValid()); // validate the object - there's no need to validate next object's // header since we validate the next object explicitly below diff --git a/src/coreclr/src/vm/syncblk.cpp b/src/coreclr/src/vm/syncblk.cpp index b8a818c..78e455a 100644 --- a/src/coreclr/src/vm/syncblk.cpp +++ b/src/coreclr/src/vm/syncblk.cpp @@ -30,7 +30,6 @@ #include "corhost.h" #include "comdelegate.h" #include "finalizerthread.h" -#include "gcscan.h" #ifdef FEATURE_COMINTEROP #include "runtimecallablewrapper.h" @@ -2518,7 +2517,7 @@ BOOL ObjHeader::Validate (BOOL bVerifySyncBlkIndex) //rest of the DWORD is SyncBlk Index if (!(bits & BIT_SBLK_IS_HASHCODE)) { - if (bVerifySyncBlkIndex && GCScan::GetGcRuntimeStructuresValid ()) + if (bVerifySyncBlkIndex && GCHeapUtilities::GetGCHeap()->RuntimeStructuresValid ()) { DWORD sbIndex = bits & MASK_SYNCBLOCKINDEX; ASSERT_AND_CHECK(SyncTableEntry::GetSyncTableEntry()[sbIndex].m_Object == obj); -- 2.7.4