From 5033470652491c8f8a01116e9e4c14c63e23c73a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 4 Jun 2020 10:33:35 -0700 Subject: [PATCH 1/2] GC stress fix for reverse pinvoke stubs Reverse PInvoke stubs change the thread to preemptive mode before they exit. Often the epilog immediately follows this helper call, but in some cases there may be a region with a few instructions before we reach the epilog. If there is a normal GC triggered while the method is in this region then the runtime will not report GC references from the stub frame. If the stub method is fully interruptible we will put GC coverage interrupts in this region when doing GC stress. When these trigger a GC then the runtime will report GC references from the stub frame. So if while executing in this region we happen to do a normal GC and then a stress invoked GC, the stress mode GC can find invalid GC references in the frame, as the objects referenced could have been collected/moved by the normal GC that happened just before. There are a couple of avenues for a fix, but since this is a stress mode only problem, I've modified GC stress avoid invoking a GC when the thread is already in preemptive mode and the method is a reverse PINvoke stub. Fixes #13720. --- src/coreclr/src/vm/gccover.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index 9a985e6a421829..052151bddcfc36 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -1411,6 +1411,32 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) return TRUE; } + // If the thread is in preemptive mode then we must be in a + // PInvoke stub with an inlined frame, or be in a reverse PInvoke + // stub that's about to return. + // + // A PInvoke stub should should properly report GC refs if we + // trigger GC here. But a reverse PInvoke stub may over-report + // leading to spurious failures, as we would not normally report + // anything for this method. + // + // So, suppress GC for the reverse PInvoke stubs. + if (!pThread->PreemptiveGCDisabled()) + { + _ASSERTE(pMD->IsILStub()); + PTR_DynamicMethodDesc pDynamicMD = pMD->AsDynamicMethodDesc(); + + if (pDynamicMD->IsReverseStub()) + { + RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr); + return TRUE; + } + else + { + _ASSERTE(pDynamicMD->IsPInvokeStub()); + } + } + #if defined(USE_REDIRECT_FOR_GCSTRESS) && !defined(TARGET_UNIX) // If we're unable to redirect, then we simply won't test GC at this // location. From ad7831a3ace500e02289b430e8e338ecd7450bf7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 4 Jun 2020 13:04:43 -0700 Subject: [PATCH 2/2] revise --- src/coreclr/src/vm/gccover.cpp | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/src/coreclr/src/vm/gccover.cpp b/src/coreclr/src/vm/gccover.cpp index 052151bddcfc36..ddd13152db9e92 100644 --- a/src/coreclr/src/vm/gccover.cpp +++ b/src/coreclr/src/vm/gccover.cpp @@ -1412,29 +1412,17 @@ BOOL OnGcCoverageInterrupt(PCONTEXT regs) } // If the thread is in preemptive mode then we must be in a - // PInvoke stub with an inlined frame, or be in a reverse PInvoke - // stub that's about to return. + // PInvoke stub, a method that has an inline PInvoke frame, + // or be in a reverse PInvoke stub that's about to return. // - // A PInvoke stub should should properly report GC refs if we + // The PInvoke cases should should properly report GC refs if we // trigger GC here. But a reverse PInvoke stub may over-report // leading to spurious failures, as we would not normally report - // anything for this method. - // - // So, suppress GC for the reverse PInvoke stubs. - if (!pThread->PreemptiveGCDisabled()) + // anything for this method at this point. + if (!pThread->PreemptiveGCDisabled() && pMD->HasUnmanagedCallersOnlyAttribute()) { - _ASSERTE(pMD->IsILStub()); - PTR_DynamicMethodDesc pDynamicMD = pMD->AsDynamicMethodDesc(); - - if (pDynamicMD->IsReverseStub()) - { - RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr); - return TRUE; - } - else - { - _ASSERTE(pDynamicMD->IsPInvokeStub()); - } + RemoveGcCoverageInterrupt(instrPtr, savedInstrPtr); + return TRUE; } #if defined(USE_REDIRECT_FOR_GCSTRESS) && !defined(TARGET_UNIX)