diff --git a/src/coreclr/gc/env/gcenv.object.h b/src/coreclr/gc/env/gcenv.object.h index 1a7077c75722a0..683ee3996c3943 100644 --- a/src/coreclr/gc/env/gcenv.object.h +++ b/src/coreclr/gc/env/gcenv.object.h @@ -21,6 +21,9 @@ extern bool g_oldMethodTableFlags; // Bits stolen from the sync block index that the GC/HandleTable knows about (currently these are at the same // positions as the mainline runtime but we can change this below when it becomes apparent how NativeAOT will // handle sync blocks). +#ifdef FEATURE_JAVAMARSHAL +#define BIT_SBLK_BRIDGE_PENDING 0x80000000 +#endif // FEATURE_JAVAMARSHAL #define BIT_SBLK_GC_RESERVE 0x20000000 #define BIT_SBLK_FINALIZER_RUN 0x40000000 diff --git a/src/coreclr/gc/gcbridge.cpp b/src/coreclr/gc/gcbridge.cpp index 31cc595922aa0c..c7f4805bdcbf39 100644 --- a/src/coreclr/gc/gcbridge.cpp +++ b/src/coreclr/gc/gcbridge.cpp @@ -261,6 +261,7 @@ static DynPtrArray g_scanStack, g_loopStack; // Objects from crossref handles registered with RegisterBridgeObject static DynPtrArray g_registeredBridges; static DynPtrArray g_registeredBridgesContexts; +static DynPtrArray g_registeredBridgeHandles; // As we traverse the graph, which ColorData objects are accessible from our current position? static DynPtrArray g_colorMergeArray; @@ -1086,10 +1087,12 @@ static void BridgeFinish() #endif } -void BridgeResetData() +void BridgeResetData(bool resetPendingBridgeHandles) { DynPtrArrayEmpty(&g_registeredBridges); DynPtrArrayEmpty(&g_registeredBridgesContexts); + if (resetPendingBridgeHandles) + DynPtrArrayEmpty(&g_registeredBridgeHandles); DynPtrArrayEmpty(&g_scanStack); DynPtrArrayEmpty(&g_loopStack); EmptyObjectBuckets(); @@ -1105,6 +1108,17 @@ void RegisterBridgeObject(Object* object, uintptr_t context) DynPtrArrayAdd(&g_registeredBridgesContexts, (void*)context); } +void RegisterPendingBridgeHandle(uintptr_t handle) +{ + DynPtrArrayAdd(&g_registeredBridgeHandles, (void*)handle); +} + +uintptr_t* GetPendingBridgeHandles(size_t* count) +{ + *count = (size_t)DynPtrArraySize(&g_registeredBridgeHandles); + return (uintptr_t*)g_registeredBridgeHandles.data; +} + uint8_t** GetRegisteredBridges(size_t* pNumBridges) { *pNumBridges = (size_t)g_registeredBridges.size; @@ -1298,6 +1312,12 @@ MarkCrossReferencesArgs* ProcessBridgeObjects() ResetObjectsHeader(); + for (int i = 0; i < DynPtrArraySize(&g_registeredBridges); i++) + { + Object* object = (Object*)DynPtrArrayGet(&g_registeredBridges, i); + object->GetHeader()->SetBit(BIT_SBLK_BRIDGE_PENDING); + } + BridgeFinish(); return args; diff --git a/src/coreclr/gc/gcbridge.h b/src/coreclr/gc/gcbridge.h index a61388f64392f9..32400b98397e19 100644 --- a/src/coreclr/gc/gcbridge.h +++ b/src/coreclr/gc/gcbridge.h @@ -9,7 +9,7 @@ #include "common.h" #include "gcinterface.h" -void BridgeResetData(); +void BridgeResetData(bool resetPendingBridgeHandles); MarkCrossReferencesArgs* ProcessBridgeObjects(); // Decides whether this collection should hand a fresh set of cross references to the client. @@ -18,6 +18,8 @@ MarkCrossReferencesArgs* ProcessBridgeObjects(); bool ShouldProcessBridgeObjects(); void RegisterBridgeObject(Object *object, uintptr_t context); +void RegisterPendingBridgeHandle(uintptr_t handle); +uintptr_t* GetPendingBridgeHandles(size_t* count); uint8_t** GetRegisteredBridges(size_t *pNumBridges); #endif // FEATURE_JAVAMARSHAL diff --git a/src/coreclr/gc/gcimpl.h b/src/coreclr/gc/gcimpl.h index 0ab8741e96fdb3..4fb14a446a2e64 100644 --- a/src/coreclr/gc/gcimpl.h +++ b/src/coreclr/gc/gcimpl.h @@ -334,6 +334,8 @@ class GCHeap : public IGCHeapInternal virtual int RefreshMemoryLimit(); virtual void NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHandles); + + virtual uintptr_t* GetPendingBridgeHandles(size_t* count); }; #endif // GCIMPL_H_ diff --git a/src/coreclr/gc/gcinterface.h b/src/coreclr/gc/gcinterface.h index fc27e0cc499136..27d0e1315dd6e8 100644 --- a/src/coreclr/gc/gcinterface.h +++ b/src/coreclr/gc/gcinterface.h @@ -1074,6 +1074,8 @@ class IGCHeap { virtual void DiagWalkHeapWithACHandling(walk_fn fn, void* context, int gen_number, bool walk_large_object_heap_p) PURE_VIRTUAL virtual void NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHandles) PURE_VIRTUAL; + + virtual uintptr_t* GetPendingBridgeHandles(size_t* count) PURE_VIRTUAL; }; #ifdef WRITE_BARRIER_CHECK diff --git a/src/coreclr/gc/interface.cpp b/src/coreclr/gc/interface.cpp index 871c84ee737a60..82ce661e176892 100644 --- a/src/coreclr/gc/interface.cpp +++ b/src/coreclr/gc/interface.cpp @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. #include "gcinternal.h" +#include "gcbridge.h" #ifdef SERVER_GC namespace SVR @@ -2737,6 +2738,17 @@ void GCHeap::NullBridgeObjectsWeakRefs(size_t length, void* unreachableObjectHan #endif } +uintptr_t* GCHeap::GetPendingBridgeHandles(size_t* count) +{ +#ifdef FEATURE_JAVAMARSHAL + return ::GetPendingBridgeHandles(count); +#else + assert(false); + *count = 0; + return nullptr; +#endif +} + HRESULT GCHeap::WaitUntilConcurrentGCCompleteAsync(int millisecondsTimeout) { #ifdef BACKGROUND_GC diff --git a/src/coreclr/gc/objecthandle.cpp b/src/coreclr/gc/objecthandle.cpp index 10ebc01485ba3d..f948cd1f00fe0e 100644 --- a/src/coreclr/gc/objecthandle.cpp +++ b/src/coreclr/gc/objecthandle.cpp @@ -1504,6 +1504,8 @@ void CALLBACK GetBridgeObjectsForProcessing(_UNCHECKED_OBJECTREF* pObjRef, uintp if (!g_theGCHeap->IsPromoted(*ppRef)) { RegisterBridgeObject(*ppRef, *pExtraInfo); + if (lp2 != 0) + RegisterPendingBridgeHandle((uintptr_t)pObjRef); } } @@ -1514,8 +1516,9 @@ uint8_t** Ref_ScanBridgeObjects(uint32_t condemned, uint32_t maxgen, ScanContext LOG((LF_GC | LF_CORPROF, LL_INFO10000, "Building bridge object graphs.\n")); uint32_t flags = HNDGCF_NORMAL; uint32_t type = HNDTYPE_CROSSREFERENCE; + bool shouldProcessBridgeObjects = ShouldProcessBridgeObjects(); - BridgeResetData(); + BridgeResetData(shouldProcessBridgeObjects); HandleTableMap* walk = &g_HandleTableMap; while (walk) { @@ -1527,14 +1530,14 @@ uint8_t** Ref_ScanBridgeObjects(uint32_t condemned, uint32_t maxgen, ScanContext HHANDLETABLE hTable = walk->pBuckets[i]->pTable[uCPUindex]; if (hTable) // or have a local var for bridgeObjectsToPromote/size (instead of NULL) that's passed in as lp2 - HndScanHandlesForGC(hTable, GetBridgeObjectsForProcessing, uintptr_t(sc), 0, &type, 1, condemned, maxgen, HNDGCF_EXTRAINFO | flags); + HndScanHandlesForGC(hTable, GetBridgeObjectsForProcessing, uintptr_t(sc), shouldProcessBridgeObjects, &type, 1, condemned, maxgen, HNDGCF_EXTRAINFO | flags); } } walk = walk->pNext; } // The callee here will free the allocated memory. - if (ShouldProcessBridgeObjects()) + if (shouldProcessBridgeObjects) { MarkCrossReferencesArgs *args = ProcessBridgeObjects(); diff --git a/src/coreclr/nativeaot/Runtime/ObjectLayout.h b/src/coreclr/nativeaot/Runtime/ObjectLayout.h index 4c477272a72e77..581af36b2b7006 100644 --- a/src/coreclr/nativeaot/Runtime/ObjectLayout.h +++ b/src/coreclr/nativeaot/Runtime/ObjectLayout.h @@ -5,8 +5,13 @@ // Low-level types describing GC object layouts. // +#include "volatile.h" + // Bits stolen from the sync block index that the GC/HandleTable knows about (currently these are at the same // positions as the mainline runtime). +#ifdef FEATURE_JAVAMARSHAL +#define BIT_SBLK_BRIDGE_PENDING 0x80000000 +#endif // FEATURE_JAVAMARSHAL #define BIT_SBLK_GC_RESERVE 0x20000000 #define BIT_SBLK_FINALIZER_RUN 0x40000000 @@ -22,6 +27,7 @@ class ObjHeader public: uint32_t GetBits() { return m_uSyncBlockValue; } + uint32_t GetBitsAcquire() { return VolatileLoad(&m_uSyncBlockValue); } void SetBit(uint32_t uBit); void ClrBit(uint32_t uBit); void SetGCBit() { m_uSyncBlockValue |= BIT_SBLK_GC_RESERVE; } diff --git a/src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp b/src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp index 54aee68af28527..f394fa67c185d9 100644 --- a/src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp +++ b/src/coreclr/nativeaot/Runtime/interoplibinterface_java.cpp @@ -26,6 +26,19 @@ namespace Volatile g_GCBridgeActive = false; CLREventStatic g_bridgeFinished; + void ClearPendingBridgeBits( + uintptr_t* handles, + size_t handleCount) + { + for (size_t i = 0; i < handleCount; i++) + { + OBJECTHANDLE handle = reinterpret_cast(handles[i]); + Object* object = ObjectFromHandle(handle); + if (object != nullptr) + object->GetHeader()->ClrBit(BIT_SBLK_BRIDGE_PENDING); + } + } + void ReleaseGCBridgeArgumentsWorker( MarkCrossReferencesArgs* args) { @@ -55,19 +68,17 @@ void JavaMarshalNative::TriggerClientBridgeProcessing( { _ASSERTE(GCHeapUtilities::IsGCInProgress()); - if (g_GCBridgeActive) - { - // Release the memory allocated since the GCBridge - // is already running and we're not passing them to it. - ReleaseGCBridgeArgumentsWorker(args); - return; - } + size_t pendingBridgeHandleCount; + uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount); + + _ASSERTE(!g_GCBridgeActive); // Not initialized if (g_MarkCrossReferences == NULL) { // Release the memory allocated since we // don't have a GC bridge callback. + ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount); ReleaseGCBridgeArgumentsWorker(args); return; } @@ -122,12 +133,14 @@ extern "C" void QCALLTYPE JavaMarshal_FinishCrossReferenceProcessing( pThisThread->DisablePreemptiveMode(); GCHeapUtilities::GetGCHeap()->NullBridgeObjectsWeakRefs(length, unreachableObjectHandles); + size_t pendingBridgeHandleCount; + uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount); + ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount); IGCHandleManager* pHandleManager = GCHandleUtilities::GetGCHandleManager(); OBJECTHANDLE* handles = (OBJECTHANDLE*)unreachableObjectHandles; for (size_t i = 0; i < length; i++) pHandleManager->DestroyHandleOfUnknownType(handles[i]); - g_GCBridgeActive = false; g_bridgeFinished.Set(); @@ -139,12 +152,21 @@ extern "C" void QCALLTYPE JavaMarshal_FinishCrossReferenceProcessing( FCIMPL2(FC_BOOL_RET, GCHandle_InternalTryGetBridgeWait, OBJECTHANDLE handle, OBJECTREF* pObjResult) { - if (g_GCBridgeActive) + Object* object = ObjectFromHandle(handle); + if (g_GCBridgeActive && object != nullptr && + (object->GetHeader()->GetBitsAcquire() & BIT_SBLK_BRIDGE_PENDING) != 0) + { + FC_RETURN_BOOL(false); + } + + // See explanation in Interop::TryGetObjectFromHandleWithoutBridgeWait + Object* confirmedObject = ObjectFromHandle(handle); + if (confirmedObject != object) { FC_RETURN_BOOL(false); } - *pObjResult = ObjectFromHandle(handle); + *pObjResult = confirmedObject; FC_RETURN_BOOL(true); } FCIMPLEND diff --git a/src/coreclr/vm/interoplibinterface.h b/src/coreclr/vm/interoplibinterface.h index c9da0f7eaeeef3..23838ed77009f3 100644 --- a/src/coreclr/vm/interoplibinterface.h +++ b/src/coreclr/vm/interoplibinterface.h @@ -150,6 +150,10 @@ class Interop static bool IsGCBridgeActive(); + static bool TryGetObjectFromHandleWithoutBridgeWait( + _In_ OBJECTHANDLE handle, + _Out_ Object** result); + static void WaitForGCBridgeFinish(); static void TriggerClientBridgeProcessing( diff --git a/src/coreclr/vm/interoplibinterface_java.cpp b/src/coreclr/vm/interoplibinterface_java.cpp index f3048be9551ca5..aa5e57e03acc02 100644 --- a/src/coreclr/vm/interoplibinterface_java.cpp +++ b/src/coreclr/vm/interoplibinterface_java.cpp @@ -20,6 +20,23 @@ namespace Volatile g_GCBridgeActive = false; CLREvent* g_bridgeFinished = nullptr; + void ClearPendingBridgeBits( + _In_reads_(handleCount) uintptr_t* handles, + size_t handleCount) + { + LIMITED_METHOD_CONTRACT; + + for (size_t i = 0; i < handleCount; i++) + { + OBJECTHANDLE handle = reinterpret_cast(handles[i]); + Object* object = OBJECTREFToObject(ObjectFromHandle(handle)); + if (object != nullptr) + { + object->GetHeader()->ClrBit(BIT_SBLK_BRIDGE_PENDING); + } + } + } + void ReleaseGCBridgeArgumentsWorker( _In_ MarkCrossReferencesArgs* args) { @@ -51,6 +68,42 @@ bool Interop::IsGCBridgeActive() return g_GCBridgeActive; } +bool Interop::TryGetObjectFromHandleWithoutBridgeWait( + _In_ OBJECTHANDLE handle, + _Out_ Object** result) +{ + LIMITED_METHOD_CONTRACT; + + // g_GCBridgeActive is Volatile and pending bit clear uses InterlockedAnd. These should be set with release + // stores guaranteeing the order: weak-reference nulling -> pending-bit clearing -> g_GCBridgeActive = false + Object* object = OBJECTREFToObject(ObjectFromHandle(handle)); + if (g_GCBridgeActive && object != nullptr && + (object->GetHeader()->GetBitsAcquire() & BIT_SBLK_BRIDGE_PENDING) != 0) + { + return false; + } + + // If object is nullptr, the handle value is stable, refetching is harmless + // If bridge is not active, refetching the handle should guarantee we get the right value + // since `g_GCBridgeActive` is Volatile + // The only remaining interesting case is gc bridge being active, with non-null object which + // had the bridge pending bit not set. + // + // If the bridge pending bit was never set because the object was not due for collection, handle value is stable + // If the bridge pending bit was set but got cleared in the meantime due to this code racing with the bridge + // finisher, refetching the handle will guarantee that we see the new value of the handle, because the + // bit is cleared with InterlockedAnd which does a release barrier (guaranteeing that the potential handle nulling + // was already published). + Object* confirmedObject = OBJECTREFToObject(ObjectFromHandle(handle)); + if (confirmedObject != object) + { + return false; + } + + *result = confirmedObject; + return true; +} + void Interop::WaitForGCBridgeFinish() { CONTRACTL @@ -80,13 +133,10 @@ void Interop::TriggerClientBridgeProcessing( } CONTRACTL_END; - if (g_GCBridgeActive) - { - // Release the memory allocated since the GCBridge - // is already running and we're not passing them to it. - ReleaseGCBridgeArgumentsWorker(args); - return; - } + size_t pendingBridgeHandleCount; + uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount); + + _ASSERTE(!g_GCBridgeActive); bool gcBridgeTriggered = JavaNative::TriggerClientBridgeProcessing(args); @@ -94,6 +144,7 @@ void Interop::TriggerClientBridgeProcessing( { // Release the memory allocated since the GCBridge // wasn't trigger for some reason. + ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount); ReleaseGCBridgeArgumentsWorker(args); return; } @@ -122,11 +173,13 @@ void Interop::FinishCrossReferenceProcessing( GCX_COOP(); GCHeapUtilities::GetGCHeap()->NullBridgeObjectsWeakRefs(length, unreachableObjectHandles); + size_t pendingBridgeHandleCount; + uintptr_t* pendingBridgeHandles = GCHeapUtilities::GetGCHeap()->GetPendingBridgeHandles(&pendingBridgeHandleCount); + ClearPendingBridgeBits(pendingBridgeHandles, pendingBridgeHandleCount); IGCHandleManager* pHandleManager = GCHandleUtilities::GetGCHandleManager(); for (size_t i = 0; i < length; i++) pHandleManager->DestroyHandleOfUnknownType(((OBJECTHANDLE*)unreachableObjectHandles)[i]); - g_GCBridgeActive = false; g_bridgeFinished->Set(); } diff --git a/src/coreclr/vm/marshalnative.cpp b/src/coreclr/vm/marshalnative.cpp index 44fbaaac59f847..d1530f14e9fc18 100644 --- a/src/coreclr/vm/marshalnative.cpp +++ b/src/coreclr/vm/marshalnative.cpp @@ -397,12 +397,9 @@ FCIMPL2(FC_BOOL_RET, MarshalNative::GCHandleInternalTryGetBridgeWait, OBJECTHAND { FCALL_CONTRACT; - if (Interop::IsGCBridgeActive()) - { + if (!Interop::TryGetObjectFromHandleWithoutBridgeWait(handle, pObjResult)) FC_RETURN_BOOL(false); - } - *pObjResult = OBJECTREFToObject(ObjectFromHandle(handle)); FC_RETURN_BOOL(true); } FCIMPLEND diff --git a/src/coreclr/vm/syncblk.h b/src/coreclr/vm/syncblk.h index 81dc4f1dec5528..71a0ffb125bb8c 100644 --- a/src/coreclr/vm/syncblk.h +++ b/src/coreclr/vm/syncblk.h @@ -82,11 +82,14 @@ typedef DPTR(EnCSyncBlockInfo) PTR_EnCSyncBlockInfo; // to zero out the ObjHeader for the current allocation. And the limits of the // GC space are initialized to respect this "off by one" error. -// m_SyncBlockValue is carved up into an index and a set of bits. Steal bits by -// reducing the mask. We use the very high bit, in _DEBUG, to be sure we never forget -// to mask the Value to obtain the Index +// m_SyncBlockValue is carved up into an index and a set of bits. Steal bits by +// reducing the mask. +#ifdef FEATURE_JAVAMARSHAL +#define BIT_SBLK_BRIDGE_PENDING 0x80000000 +#else #define BIT_SBLK_UNUSED 0x80000000 +#endif // FEATURE_JAVAMARSHAL #define BIT_SBLK_FINALIZER_RUN 0x40000000 #define BIT_SBLK_GC_RESERVE 0x20000000 @@ -861,6 +864,14 @@ class ObjHeader return m_SyncBlockValue.LoadWithoutBarrier(); } + DWORD GetBitsAcquire() + { + LIMITED_METHOD_CONTRACT; + SUPPORTS_DAC; + + return m_SyncBlockValue.Load(); + } + DWORD SetBits(DWORD newBits, DWORD oldBits) {