diff --git a/src/coreclr/debug/daccess/request.cpp b/src/coreclr/debug/daccess/request.cpp index f075dbe29c4e2e..6ffcc763a4fa39 100644 --- a/src/coreclr/debug/daccess/request.cpp +++ b/src/coreclr/debug/daccess/request.cpp @@ -3842,11 +3842,11 @@ HRESULT ClrDataAccess::GetClrWatsonBucketsWorker(Thread * pThread, GenericModeBl if (oThrowable != NULL) { // Does the throwable have buckets? - if (((EXCEPTIONREF)oThrowable)->AreWatsonBucketsPresent()) + U1ARRAYREF refWatsonBucketArray = ((EXCEPTIONREF)oThrowable)->GetWatsonBucketReference(); + if (refWatsonBucketArray != NULL) { // Get the watson buckets from the throwable for non-preallocated // exceptions - U1ARRAYREF refWatsonBucketArray = ((EXCEPTIONREF)oThrowable)->GetWatsonBucketReference(); pBuckets = dac_cast(refWatsonBucketArray->GetDataPtr()); } else diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 74a1f9f6be7822..5249ea70661e0d 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -9096,6 +9096,7 @@ void SetupWatsonBucketsForUEF(BOOL fUseLastThrownObject) struct { OBJECTREF oThrowable; + U1ARRAYREF oBuckets; } gc; ZeroMemory(&gc, sizeof(gc)); GCPROTECT_BEGIN(gc); @@ -9197,9 +9198,10 @@ void SetupWatsonBucketsForUEF(BOOL fUseLastThrownObject) SetupWatsonBucketsForNonPreallocatedExceptions(gc.oThrowable); } - if (((EXCEPTIONREF)gc.oThrowable)->AreWatsonBucketsPresent()) + gc.oBuckets = ((EXCEPTIONREF)gc.oThrowable)->GetWatsonBucketReference(); + if (gc.oBuckets != NULL) { - pUEWatsonBucketTracker->CopyBucketsFromThrowable(gc.oThrowable); + pUEWatsonBucketTracker->CopyBuckets(gc.oBuckets); } if (pUEWatsonBucketTracker->RetrieveWatsonBuckets() == NULL) @@ -9519,6 +9521,7 @@ BOOL SetupWatsonBucketsForFailFast(EXCEPTIONREF refException) { OBJECTREF refException; OBJECTREF oInnerMostExceptionThrowable; + U1ARRAYREF oBuckets; } gc; ZeroMemory(&gc, sizeof(gc)); GCPROTECT_BEGIN(gc); @@ -9669,10 +9672,11 @@ BOOL SetupWatsonBucketsForFailFast(EXCEPTIONREF refException) } // If it has the buckets, copy them over to the current Watson bucket tracker - if (((EXCEPTIONREF)gc.oInnerMostExceptionThrowable)->AreWatsonBucketsPresent()) + gc.oBuckets = ((EXCEPTIONREF)gc.oInnerMostExceptionThrowable)->GetWatsonBucketReference(); + if (gc.oBuckets != NULL) { pUEWatsonBucketTracker->ClearWatsonBucketDetails(); - pUEWatsonBucketTracker->CopyBucketsFromThrowable(gc.oInnerMostExceptionThrowable); + pUEWatsonBucketTracker->CopyBuckets(gc.oBuckets); if (pUEWatsonBucketTracker->RetrieveWatsonBuckets() != NULL) { LOG((LF_EH, LL_INFO1000, "SetupWatsonBucketsForFailFast - Got watson buckets from regular innermost exception.\n")); @@ -9711,11 +9715,12 @@ BOOL SetupWatsonBucketsForFailFast(EXCEPTIONREF refException) SetupWatsonBucketsForNonPreallocatedExceptions(gc.refException); } - if (((EXCEPTIONREF)gc.refException)->AreWatsonBucketsPresent()) + gc.oBuckets = ((EXCEPTIONREF)gc.refException)->GetWatsonBucketReference(); + if (gc.oBuckets != NULL) { // Copy the buckets to the current watson bucket tracker pUEWatsonBucketTracker->ClearWatsonBucketDetails(); - pUEWatsonBucketTracker->CopyBucketsFromThrowable(gc.refException); + pUEWatsonBucketTracker->CopyBuckets(gc.oBuckets); if (pUEWatsonBucketTracker->RetrieveWatsonBuckets() != NULL) { LOG((LF_EH, LL_INFO1000, "SetupWatsonBucketsForFailFast - Watson buckets copied from the exception object.\n")); @@ -9950,6 +9955,9 @@ void SetupInitialThrowBucketDetails(UINT_PTR adjustedIp) EX_TRY { CopyWatsonBucketsToThrowable(pUEWatsonBucketTracker->RetrieveWatsonBuckets()); + + // Technically this assert can fail, as another thread could clear the buckets after + // CopyWatsonBucketsToThrowable but before the assert runs, but it is very unlikely. _ASSERTE(((EXCEPTIONREF)gc.oCurrentThrowable)->AreWatsonBucketsPresent()); } EX_CATCH @@ -10686,7 +10694,7 @@ void EHWatsonBucketTracker::Init() // This method copies the bucketing details from the specified throwable // to the current Watson Bucket tracker. -void EHWatsonBucketTracker::CopyBucketsFromThrowable(OBJECTREF oThrowable) +void EHWatsonBucketTracker::CopyBuckets(U1ARRAYREF oBuckets) { #ifndef DACCESS_COMPILE CONTRACTL @@ -10694,8 +10702,7 @@ void EHWatsonBucketTracker::CopyBucketsFromThrowable(OBJECTREF oThrowable) NOTHROW; GC_NOTRIGGER; MODE_ANY; - PRECONDITION(oThrowable != NULL); - PRECONDITION(((EXCEPTIONREF)oThrowable)->AreWatsonBucketsPresent()); + PRECONDITION(oBuckets != NULL); PRECONDITION(IsWatsonEnabled()); } CONTRACTL_END; @@ -10704,16 +10711,16 @@ void EHWatsonBucketTracker::CopyBucketsFromThrowable(OBJECTREF oThrowable) struct { - OBJECTREF oFrom; + U1ARRAYREF oFromBuckets; } _gc; ZeroMemory(&_gc, sizeof(_gc)); GCPROTECT_BEGIN(_gc); - _gc.oFrom = oThrowable; + _gc.oFromBuckets = oBuckets; - LOG((LF_EH, LL_INFO1000, "EHWatsonBucketTracker::CopyEHWatsonBucketTracker - Copying bucketing details from throwable (%p) to tracker (%p)\n", - OBJECTREFToObject(_gc.oFrom), this)); + LOG((LF_EH, LL_INFO1000, "EHWatsonBucketTracker::CopyEHWatsonBucketTracker - Copying bucketing details from bucket (%p) to tracker (%p)\n", + OBJECTREFToObject(_gc.oFromBuckets), this)); // Watson bucket is a "GenericModeBlock" type. Set up an empty GenericModeBlock // to hold the bucket parameters. @@ -10728,8 +10735,7 @@ void EHWatsonBucketTracker::CopyBucketsFromThrowable(OBJECTREF oThrowable) else { // Get the raw array data pointer - U1ARRAYREF refWatsonBucketArray = ((EXCEPTIONREF)_gc.oFrom)->GetWatsonBucketReference(); - PTR_VOID pRawWatsonBucketArray = dac_cast(refWatsonBucketArray->GetDataPtr()); + PTR_VOID pRawWatsonBucketArray = dac_cast(_gc.oFromBuckets->GetDataPtr()); // Copy over the details to our new allocation memcpyNoGCRefs(pgmb, pRawWatsonBucketArray, sizeof(GenericModeBlock)); diff --git a/src/coreclr/vm/exstatecommon.h b/src/coreclr/vm/exstatecommon.h index ba8086a5402fe1..b2abb9d9f3c1d3 100644 --- a/src/coreclr/vm/exstatecommon.h +++ b/src/coreclr/vm/exstatecommon.h @@ -483,7 +483,7 @@ class EHWatsonBucketTracker EHWatsonBucketTracker(); void Init(); void CopyEHWatsonBucketTracker(const EHWatsonBucketTracker& srcTracker); - void CopyBucketsFromThrowable(OBJECTREF oThrowable); + void CopyBuckets(U1ARRAYREF oBuckets); void SaveIpForWatsonBucket(UINT_PTR ip); UINT_PTR RetrieveWatsonBucketIp(); PTR_VOID RetrieveWatsonBuckets(); diff --git a/src/coreclr/vm/object.cpp b/src/coreclr/vm/object.cpp index 36b100d6e9125a..af2201cbf4dce6 100644 --- a/src/coreclr/vm/object.cpp +++ b/src/coreclr/vm/object.cpp @@ -1033,6 +1033,41 @@ OBJECTREF::OBJECTREF(const OBJECTREF & objref) } +//------------------------------------------------------------- +// VolatileLoadWithoutBarrier constructor +//------------------------------------------------------------- +OBJECTREF::OBJECTREF(const OBJECTREF *pObjref, tagVolatileLoadWithoutBarrier tag) +{ + STATIC_CONTRACT_NOTHROW; + STATIC_CONTRACT_GC_NOTRIGGER; + STATIC_CONTRACT_MODE_COOPERATIVE; + STATIC_CONTRACT_FORBID_FAULT; + + Object* objrefAsObj = VolatileLoadWithoutBarrier(&pObjref->m_asObj); + VALIDATEOBJECT(objrefAsObj); + + // !!! If this assert is fired, there are two possibilities: + // !!! 1. You are doing a type cast, e.g. *(OBJECTREF*)pObj + // !!! Instead, you should use ObjectToOBJECTREF(*(Object**)pObj), + // !!! or ObjectToSTRINGREF(*(StringObject**)pObj) + // !!! 2. There is a real GC hole here. + // !!! Either way you need to fix the code. + _ASSERTE(Thread::IsObjRefValid(pObjref)); + if ((objrefAsObj != 0) && + ((IGCHeap*)GCHeapUtilities::GetGCHeap())->IsHeapPointer( (BYTE*)this )) + { + _ASSERTE(!"Write Barrier violation. Must use SetObjectReference() to assign OBJECTREF's into the GC heap!"); + } + m_asObj = objrefAsObj; + + if (m_asObj != 0) { + ENABLESTRESSHEAP(); + } + + Thread::ObjectRefNew(this); +} + + //------------------------------------------------------------- // To allow NULL to be used as an OBJECTREF. //------------------------------------------------------------- diff --git a/src/coreclr/vm/object.h b/src/coreclr/vm/object.h index e35bd90fd68ab5..398b4f49030a2e 100644 --- a/src/coreclr/vm/object.h +++ b/src/coreclr/vm/object.h @@ -2418,7 +2418,7 @@ class ExceptionObject : public Object OBJECTREF GetInnerException() { LIMITED_METHOD_DAC_CONTRACT; - return _innerException; + return VolatileLoadWithoutBarrierOBJECTREF(&_innerException); } // Returns the innermost exception object - equivalent of the @@ -2431,7 +2431,7 @@ class ExceptionObject : public Object OBJECTREF oInnerMostException = NULL; OBJECTREF oCurrent = NULL; - oCurrent = _innerException; + oCurrent = GetInnerException(); while(oCurrent != NULL) { oInnerMostException = oCurrent; @@ -2469,7 +2469,7 @@ class ExceptionObject : public Object STRINGREF GetRemoteStackTraceString() { LIMITED_METHOD_DAC_CONTRACT; - return _remoteStackTraceString; + return (STRINGREF)VolatileLoadWithoutBarrierOBJECTREF(&_remoteStackTraceString); } void SetHelpURL(STRINGREF helpURL) @@ -2512,7 +2512,7 @@ class ExceptionObject : public Object U1ARRAYREF GetWatsonBucketReference() { LIMITED_METHOD_CONTRACT; - return _watsonBuckets; + return (U1ARRAYREF)VolatileLoadWithoutBarrierOBJECTREF(&_watsonBuckets); } // This method will return a BOOL to indicate if the @@ -2520,7 +2520,7 @@ class ExceptionObject : public Object BOOL AreWatsonBucketsPresent() { LIMITED_METHOD_CONTRACT; - return (_watsonBuckets != NULL)?TRUE:FALSE; + return (GetWatsonBucketReference() != NULL)?TRUE:FALSE; } // This method will save the IP to be used for watson bucketing. @@ -2545,7 +2545,7 @@ class ExceptionObject : public Object { LIMITED_METHOD_CONTRACT; - return _ipForWatsonBuckets; + return VolatileLoadWithoutBarrier(&_ipForWatsonBuckets); } // README: diff --git a/src/coreclr/vm/vars.hpp b/src/coreclr/vm/vars.hpp index 9400fd130132f6..ac857948d5137b 100644 --- a/src/coreclr/vm/vars.hpp +++ b/src/coreclr/vm/vars.hpp @@ -157,6 +157,8 @@ class OBJECTREF { }; public: + enum class tagVolatileLoadWithoutBarrier { tag }; + //------------------------------------------------------------- // Default constructor, for non-initializing declarations: // @@ -169,6 +171,12 @@ class OBJECTREF { //------------------------------------------------------------- OBJECTREF(const OBJECTREF & objref); + //------------------------------------------------------------- + // Copy constructor, for passing OBJECTREF's as function arguments + // using a volatile without barrier load + //------------------------------------------------------------- + OBJECTREF(const OBJECTREF * pObjref, tagVolatileLoadWithoutBarrier tag); + //------------------------------------------------------------- // To allow NULL to be used as an OBJECTREF. //------------------------------------------------------------- @@ -302,6 +310,7 @@ class REF : public OBJECTREF #define OBJECTREFToObject(objref) ((objref).operator-> ()) #define ObjectToSTRINGREF(obj) (STRINGREF(obj)) #define STRINGREFToObject(objref) (*( (StringObject**) &(objref) )) +#define VolatileLoadWithoutBarrierOBJECTREF(pObj) (OBJECTREF(pObj, OBJECTREF::tagVolatileLoadWithoutBarrier::tag)) // the while (0) syntax below is to force a trailing semicolon on users of the macro #define VALIDATEOBJECT(obj) do {if ((obj) != NULL) (obj)->Validate();} while (0) @@ -316,6 +325,7 @@ class REF : public OBJECTREF #define OBJECTREFToObject(objref) ((PTR_Object) (objref)) #define ObjectToSTRINGREF(obj) ((PTR_StringObject) (obj)) #define STRINGREFToObject(objref) ((PTR_StringObject) (objref)) +#define VolatileLoadWithoutBarrierOBJECTREF(pObj) VolatileLoadWithoutBarrier(pObj) #endif // _DEBUG_IMPL diff --git a/src/tests/Regressions/coreclr/GitHub_45929/test45929.cs b/src/tests/Regressions/coreclr/GitHub_45929/test45929.cs index 72632a5b4aaf6b..ba783a498a7de7 100644 --- a/src/tests/Regressions/coreclr/GitHub_45929/test45929.cs +++ b/src/tests/Regressions/coreclr/GitHub_45929/test45929.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Diagnostics; using System.Reflection; using System.Runtime.ExceptionServices; using System.Threading; @@ -46,18 +47,40 @@ public static void Run() long progress = 0; var test = new Test(); const int MaxCount = 1000000; - Parallel.For( - 0, - MaxCount, - new ParallelOptions() { MaxDegreeOfParallelism = Environment.ProcessorCount }, - i => + int increment = 100; + bool done = false; + Stopwatch stopwatch = new Stopwatch(); + stopwatch.Start(); + Console.WriteLine($"{DateTime.Now} : {progress * 100D / MaxCount:000.0}% : {stopwatch.ElapsedMilliseconds}"); + + Action makeProgress = i => { - if (Interlocked.Increment(ref progress) % 10000 == 0) + if (done) return; + long newProgress = Interlocked.Increment(ref progress); + if (newProgress % increment == 0) { - Console.WriteLine($"{DateTime.Now} : {progress * 100D / MaxCount:000.0}%"); + int newIncrement = (increment * 3) / 2; + if (newIncrement > 10000) + newIncrement = 10000; + increment = newIncrement; + + Console.WriteLine($"{DateTime.Now} : {newProgress * 100D / MaxCount:000.0}% : {stopwatch.ElapsedMilliseconds}"); + if (stopwatch.ElapsedMilliseconds > 150000) + { + Console.WriteLine($"Attempting to finish early"); + done = true; + } } test.Invoke(); - }); + }; + + makeProgress(0); + + Parallel.For( + 1, + MaxCount, + new ParallelOptions() { MaxDegreeOfParallelism = Environment.ProcessorCount }, + makeProgress); } public void Invoke()