[Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

[aganea]

Summary

This is a support patch for D69825.
It handles an exception in the same way as the global exception handler (same side-effect, print call stack & cleanup), and can return the exception code.
It enables CrashRecoveryContext exceptions by default (instead of disabled before) - however the exception handlers are lazily installed on the first creation of a CrashRecoveryContext.

[hans]

This is a support patch for D69825.

Thanks for breaking this out!

It handles an exception in the same way as the global exception handler (same side-effect, print call stack & cleanup), and can return the exception code.

I'm not familiar with this code, so I was struggling with the patch title: "Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions".

Do I understand correctly that the main point is to get a stack trace when CrashRecoveryContext::RunSafely() fails, instead of just returning an error?

It enables CrashRecoveryContext exceptions by default (instead of disabled before) - however the exception handlers are lazily installed on the first creation of a CrashRecoveryContext.

Why does it now need to be enabled by default?

llvm/include/llvm/Support/CrashRecoveryContext.h

26

I guess this needs an update, if it's now being enabled by default?

104

ultra nit: period at the end would be nice :)

llvm/include/llvm/Support/Signals.h

116

Any reason not to use "int" for the return type here? "signed" is unusual as a type in LLVM I think.

llvm/lib/Support/CrashRecoveryContext.cpp

89

" = true" would be more common in LLVM code.

[aganea]

In D70568#1760185, @hans wrote:

Do I understand correctly that the main point is to get a stack trace when CrashRecoveryContext::RunSafely() fails, instead of just returning an error?

Correct. Otherwise, when running with D69825, the callstack was not displayed anymore when a crash occurred in clang-cl. This side-effect is that it also calls the cleanup, thus the change in clang/test/Modules/signal.m (the temp files are properly deleted in the case of a crash).

It enables CrashRecoveryContext exceptions by default (instead of disabled before) - however the exception handlers are lazily installed on the first creation of a CrashRecoveryContext.

Why does it now need to be enabled by default?

I used to call CrashRecoveryContext::Enable() in CC1Command, just before calling the CC1 callback. And to be consistent, call CrashRecoveryContext::Disable() after the CC1 callback.
But then, if we're running several compilations in parallel, it gets complicated, we can't just toggle it on/off all the time - it would need a refcount as the comment in CrashRecoveryContext::Enable() says.
I thought it'd be easier to just have it always enabled, as the signal handlers won't be installed before usage? Do you prefer that, or the refcounting?

[hans]

Sorry for the slow response. This is a pretty complicated patch, so I needed some time to wrap my head around it. I'm still not entirely sure I got it right, but here are some high-level comments.

• About making CrashRecoveryContext::Enable() the default, that seems somewhat orthogonal to this change. You mention the use case of running several compilations in parallel, but I don't think that's a scenario that happens today? If doing this is necessary, I think it would be good to break it out into a separate patch.

• The main purpose of the patch is the new "EnableExceptionHandler" mode for CrashRecoveryContext. I find the name a bit confusing, because on Posix there's no exception handler, and on Win32 an exception handler is used to implement CrashRecoverContext::RunSafely, but that's a different one.
  
  What the mode really does is print a stack trace, write a minidump (on windows), and run cleanups. Perhaps a better name for that would be "DumpStackAndCleanupOnFailure"? Maybe they could be separate flags even.

• The mechanism for doing the stack dump and cleanups looks a bit scary to me. That's sys::InvokeExceptionHandler, a name which doesn't really make sense on Posix where it calls SignalHandler, and passes in the RetCode argument as the signal parameter. On Win32 it calls LLVMUnhandledExceptionFilter, and it does this inside the SEH filter function, although I think it will always return EXCEPTION_EXECUTE_HANDLER, so it could just be called inside the exception handler instead?
  
  On Posix, the call to SignalHandler looks scary, because it does a bunch of signal stuff, and e.g. on S390 hosts it raises the signal again to make sure to crash (which I don't think we want inside a CrashRecoveryContext).
  
  I wish the stack dumping and cleanup could be factored out into something that doesn't involve all the signal stuff, and that could have a clean interface that CrashRecoveryContext could simply call when it catches a crash. Maybe sys::DumpStackAndCleanup(...). Do you think this would be possible?

[aganea]

Thanks for analyzing this!

In D70568#1763769, @hans wrote:

• About making CrashRecoveryContext::Enable() the default, that seems somewhat orthogonal to this change. You mention the use case of running several compilations in parallel, but I don't think that's a scenario that happens today? If doing this is necessary, I think it would be good to break it out into a separate patch.

Even for non-parallel compilations, we still need to enable the CrashRecoveryContext inside the new CC1Command, just before calling RunSafely().
If you look at a previous diff of D69825, in clang/lib/Driver/Job.cpp, L389 there is:

static bool CRCEnabled{};
if (!CRCEnabled) {
  llvm::CrashRecoveryContext::Enable();
  CRCEnabled = true;
}

Which I removed, because I find it a bit awkward to "enable" something that should be decided internally by CrashRecoveryContext (this Enable API sounds more like internal behaviour leaking outside of CrashRecoveryContext).
The purpose of Enable/Disable is either for debugging (when enabling LIBCLANG_DISABLE_CRASH_RECOVERY) or possibly bubbling up the crash, as described in D23662.
Also consider the scenario where we sequentially compile several TUs: clang-cl a.cpp b.cpp c.cpp. Only the first call to CC1Command::Execute() should call llvm::CrashRecoveryContext::Enable().
It is orthogonal, I can send a separate patch. Should we instead call llvm::CrashRecoveryContext::Enable() in D69825, when clang.exe starts?

• The main purpose of the patch is the new "EnableExceptionHandler" mode for CrashRecoveryContext. I find the name a bit confusing, because on Posix there's no exception handler, and on Win32 an exception handler is used to implement CrashRecoverContext::RunSafely, but that's a different one.
  
  What the mode really does is print a stack trace, write a minidump (on windows), and run cleanups. Perhaps a better name for that would be "DumpStackAndCleanupOnFailure"? Maybe they could be separate flags even.

Will do.

• The mechanism for doing the stack dump and cleanups looks a bit scary to me. That's sys::InvokeExceptionHandler, a name which doesn't really make sense on Posix where it calls SignalHandler, and passes in the RetCode argument as the signal parameter. On Win32 it calls LLVMUnhandledExceptionFilter, and it does this inside the SEH filter function, although I think it will always return EXCEPTION_EXECUTE_HANDLER, so it could just be called inside the exception handler instead?
  
  On Posix, the call to SignalHandler looks scary, because it does a bunch of signal stuff, and e.g. on S390 hosts it raises the signal again to make sure to crash (which I don't think we want inside a CrashRecoveryContext).
  
  I wish the stack dumping and cleanup could be factored out into something that doesn't involve all the signal stuff, and that could have a clean interface that CrashRecoveryContext could simply call when it catches a crash. Maybe sys::DumpStackAndCleanup(...). Do you think this would be possible?

I'll make it possible :-)

[hans]

In D70568#1763824, @aganea wrote:

Thanks for analyzing this!

In D70568#1763769, @hans wrote:

• About making CrashRecoveryContext::Enable() the default, that seems somewhat orthogonal to this change. You mention the use case of running several compilations in parallel, but I don't think that's a scenario that happens today? If doing this is necessary, I think it would be good to break it out into a separate patch.

Even for non-parallel compilations, we still need to enable the CrashRecoveryContext inside the new CC1Command, just before calling RunSafely().
If you look at a previous diff of D69825, in clang/lib/Driver/Job.cpp, L389 there is:

static bool CRCEnabled{};
if (!CRCEnabled) {
  llvm::CrashRecoveryContext::Enable();
  CRCEnabled = true;
}

Which I removed, because I find it a bit awkward to "enable" something that should be decided internally by CrashRecoveryContext (this Enable API sounds more like internal behaviour leaking outside of CrashRecoveryContext).
The purpose of Enable/Disable is either for debugging (when enabling LIBCLANG_DISABLE_CRASH_RECOVERY) or possibly bubbling up the crash, as described in D23662.
Also consider the scenario where we sequentially compile several TUs: clang-cl a.cpp b.cpp c.cpp. Only the first call to CC1Command::Execute() should call llvm::CrashRecoveryContext::Enable().
It is orthogonal, I can send a separate patch. Should we instead call llvm::CrashRecoveryContext::Enable() in D69825, when clang.exe starts?

Yes, maybe enabling it on startup is the best way to do it.

[dexonsmith]

This is really cool; we've wanted this for a long time.

One concern I have is that I think this will interfere (effectively disable) automatic OS handling of these crashes, which means they won't be collated and reported anymore. Can we make this configurable somehow? (E.g., leave behind an -ffork-cc1 -fno-fork-cc1 and a CMake flag to pick? Or just the CMake flag?)

(I also have a couple of minor nits inline.)

llvm/lib/Support/CrashRecoveryContext.cpp

100

Can you move the nullptr default assignments back to the header, so it's still obvious how they are set up?

llvm/lib/Support/Unix/Signals.inc

348

I'm not sure I've seen signed in code before. Might it be simpler to just say int for the return type? Or is that somehow incorrect/confusing?

[dexonsmith]

In D70568#1768129, @dexonsmith wrote:

Can we make this configurable somehow? (E.g., leave behind an -ffork-cc1 -fno-fork-cc1 and a CMake flag to pick? Or just the CMake flag?)

(I'm not sure we'd want to expose this on the driver command-line. But making it just a CMake flag is awkward because then you can't run tests for both without configuring twice. Another option that would keep both paths tested is to have an environment variable such as CLANG_FORK_CC1={0,1}, with a CMake flag to pick the default behaviour.)

[dexonsmith]

In D70568#1768138, @dexonsmith wrote:

In D70568#1768129, @dexonsmith wrote:

Sorry, you can ignore all of my high-level comments, I thought I was commenting on https://reviews.llvm.org/D69825.

[aganea]

Addressed comments, and:

• Removed CrashRecoveryContext::HandleCrash() and #pragma clang handle_crash which wasn't covered by any test and most likely deprecated by #pragma clang crash
• Added refcounting to CrashRecoveryContext::Enable/Disable as well as lazy installation of the exception handlers in CrashRecoveryContext.
• Partially reverted rG4624e83ce7b124545b55e45ba13f2d900ed65654/llvm/lib/Support/Unix/Signals.inc to simplify checks for IntSigs (and to fix the issue mentioned here). @vsk

All tests pass on Windows/MSVC2019/Clang9/10 and Linux/Clang9/GCC9.1.

NB: If desired, I could split this down further into smaller patches (before commit?), to simplify bisection if anything goes bad later on.

llvm/include/llvm/Support/CrashRecoveryContext.h

26

I've reverted to the previous behavior, as described in the comment.

[hans]

clang/lib/Lex/Pragma.cpp

1108 ↗

(On Diff #232854)

This was added in http://llvm.org/r111449 and looks like it was just a debugging aid. Perhaps you could just remove it in a separate change (that is, without waiting for the rest of this change).

llvm/include/llvm/Support/CrashRecoveryContext.h

105

The exception terminology only really applies on Windows. Maybe "Selects whether handling of failures should be done in the same way as for regular crashes. When this is active, ...".

llvm/include/llvm/Support/Signals.h

115

What is ExceptContext and what does CleanupOnSignal do with it?

llvm/lib/Support/CrashRecoveryContext.cpp

65–69

It would be good to have a comment that explains the RetCode and ExceptContext parameters.

98

Style nit: function names should start with lower-case letter. (The current code is not very consistent, but at least for new names we should try.)

Also, could this just just be part of (un)installExceptionOrSignalHandlers()? It Seems like it's essentially just doing some extra checks around calls to those, and it seems unfortunate to have both "install()" and "installExceptionOrSignalHandlers()", but one should not be called directly etc.

And on a higher level, it worries me that the set of possible states (enabled/disabled, installed/uninstalled) is so large. I guess this is because CrashRecoveryContexts can be nested and enabled individually?

Could we use asserts to make it clearer what states should be possible? For example, in Install() you do "if (gCrashRecoveryEnabledCount <= 0) return". Why would we even hit this code if there are no enabled contexts? Etc.

140

Don't we need to install it here?

191

I guess the "inline" is not necessary?

llvm/lib/Support/Unix/Signals.inc

212

This seems unrelated? Could it be done in a separate patch?

358

Isn't this redundant with the llvm::is_contained check above, after which we return?

llvm/unittests/Support/CrashRecoveryTest.cpp

108

Isn't this a pretty surprising API, that after calling Disable(), it's still enabled?

[hans]

I looked over this again, and also studied CrashRecoveryContext some more.

I don't really understand why this patch needs to modify the code for how the CRC is enabled and installed, etc.

I thought all we need for in-process-cc1 is to add the DumpStackAndCleanupOnFailure flag and behavior, nothing more.

Do you think this is possible, or am I missing something?

[aganea]

In D70568#1789527, @hans wrote:

I looked over this again, and also studied CrashRecoveryContext some more.

I don't really understand why this patch needs to modify the code for how the CRC is enabled and installed, etc.

I thought all we need for in-process-cc1 is to add the DumpStackAndCleanupOnFailure flag and behavior, nothing more.

Do you think this is possible, or am I missing something?

Hi Hans,
Sorry for taking time to respond.
The way Enable()/Disable() is currently implemented will not work when the tool executes jobs in parallel (ie. llvm-buildozer I presented at LLVM conference; or our re-implementation of /MP which I haven't published yet). It needs refcounting, otherwise one instance might disable the CRC while other threads are running, which effectively disables the crash handlers.
But we can discuss that in a separate review if you prefer. I'll remove it from this patch unless you say otherwise.

[hans]

The way Enable()/Disable() is currently implemented will not work when the tool executes jobs in parallel (ie. llvm-buildozer I presented at LLVM conference; or our re-implementation of /MP which I haven't published yet). It needs refcounting, otherwise one instance might disable the CRC while other threads are running, which effectively disables the crash handlers.
But we can discuss that in a separate review if you prefer. I'll remove it from this patch unless you say otherwise.

Yes, I think it would be good to deal with that separately, to allow for incremental progress.

[hans]

In D70568#1789722, @hans wrote:

The way Enable()/Disable() is currently implemented will not work when the tool executes jobs in parallel (ie. llvm-buildozer I presented at LLVM conference; or our re-implementation of /MP which I haven't published yet). It needs refcounting, otherwise one instance might disable the CRC while other threads are running, which effectively disables the crash handlers.
But we can discuss that in a separate review if you prefer. I'll remove it from this patch unless you say otherwise.

Yes, I think it would be good to deal with that separately, to allow for incremental progress.

Did you have a chance to look into this any more?

[aganea]

Changes as suggested by @hans.

llvm/include/llvm/Support/Signals.h

115

Improved comment.

llvm/lib/Support/CrashRecoveryContext.cpp

98

Reverted this part.

140

Reverted this part.

llvm/lib/Support/Unix/Signals.inc

212

Reverted this part.

358

Above it is checking for "Info" Sigs, whereas here it is "Int" Sigs.

llvm/unittests/Support/CrashRecoveryTest.cpp

108

Reverted this part.

[hans]

Looks good to me. Thanks for pushing this forward!

(Only one very minor comment.)

llvm/unittests/Support/CrashRecoveryTest.cpp

72

Is there a purpose to passing the pointer her, or could it just use incrementGlobal? If we can't, maybe call it WithPointer or something instead (or use an overload) - I was confused by the Cookie part until I realize you just mean it takes some argument.

[aganea]

Thanks for taking the time @hans!

llvm/unittests/Support/CrashRecoveryTest.cpp

72

The cookie thing is because:

using SignalHandlerCallback = void (*)(void *);

/// Add a function to be called when an abort/kill signal is delivered to the
/// process. The handler can have a cookie passed to it to identify what
/// instance of the handler it is.
void AddSignalHandler(SignalHandlerCallback FnPtr, void *Cookie);

Sure, I'll change it to an overload.

[mstorsjo]

llvm/lib/Support/CrashRecoveryContext.cpp

269

This implicitly converts a pointer to uintptr_t, which is an error by default in mingw mode; a cast is needed.

[aganea]

llvm/lib/Support/CrashRecoveryContext.cpp

269

Fixed by rGde797ccdd74f46d5f637ccf66c78da9905a46f42

llvm/include/llvm/Support/CrashRecoveryContext.h

 ///
 /// This class implements support for running operations in a safe context so
 /// that crashes (memory errors, stack overflow, assertion violations) can be
 /// detected and control restored to the crashing thread. Crash detection is
 /// purely "best effort", the exact set of failures which can be recovered from
 /// is platform dependent.
 ///
 /// Clients make use of this code by first calling
 /// CrashRecoveryContext::Enable(), and then executing unsafe operations via a
 /// CrashRecoveryContext object. For example:
 ///
 /// \code
 ///    void actual_work(void *);
 ///
 ///    void foo() {
 ///      CrashRecoveryContext CRC;
 ///
public:
   bool RunSafelyOnThread(void (*Fn)(void*), void *UserData,
                          unsigned RequestedStackSize = 0) {
     return RunSafelyOnThread([&]() { Fn(UserData); }, RequestedStackSize);
   }
 
   /// Explicitly trigger a crash recovery in the current process, and
   /// return failure from RunSafely(). This function does not return.
   void HandleCrash();
+
+  /// In case of a crash, this is the crash identifier.
+  int RetCode = 0;
+
+  /// Selects whether handling of failures should be done in the same way as
+  /// for regular crashes. When this is active, a crash would print the
+  /// callstack, clean-up any temporary files and create a coredump/minidump.
+  bool DumpStackAndCleanupOnFailure = false;
 };
 
 /// Abstract base class of cleanup handlers.
 ///
 /// Derived classes override method recoverResources, which makes actual work on
 /// resource recovery.
 ///
 /// Cleanup handlers are stored in a double list, which is owned and managed by

llvm/include/llvm/Support/Signals.h

namespace sys {
   /// different thread on some platforms.
   ///
   /// This is a no-op on Windows.
   void SetOneShotPipeSignalFunction(void (*Handler)());
 
   /// On Unix systems, this function exits with an "IO error" exit code.
   /// This is a no-op on Windows.
   void DefaultOneShotPipeSignalHandler();
+
+  /// This function does the following:
+  /// - clean up any temporary files registered with RemoveFileOnSignal()
+  /// - dump the callstack from the exception context
+  /// - call any relevant interrupt/signal handlers
+  /// - create a core/mini dump of the exception context whenever possible
+  /// Context is a system-specific failure context: it is the signal type on
+  /// Unix; the ExceptionContext on Windows.
+  void CleanupOnSignal(uintptr_t Context);
 } // End sys namespace
 } // End llvm namespace
 
 #endif

llvm/lib/Support/CrashRecoveryContext.cpp

 //===--- CrashRecoveryContext.cpp - Crash Recovery ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Signals.h"
 #include "llvm/Support/ThreadLocal.h"
 #include <mutex>
 #include <setjmp.h>
+#ifdef _WIN32
+#include <excpt.h> // for GetExceptionInformation
+#endif
+#if LLVM_ON_UNIX
+#include <sysexits.h> // EX_IOERR
+#endif
+
 using namespace llvm;
 
 namespace {
 
 struct CrashRecoveryContextImpl;
 
 static ManagedStatic<
     sys::ThreadLocal<const CrashRecoveryContextImpl> > CurrentContext;
public:
   /// Called when the separate crash-recovery thread was finished, to
   /// indicate that we don't need to clear the thread-local CurrentContext.
   void setSwitchedThread() {
 #if defined(LLVM_ENABLE_THREADS) && LLVM_ENABLE_THREADS != 0
     SwitchedThread = true;
 #endif
   }
 
-  void HandleCrash() {
+  // If the function ran by the CrashRecoveryContext crashes or fails, then
+  // 'RetCode' represents the returned error code, as if it was returned by a
+  // process. 'Context' represents the signal type on Unix; on Windows, it is
+  // the ExceptionContext.
+  void HandleCrash(int RetCode, uintptr_t Context) {
     // Eliminate the current context entry, to avoid re-entering in case the
     // cleanup code crashes.
     CurrentContext->set(Next);
 
     assert(!Failed && "Crash recovery context already failed!");
     Failed = true;
 
-    // FIXME: Stash the backtrace.
+    if (CRC->DumpStackAndCleanupOnFailure)
+      sys::CleanupOnSignal(Context);
+
+    CRC->RetCode = RetCode;
 
     // Jump back to the RunSafely we were called under.
     longjmp(JumpBuffer, 1);
   }
 };
 
 }
 
 static ManagedStatic<std::mutex> gCrashRecoveryContextMutex;
 static bool gCrashRecoveryEnabled = false;
 
 static ManagedStatic<sys::ThreadLocal<const CrashRecoveryContext>>
        tlIsRecoveringFromCrash;
 
 static void installExceptionOrSignalHandlers();
 static void uninstallExceptionOrSignalHandlers();
 
 CrashRecoveryContextCleanup::~CrashRecoveryContextCleanup() {}
 
 CrashRecoveryContext::~CrashRecoveryContext() {
   // Reclaim registered resources.
   CrashRecoveryContextCleanup *i = head;
   const CrashRecoveryContext *PC = tlIsRecoveringFromCrash->get();
   tlIsRecoveringFromCrash->set(this);
   while (i) {
     CrashRecoveryContextCleanup *tmp = i;
     i = tmp->next;
     tmp->cleanupFired = true;

 
 void CrashRecoveryContext::Enable() {
   std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
   // FIXME: Shouldn't this be a refcount or something?
   if (gCrashRecoveryEnabled)
     return;
   gCrashRecoveryEnabled = true;
   installExceptionOrSignalHandlers();
 }
 
 void CrashRecoveryContext::Disable() {
   std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
   if (!gCrashRecoveryEnabled)
     return;
   gCrashRecoveryEnabled = false;
   uninstallExceptionOrSignalHandlers();
 }

 // internal exception handling of other libraries on that thread. SEH works
 // exactly as you would expect normal exception handling to work: it only
 // catches exceptions if they would bubble out from the stack frame with __try /
 // __except.
 
 static void installExceptionOrSignalHandlers() {}
 static void uninstallExceptionOrSignalHandlers() {}
 
-bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
-  if (!gCrashRecoveryEnabled) {
+// We need this function because the call to GetExceptionInformation() can only
+// occur inside the __except evaluation block
+static int ExceptionFilter(bool DumpStackAndCleanup,
+                           _EXCEPTION_POINTERS *Except) {
+  if (DumpStackAndCleanup)
+    sys::CleanupOnSignal((uintptr_t)Except);
+  return EXCEPTION_EXECUTE_HANDLER;
+}
+
+static bool InvokeFunctionCall(function_ref<void()> Fn,
+                               bool DumpStackAndCleanup, int &RetCode) {
+  __try {
     Fn();
-    return true;
-  }
+  } __except (ExceptionFilter(DumpStackAndCleanup, GetExceptionInformation())) {
+    RetCode = GetExceptionCode();
+    return false;
+  }
 
-  bool Result = true;
-  __try {
+bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
+  if (!gCrashRecoveryEnabled) {
     Fn();
-  } __except (1) { // Catch any exception.
-    Result = false;
+    return true;
   }
-  return Result;
+  return InvokeFunctionCall(Fn, DumpStackAndCleanupOnFailure, RetCode);
 }
 
 #else // !_MSC_VER
 
 #if defined(_WIN32)
 // This is a non-MSVC compiler, probably mingw gcc or clang without
 // -fms-extensions. Use vectored exception handling (VEH).
 //
if (!CRCI) {
     CrashRecoveryContext::Disable();
     return EXCEPTION_CONTINUE_SEARCH;
   }
 
   // TODO: We can capture the stack backtrace here and store it on the
   // implementation if we so choose.
 
   // Handle the crash
-  const_cast<CrashRecoveryContextImpl*>(CRCI)->HandleCrash();
+  const_cast<CrashRecoveryContextImpl *>(CRCI)->HandleCrash(
+      (int)ExceptionInfo->ExceptionRecord->ExceptionCode, ExceptionInfo);
 
   // Note that we don't actually get here because HandleCrash calls
   // longjmp, which means the HandleCrash function never returns.
   llvm_unreachable("Handled the crash, should have longjmp'ed out of here");
 }
 
 // Because the Enable and Disable calls are static, it means that
 // there may not actually be an Impl available, or even a current
static void CrashRecoverySignalHandler(int Signal) {
   }
 
   // Unblock the signal we received.
   sigset_t SigMask;
   sigemptyset(&SigMask);
   sigaddset(&SigMask, Signal);
   sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
 
+  // As per convention, -2 indicates a crash or timeout as opposed to failure to
+  // execute (see llvm/include/llvm/Support/Program.h)
+  int RetCode = -2;
+
+  // Don't consider a broken pipe as a crash (see clang/lib/Driver/Driver.cpp)
+  if (Signal == SIGPIPE)
+    RetCode = EX_IOERR;
+
   if (CRCI)
-    const_cast<CrashRecoveryContextImpl*>(CRCI)->HandleCrash();
+    const_cast<CrashRecoveryContextImpl *>(CRCI)->HandleCrash(RetCode, Signal);
 }
 
 static void installExceptionOrSignalHandlers() {
   // Setup the signal handler.
   struct sigaction Handler;
   Handler.sa_handler = CrashRecoverySignalHandler;
   Handler.sa_flags = 0;
   sigemptyset(&Handler.sa_mask);
bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
   return true;
 }
 
 #endif // !_MSC_VER
 
 void CrashRecoveryContext::HandleCrash() {
   CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl;
   assert(CRCI && "Crash recovery context never initialized!");
-  CRCI->HandleCrash();
+  // As per convention, -2 indicates a crash or timeout as opposed to failure to
+  // execute (see llvm/include/llvm/Support/Program.h)
+  CRCI->HandleCrash(-2, 0);
 }
 
 // FIXME: Portability.
 static void setThreadBackgroundPriority() {
 #ifdef __APPLE__
   setpriority(PRIO_DARWIN_THREAD, 0, PRIO_DARWIN_BG);
 #endif
 }

llvm/lib/Support/Unix/Signals.inc

 } // namespace
 
 static StringRef Argv0;
 
 /// Signals that represent requested termination. There's no bug or failure, or
 /// if there is, it's not our direct responsibility. For whatever reason, our
 /// continued execution is no longer desirable.
 static const int IntSigs[] = {
   SIGHUP, SIGINT, SIGTERM, SIGUSR2
 };
 
 /// Signals that represent that we have a bug, and our prompt termination has
 /// been ordered.
 static const int KillSigs[] = {
   SIGILL, SIGTRAP, SIGABRT, SIGFPE, SIGBUS, SIGSEGV, SIGQUIT
 #ifdef SIGSYS
   , SIGSYS
static void UnregisterHandlers() {
   }
 }
 
 /// Process the FilesToRemove list.
 static void RemoveFilesToRemove() {
   FileToRemoveList::removeAllFiles(FilesToRemove);
 }
 
+void sys::CleanupOnSignal(uintptr_t Context) {
+  int Sig = (int)Context;
+
+  if (llvm::is_contained(InfoSigs, Sig)) {
+    InfoSignalHandler(Sig);
+    return;
+  }
+
+  RemoveFilesToRemove();
+
+  if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE)
+    return;
+
+  llvm::sys::RunSignalHandlers();
+}
+
 // The signal handler that runs.
 static RETSIGTYPE SignalHandler(int Sig) {
   // Restore the signal behavior to default, so that the program actually
   // crashes when we return and the signal reissues.  This also ensures that if
   // we crash in our signal handler that the program will terminate immediately
   // instead of recursing in the signal handler.
   UnregisterHandlers();
 

llvm/lib/Support/Windows/Signals.inc

 }
 
 #if defined(__MINGW32__) && !defined(__MINGW64_VERSION_MAJOR)
 // Provide a prototype for RtlCaptureContext, mingw32 from mingw.org is
 // missing it but mingw-w64 has it.
 extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
 #endif
 
-void llvm::sys::PrintStackTrace(raw_ostream &OS) {
-  STACKFRAME64 StackFrame = {};
-  CONTEXT Context = {};
+static void LocalPrintStackTrace(raw_ostream &OS, PCONTEXT C) {
+  STACKFRAME64 StackFrame{};
+  CONTEXT Context{};
+  if (!C) {
     ::RtlCaptureContext(&Context);
+    C = &Context;
+  }
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = Context.Rip;
   StackFrame.AddrStack.Offset = Context.Rsp;
   StackFrame.AddrFrame.Offset = Context.Rbp;
 #elif defined(_M_IX86)
   StackFrame.AddrPC.Offset = Context.Eip;
   StackFrame.AddrStack.Offset = Context.Esp;
   StackFrame.AddrFrame.Offset = Context.Ebp;
 #elif defined(_M_ARM64)
   StackFrame.AddrPC.Offset = Context.Pc;
   StackFrame.AddrStack.Offset = Context.Sp;
   StackFrame.AddrFrame.Offset = Context.Fp;
 #elif defined(_M_ARM)
   StackFrame.AddrPC.Offset = Context.Pc;
   StackFrame.AddrStack.Offset = Context.Sp;
   StackFrame.AddrFrame.Offset = Context.R11;
 #endif
   StackFrame.AddrPC.Mode = AddrModeFlat;
   StackFrame.AddrStack.Mode = AddrModeFlat;
   StackFrame.AddrFrame.Mode = AddrModeFlat;
   PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
-                           StackFrame, &Context);
+                           StackFrame, C);
 }
 
+void llvm::sys::PrintStackTrace(raw_ostream &OS) {
+  LocalPrintStackTrace(OS, nullptr);
+}
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandler();
   InterruptFunction = IF;
   LeaveCriticalSection(&CriticalSection);
 }
 
 void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
   if (!fMiniDumpWriteDump(::GetCurrentProcess(), ::GetCurrentProcessId(),
                           FileHandle, DumpType, ExceptionInfo, NULL, NULL))
     return mapWindowsError(::GetLastError());
 
   llvm::errs() << "Wrote crash dump file \"" << DumpPath << "\"\n";
   return std::error_code();
 }
 
+void sys::CleanupOnSignal(uintptr_t Context) {
+  LLVMUnhandledExceptionFilter((LPEXCEPTION_POINTERS)Context);
+}
+
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
   // We'll automatically write a Minidump file here to help diagnose
   // the nasty sorts of crashes that aren't 100% reproducible from a set of
   // inputs (or in the event that the user is unable or unwilling to provide a
   // reproducible case).
   if (!llvm::sys::Process::AreCoreFilesPrevented()) {
     MINIDUMP_EXCEPTION_INFORMATION ExceptionInfo;
     ExceptionInfo.ThreadId = ::GetCurrentThreadId();
     ExceptionInfo.ExceptionPointers = ep;
     ExceptionInfo.ClientPointers = FALSE;
 
     if (std::error_code EC = WriteWindowsDumpFile(&ExceptionInfo))
       llvm::errs() << "Could not write crash dump file: " << EC.message()
                    << "\n";
   }
 
-  // Initialize the STACKFRAME structure.
-  STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
-  StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
-  StackFrame.AddrPC.Mode = AddrModeFlat;
-  StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
-  StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
-  StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
-  StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
-  HANDLE hProcess = GetCurrentProcess();
-  HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
-                           ep->ContextRecord);
+  LocalPrintStackTrace(llvm::errs(), ep ? ep->ContextRecord : nullptr);
 
-  _exit(ep->ExceptionRecord->ExceptionCode);
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
   // We are running in our very own thread, courtesy of Windows.
   EnterCriticalSection(&CriticalSection);
   Cleanup();
 
   // If an interrupt function has been set, go and run one it; otherwise,

llvm/unittests/Support/CrashRecoveryTest.cpp

 //===- llvm/unittest/Support/CrashRecoveryTest.cpp ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Signals.h"
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
 #define WIN32_LEAN_AND_MEAN
 #define NOGDI
 #include <windows.h>
 #endif
 
 using namespace llvm;
 using namespace llvm::sys;
 
 static int GlobalInt = 0;
 static void nullDeref() { *(volatile int *)0x10 = 0; }
 static void incrementGlobal() { ++GlobalInt; }
 static void llvmTrap() { LLVM_BUILTIN_TRAP; }
+static void incrementGlobalWithParam(void *) { ++GlobalInt; }
 
 TEST(CrashRecoveryTest, Basic) {
   llvm::CrashRecoveryContext::Enable();
   GlobalInt = 0;
   EXPECT_TRUE(CrashRecoveryContext().RunSafely(incrementGlobal));
   EXPECT_EQ(1, GlobalInt);
   EXPECT_FALSE(CrashRecoveryContext().RunSafely(nullDeref));
   EXPECT_FALSE(CrashRecoveryContext().RunSafely(llvmTrap));
TEST(CrashRecoveryTest, Cleanup) {
 
   GlobalInt = 0;
   {
     CrashRecoveryContext CRC;
     CRC.registerCleanup(new IncrementGlobalCleanup(&CRC));
     EXPECT_FALSE(CRC.RunSafely(nullDeref));
   } // run cleanups
   EXPECT_EQ(1, GlobalInt);
+  llvm::CrashRecoveryContext::Disable();
+}
+
+TEST(CrashRecoveryTest, DumpStackCleanup) {
+  SmallString<128> Filename;
+  std::error_code EC = sys::fs::createTemporaryFile("crash", "test", Filename);
+  EXPECT_FALSE(EC);
+  sys::RemoveFileOnSignal(Filename);
+  llvm::sys::AddSignalHandler(incrementGlobalWithParam, nullptr);
+  GlobalInt = 0;
+  llvm::CrashRecoveryContext::Enable();
+  {
+    CrashRecoveryContext CRC;
+    CRC.DumpStackAndCleanupOnFailure = true;
+    EXPECT_TRUE(CRC.RunSafely(noop));
+  }
+  EXPECT_TRUE(sys::fs::exists(Filename));
+  EXPECT_EQ(GlobalInt, 0);
+  {
+    CrashRecoveryContext CRC;
+    CRC.DumpStackAndCleanupOnFailure = true;
+    EXPECT_FALSE(CRC.RunSafely(nullDeref));
+    EXPECT_NE(CRC.RetCode, 0);
+  }
+  EXPECT_FALSE(sys::fs::exists(Filename));
+  EXPECT_EQ(GlobalInt, 1);
+  llvm::CrashRecoveryContext::Disable();
 }
 
 #ifdef _WIN32
 static void raiseIt() {
   RaiseException(123, EXCEPTION_NONCONTINUABLE, 0, NULL);
 }
 
 TEST(CrashRecoveryTest, RaiseException) {
   llvm::CrashRecoveryContext::Enable();
   EXPECT_FALSE(CrashRecoveryContext().RunSafely(raiseIt));
 }
 
 static void outputString() {
   OutputDebugStringA("output for debugger\n");
 }
 
 TEST(CrashRecoveryTest, CallOutputDebugString) {
   llvm::CrashRecoveryContext::Enable();
   EXPECT_TRUE(CrashRecoveryContext().RunSafely(outputString));
 }
 
 #endif