[Support] On Windows, handle interrupt signals without crash message

[jhenderson]

Summary

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C. This makes sense, as such signals are not really crashes. Prior to this change, this wasn't the case on Windows, however. This patch changes the Windows behaviour to be consistent with Linux, and adds testing that verifies this.

The test uses llvm-symbolizer, but any tool with an interactive mode would do the job.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45754.

[jhenderson]

If anybody would be willing to verify the test on cygwin, that would be helpful. I'm not 100% confident it will work there, but don't have cygwin installed to verify.

[jhenderson]

Forgot to run this with python 3, and there's a small problem which I need to fix.

[jhenderson]

Made test python3 compatible.

[jhenderson]

Ping?

[grimar]

I do not think I am a good person to accept/review this. Your comments in PR45754 looks reasonable to me though.

[aganea]

LGTM.

[rnk]

lgtm

I don't have Cygwin anymore. IMO your code change seems low risk for cygwin, although the test could always fail with Cygwin Python, but IMO it's not worth worrying about.

[MaskRay]

llvm/test/Support/interrupts.test

4

count 0 < %t.err

or 2>&1 | count 0

[MaskRay]

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C.

That is the default behavior on a *NIX platform: on SIGINT, unregister the handler and call raise(SIGINT) to let the kernel terminate the process.

If a callback is registered via llvm::sys::SetInterruptFunction (used by bugpoint and clangd), that callback will run instead.

[jhenderson]

In D79847#2047739, @MaskRay wrote:

On Linux, the signal handlers are not run on signals such as SIGINT due to CTRL-C.

That is the default behavior on a *NIX platform: on SIGINT, unregister the handler and call raise(SIGINT) to let the kernel terminate the process.

If a callback is registered via llvm::sys::SetInterruptFunction (used by bugpoint and clangd), that callback will run instead.

I wasn't quite sure what this comment was suggesting/discussing. Either way, I'll tweak my commit message to say that this matches the default behaviour for LLVM on Unix (see sys::CleanupOnSignal).

llvm/test/Support/interrupts.test

4

I'm deliberately avoiding the latter, since llvm-symbolizer prints to stdout as part of the script below.

llvm/lib/Support/Windows/Signals.inc

 /// handler it is.
 void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr,
                                  void *Cookie) {
   insertSignalHandler(FnPtr, Cookie);
   RegisterHandler();
   LeaveCriticalSection(&CriticalSection);
 }
 
-static void Cleanup() {
+static void Cleanup(bool ExecuteSignalHandlers) {
   if (CleanupExecuted)
     return;
 
   EnterCriticalSection(&CriticalSection);
 
   // Prevent other thread from registering new files and directories for
   // removal, should we be executing because of the console handler callback.
   CleanupExecuted = true;
 
   // FIXME: open files cannot be deleted.
   if (FilesToRemove != NULL)
     while (!FilesToRemove->empty()) {
       llvm::sys::fs::remove(FilesToRemove->back());
       FilesToRemove->pop_back();
     }
+
+  if (ExecuteSignalHandlers)
     llvm::sys::RunSignalHandlers();
+
   LeaveCriticalSection(&CriticalSection);
 }
 
 void llvm::sys::RunInterruptHandlers() {
   // The interrupt handler may be called from an interrupt, but it may also be
   // called manually (such as the case of report_fatal_error with no registered
   // error handler). We must ensure that the critical section is properly
   // initialized.
   InitializeThreading();
-  Cleanup();
+  Cleanup(true);
 }
 
 /// Find the Windows Registry Key for a given location.
 ///
 /// \returns a valid HKEY if the location exists, else NULL.
 static HKEY FindWERKey(const llvm::Twine &RegistryLocation) {
   HKEY Key;
   if (ERROR_SUCCESS != ::RegOpenKeyExA(HKEY_LOCAL_MACHINE,
WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
   return std::error_code();
 }
 
 void sys::CleanupOnSignal(uintptr_t Context) {
   LLVMUnhandledExceptionFilter((LPEXCEPTION_POINTERS)Context);
 }
 
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
-  Cleanup();
+  Cleanup(true);
 
   // 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();
static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   LocalPrintStackTrace(llvm::errs(), ep ? &ContextCopy : nullptr);
 
   return EXCEPTION_EXECUTE_HANDLER;
 }
 
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
   // We are running in our very own thread, courtesy of Windows.
   EnterCriticalSection(&CriticalSection);
-  Cleanup();
+  // This function is only ever called when a CTRL-C or similar control signal
+  // is fired. Killing a process in this way is normal, so don't trigger the
+  // signal handlers.
+  Cleanup(false);
 
   // If an interrupt function has been set, go and run one it; otherwise,
   // the process dies.
   void (*IF)() = InterruptFunction;
   InterruptFunction = 0;      // Don't run it on another CTRL-C.
 
   if (IF) {
     // Note: if the interrupt function throws an exception, there is nothing

llvm/test/Support/interrupts.test

+## Show that SIGINT and similar signals don't cause crash messages to be
+## reported.
+# RUN: %python %s wrapper llvm-symbolizer 2> %t.err
+# RUN: FileCheck %s --input-file=%t.err --allow-empty --implicit-check-not={{.}}
+
+import os
+import signal
+import subprocess
+import sys
+import time
+
+def run_symbolizer():
+    proc = subprocess.Popen([sys.argv[2]], stdout=subprocess.PIPE, stdin=subprocess.PIPE)
+    # Write then read some output to ensure the process has started fully.
+    proc.stdin.write(b'foo\n')
+    proc.stdin.flush()
+    proc.stdout.readline()
+    # Windows handles signals differently.
+    if os.name == 'nt':
+        os.kill(0, signal.CTRL_BREAK_EVENT)
+    else:
+        proc.send_signal(signal.SIGINT)
+
+# On Windows, this function spawns the subprocess in its own (hidden) console,
+# so that signals do not interfere with the calling test. This isn't necessary
+# on other systems.
+def run_wrapper():
+    args = [sys.executable, __file__, 'symbolizer'] + sys.argv[2:]
+    if os.name == 'nt':
+        startupinfo = subprocess.STARTUPINFO()
+        startupinfo.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+        proc = subprocess.Popen(args,
+                                stderr=sys.stderr,
+                                startupinfo=startupinfo,
+                                creationflags=subprocess.CREATE_NEW_CONSOLE)
+    else:
+        proc = subprocess.Popen(args,
+                                stderr=subprocess.PIPE)
+
+if sys.argv[1] == 'wrapper':
+    run_wrapper()
+else:
+    run_symbolizer()