https://github.com/ojhunt updated https://github.com/llvm/llvm-project/pull/219092
>From f8dd3cc4655aea723061cb5d92dbef335b284c57 Mon Sep 17 00:00:00 2001 From: Oliver Hunt <[email protected]> Date: Tue, 25 Aug 2026 13:25:02 -0600 Subject: [PATCH] Fix c-index-test when compiling llvm to a dylib CrashRecoveryContext fails to fully protect against the crashing thread holding a lock, similarly it does not handle a stack allocated TimerGroup on a torn down stack frame (CrashRecoveryContext uses setjmp/longjmp so does not run lock or TimerGroup destructors). This PR makes the CRC more robust to these failures by resetting the lock state on a crash, and by evicting any known dead TimerGroups from the timer list. --- clang/tools/driver/driver.cpp | 15 ++-- llvm/include/llvm/Support/Timer.h | 16 ++++- llvm/lib/Object/OffloadBundle.cpp | 15 ++-- llvm/lib/Support/CrashRecoveryContext.cpp | 3 + llvm/lib/Support/Timer.cpp | 72 +++++++++++++++++++- llvm/unittests/Support/CrashRecoveryTest.cpp | 26 +++++++ 6 files changed, 125 insertions(+), 22 deletions(-) diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp index d4d913a8977a4..dae19ecb00df0 100644 --- a/clang/tools/driver/driver.cpp +++ b/clang/tools/driver/driver.cpp @@ -33,7 +33,6 @@ #include "llvm/Option/ArgList.h" #include "llvm/Option/OptTable.h" #include "llvm/Option/Option.h" -#include "llvm/Support/BuryPointer.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/CrashRecoveryContext.h" #include "llvm/Support/ErrorHandling.h" @@ -455,16 +454,10 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) { *C, *FailingCommand)) Res = 1; - if (!UseNewCC1Process && IsCrash) { - // When crashing in -fintegrated-cc1 mode, bury the timer pointers, because - // the internal linked list might point to already released stack frames. - llvm::BuryPointer(llvm::TimerGroup::acquireTimerGlobals()); - } else { - // If any timers were active but haven't been destroyed yet, print their - // results now. This happens in -disable-free mode. - llvm::TimerGroup::printAll(llvm::errs()); - llvm::TimerGroup::clearAll(); - } + // If any timers were active but haven't been destroyed yet, print their + // results now. This happens in -disable-free mode. + llvm::TimerGroup::printAll(llvm::errs()); + llvm::TimerGroup::clearAll(); #ifdef _WIN32 // Exit status should not be negative on Win32, unless abnormal termination. diff --git a/llvm/include/llvm/Support/Timer.h b/llvm/include/llvm/Support/Timer.h index 097eaf3422ca3..a0cfdac95bcfc 100644 --- a/llvm/include/llvm/Support/Timer.h +++ b/llvm/include/llvm/Support/Timer.h @@ -260,9 +260,19 @@ class TimerGroup { /// global constructors and destructors. LLVM_ABI static void constructForStatistics(); - /// This makes the timer globals unmanaged, and lets the user manage the - /// lifetime. - LLVM_ABI static void *acquireTimerGlobals(); + /// When a crash occurs CrashRecoveryContext needs to ensure that the process + /// can recover from two failure modes: + /// 1. A crash occuring under RunSafelyOnNewStack or similar APIs. This can + /// lead to a lock being held by a now terminated thread; and + /// 2. A crash occuring while a stack allocated timer or timergroup was live. + /// The first case leads to an eventual deadlock due to the global timer lock + /// being held, the latter leads to corruption of the timergroup list due to + /// the presence of dangling references. + /// To fix this CrashRecoveryContext calls recoverFromCrash. This resets the + /// global lock as the only possible holder would be a now dead thread or + /// stackframe, and uses \p StackBoundary to determine which locks are now + /// dead and should be evicted. + LLVM_ABI static void recoverFromCrash(uintptr_t StackBoundary); private: friend class Timer; diff --git a/llvm/lib/Object/OffloadBundle.cpp b/llvm/lib/Object/OffloadBundle.cpp index 428fe24f33d5e..f94cadfff6584 100644 --- a/llvm/lib/Object/OffloadBundle.cpp +++ b/llvm/lib/Object/OffloadBundle.cpp @@ -26,8 +26,11 @@ using namespace llvm; using namespace llvm::object; -static TimerGroup OffloadBundlerTimerGroup("Offload Bundler Timer Group", - "Timer group for offload bundler"); +static TimerGroup &offloadBundlerTimerGroup() { + static TimerGroup Group("Offload Bundler Timer Group", + "Timer group for offload bundler"); + return Group; +} // Returns the on-disk size recorded in the compressed offload bundle header at // the start of \p Blob, or std::nullopt if the header carries no size field. @@ -344,7 +347,7 @@ CompressedOffloadBundle::compress(compression::Params P, if (!compression::zstd::isAvailable() && !compression::zlib::isAvailable()) return createStringError("compression not supported."); Timer HashTimer("Hash Calculation Timer", "Hash calculation time", - OffloadBundlerTimerGroup); + offloadBundlerTimerGroup()); if (VerboseStream) HashTimer.startTimer(); MD5 Hash; @@ -360,7 +363,7 @@ CompressedOffloadBundle::compress(compression::Params P, reinterpret_cast<const uint8_t *>(Input.getBuffer().data()), Input.getBuffer().size()); Timer CompressTimer("Compression Timer", "Compression time", - OffloadBundlerTimerGroup); + offloadBundlerTimerGroup()); if (VerboseStream) CompressTimer.startTimer(); compression::compress(P, BufferUint8, CompressedBuffer); @@ -576,7 +579,7 @@ CompressedOffloadBundle::decompress(const MemoryBuffer &Input, auto StoredHash = Normalized.Hash; Timer DecompressTimer("Decompression Timer", "Decompression time", - OffloadBundlerTimerGroup); + offloadBundlerTimerGroup()); if (VerboseStream) DecompressTimer.startTimer(); @@ -598,7 +601,7 @@ CompressedOffloadBundle::decompress(const MemoryBuffer &Input, // Recalculate MD5 hash for integrity check. Timer HashRecalcTimer("Hash Recalculation Timer", "Hash recalculation time", - OffloadBundlerTimerGroup); + offloadBundlerTimerGroup()); HashRecalcTimer.startTimer(); MD5 Hash; MD5::MD5Result Result; diff --git a/llvm/lib/Support/CrashRecoveryContext.cpp b/llvm/lib/Support/CrashRecoveryContext.cpp index 493ba951fd3e1..7d6720fc45de2 100644 --- a/llvm/lib/Support/CrashRecoveryContext.cpp +++ b/llvm/lib/Support/CrashRecoveryContext.cpp @@ -10,7 +10,9 @@ #include "llvm/Config/llvm-config.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ExitCodes.h" +#include "llvm/Support/ProgramStack.h" #include "llvm/Support/Signals.h" +#include "llvm/Support/Timer.h" #include "llvm/Support/thread.h" #include <cassert> #include <mutex> @@ -433,6 +435,7 @@ bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) { CRCI->ValidJumpBuffer = true; if (setjmp(CRCI->JumpBuffer) != 0) { + TimerGroup::recoverFromCrash(llvm::getStackPointer()); return false; } } diff --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp index b08f5083e00a8..92197a114de1c 100644 --- a/llvm/lib/Support/Timer.cpp +++ b/llvm/lib/Support/Timer.cpp @@ -23,9 +23,11 @@ #include "llvm/Support/ManagedStatic.h" #include "llvm/Support/Mutex.h" #include "llvm/Support/Process.h" +#include "llvm/Support/ProgramStack.h" #include "llvm/Support/Signposts.h" #include "llvm/Support/raw_ostream.h" #include <limits> +#include <new> #include <optional> #if HAVE_UNISTD_H @@ -328,6 +330,74 @@ TimerGroup::~TimerGroup() { unlink(); } +// ***REVIEWER***: I really don't know if this is warranted? I'm unaware of any +// major environment where the stack grows up these days, but there's a +// difference between me being unaware of such, and it not actually existing or +// being a supported host platform. +__attribute__((noinline)) static void +stackGrowsDownResult(uintptr_t BaseStackPtr, bool *Result) { + *Result = llvm::getStackPointer() < BaseStackPtr; +} +__attribute__((noinline)) static bool +stackGrowsDownInner(uintptr_t BaseStackPtr) { + // We use this to force a non-zero-sized stack frame, and we force it to live + // by using it to return the growth direction; + bool StackGrowsDown = false; + stackGrowsDownResult(BaseStackPtr, &StackGrowsDown); + return StackGrowsDown; +} +static bool stackGrowsDown() { + static bool GrowsDown = stackGrowsDownInner(llvm::getStackPointer()); + return GrowsDown; +} + +void TimerGroup::recoverFromCrash(uintptr_t StackBoundary) { + if (!isTimerGlobalsConstructed()) + return; + + // Reset the global timer group lock. We cannot call the destructor as it may + // currently be held by a dead thread, so we simply reinitialize in place. + sys::SmartMutex<true> &Lock = timerLock(); + new (&Lock) sys::SmartMutex<true>(); + + sys::SmartScopedLock<true> L(Lock); + + // Drop any dead stack allocated timer groups. We have to be careful + // to avoid ever trying to use or dereference any of these pointers until + // we have verified that they are still in a live part of the stack. + // We have to compare to StackBoundary rather than local stack position + // as our own stack frame may be one of the frames responsible for overwriting + // the dangling TimerGroups we're evicting. + auto IsTimerGroupInDeadFrame = [&](const void *Candidate) { + if (stackGrowsDown()) + return (uintptr_t)Candidate < StackBoundary; + return (uintptr_t)Candidate > StackBoundary; + }; + + TimerGroup **PrevGroupSlot = &TimerGroupList; + TimerGroup *TG = TimerGroupList; + while (TG) { + if (IsTimerGroupInDeadFrame(TG)) { + *PrevGroupSlot = nullptr; + break; + } + + Timer **PrevTimerSlot = &TG->FirstTimer; + Timer *T = TG->FirstTimer; + while (T) { + if (IsTimerGroupInDeadFrame(T)) { + *PrevTimerSlot = nullptr; + break; + } + PrevTimerSlot = &T->Next; + T = T->Next; + } + + PrevGroupSlot = &TG->Next; + TG = TG->Next; + } +} + void TimerGroup::removeTimer(Timer &T) { sys::SmartScopedLock<true> L(timerLock()); @@ -571,8 +641,6 @@ void TimerGroup::constructForStatistics() { ManagedTimerGlobals->initDeferred(); } -void *TimerGroup::acquireTimerGlobals() { return ManagedTimerGlobals.claim(); } - static bool isTimerGlobalsConstructed() { return ManagedTimerGlobals.isConstructed(); } diff --git a/llvm/unittests/Support/CrashRecoveryTest.cpp b/llvm/unittests/Support/CrashRecoveryTest.cpp index ceafba5b36f11..bfafa494e0c9a 100644 --- a/llvm/unittests/Support/CrashRecoveryTest.cpp +++ b/llvm/unittests/Support/CrashRecoveryTest.cpp @@ -14,6 +14,7 @@ #include "llvm/Support/FileSystem.h" #include "llvm/Support/Program.h" #include "llvm/Support/Signals.h" +#include "llvm/Support/Timer.h" #include "llvm/Support/raw_ostream.h" #include "llvm/TargetParser/Host.h" #include "llvm/TargetParser/Triple.h" @@ -47,6 +48,31 @@ TEST(CrashRecoveryTest, Basic) { EXPECT_FALSE(CrashRecoveryContext().RunSafely(llvmTrap)); } +// Mirrors clang's `#pragma clang __debug crash`, which keeps a Timer live on +// the stack across a deliberate crash (clang/lib/Lex/Pragma.cpp). Recovery +// runs no destructors, so without TimerGroup::recoverFromCrash() this leaves +// a dangling entry in the shared, default TimerGroup's list. +static void crashWithLiveTimer() { + llvm::Timer T("crash-recovery-test", "timer live during crash"); + llvm::TimeRegion R(&T); + llvmTrap(); +} + +TEST(CrashRecoveryTest, TimerReuseAfterCrash) { + llvm::CrashRecoveryContext::Enable(); + EXPECT_FALSE(CrashRecoveryContext().RunSafely(crashWithLiveTimer)); + + // An unrelated, later Timer on the same (default) group must construct and + // destroy without hanging. + { + llvm::Timer T("crash-recovery-test-2", "unrelated later timer"); + T.startTimer(); + T.stopTimer(); + } + + llvm::CrashRecoveryContext::Disable(); +} + struct IncrementGlobalCleanup : CrashRecoveryContextCleanup { IncrementGlobalCleanup(CrashRecoveryContext *CRC) : CrashRecoveryContextCleanup(CRC) {} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
