[Support] Ensure handlers are set up before printing the stacktrace

[werat]

Summary

On Windows lldbassert()[1] will segfault[2] if RegisterHandler() from lib/Support/Windows/Signals.inc wasn't called earlier at some point (it loads Dbghelp.dll and initializes the function pointers [3]). The RegisterHandler() function is called from RemoveFileOnSignal, DontRemoveFileOnSignal, SetInterruptFunction, AddSignalHandler and PrintStackTraceOnErrorSignal.

For LLDB PrintStackTraceOnErrorSignal() is called from InitLLVM ctor [4], which is created in the LLDB driver [5]. However in case of using the SB API via liblldb, none of these functions is called and thus any lldbassert() will crash the process.

At first glance it seems SBDebugger::Initialize() can do the initialization, but PrintStackTraceOnErrorSignal requires argv, which is not available there at the moment (and I'm not sure whether this is desirable behavior for the _library_).
It's also possible to just call RegisterHandler() in llvm::sys::PrintStackTrace() to ensure the Dbghelp.dll is always loaded. This patch does that.

Please, let me know if this approach is OK or a different solution should be implemented.

[1] https://github.com/llvm/llvm-project/blob/0236c571810dea1b72d99ee0af124a7a09976e00/lldb/source/Utility/LLDBAssert.cpp#L45
[2] https://github.com/llvm/llvm-project/blob/0236c571810dea1b72d99ee0af124a7a09976e00/llvm/lib/Support/Windows/Signals.inc#L300
[3] https://github.com/llvm/llvm-project/blob/0236c571810dea1b72d99ee0af124a7a09976e00/llvm/lib/Support/Windows/Signals.inc#L163
[4] https://github.com/llvm/llvm-project/blob/0236c571810dea1b72d99ee0af124a7a09976e00/llvm/lib/Support/InitLLVM.cpp#L37
[5] https://github.com/llvm/llvm-project/blob/0236c571810dea1b72d99ee0af124a7a09976e00/lldb/tools/driver/Driver.cpp#L778

[aganea]

I'm not sure it's a good idea to start loading DLLs when we get to this point.
I would rather suggest calling sys::PrintStackTraceOnErrorSignal({}); in SBDebugger::Initialize(). "argv0" will be auto-detected if necessary, otherwise if you can, change the SBDebugger::Initialize() API to pass it? It's only there so that we can find llvm-symbolizer next to the running executable.
You can also add something along those lines to avoid the crash:

static bool isDebugHelpInitialized(void) {
   return fStackWalk64 && fSymInitialize && fSymSetOptions && fMiniDumpWriteDump;
}
...
static void PrintStackTraceForThread(llvm::raw_ostream &OS, HANDLE hProcess,
                                     HANDLE hThread, STACKFRAME64 &StackFrame,
                                     CONTEXT *Context) {
  if (!isDebugHelpInitialized())
    return;

  // Initialize the symbol handler.
  fSymSetOptions(SYMOPT_DEFERRED_LOADS | SYMOPT_LOAD_LINES);

[werat]

Call sys::PrintStackTraceOnErrorSignal() in SBDebugger::Initialize().

[werat]

Initialize signal handlers in PrintStackTraceOnErrorSignal() only once.

[aganea]

Could you please apply clang format just for the changed lines? You can do that usually through git clang-format.

[werat]

In D119009#3297591, @aganea wrote:

I'm not sure it's a good idea to start loading DLLs when we get to this point.
I would rather suggest calling sys::PrintStackTraceOnErrorSignal({}); in SBDebugger::Initialize(). "argv0" will be auto-detected if necessary, otherwise if you can, change the SBDebugger::Initialize() API to pass it? It's only there so that we can find llvm-symbolizer next to the running executable.

Thanks for a quick review! I followed your adviced and added a call to PrintStackTraceOnErrorSignal() in Debugger::Initialize(). It's possible to change the API to accept argv is possible, but it we can't introduce breaking changes, so the old code will not have argv.

Given that now PrintStackTraceOnErrorSignal will be called twice in LLDB driver (and in general case Initialize() can be called many times), I modified the implementation of PrintStackTraceOnErrorSignal a little bit -- don't do anything if ::Argv0 is already set.

[werat]

Revert accidental formatting changes

[werat]

Remove accidental old code

[werat]

This comment was removed by werat.

[labath]

I don't think this is a good idea. I'm very much against libraries randomly installing signal handlers, and I think that is the reason this function is called from the main program in lldb (and elsewhere in llvm afaik). If there's no way to get a backtrace without this on windows, then it'd be better to disable the lldbassert functionality there.

[aganea]

In that case @werat can you get the caller application to call llvm::InitLLVM? That plus the snippet in https://reviews.llvm.org/D119009#3297591 should shield from crashes if the handler isn't installed.

[werat]

In D119009#3302004, @labath wrote:

I don't think this is a good idea. I'm very much against libraries randomly installing signal handlers, and I think that is the reason this function is called from the main program in lldb (and elsewhere in llvm afaik). If there's no way to get a backtrace without this on windows, then it'd be better to disable the lldbassert functionality there.

In D119009#3302118, @aganea wrote:

In that case @werat can you get the caller application to call llvm::InitLLVM? That plus the snippet in https://reviews.llvm.org/D119009#3297591 should shield from crashes if the handler isn't installed.

In my case the application doesn't link againt LLVM libraries, it only uses liblldb.dll (which doesn't export llvm::InitLLVM or llvm::sys::PrintStackTraceOnErrorSignal).
It would be nice to have backtraces when using LLDB as a library, but I agree that only-main-program-install-signal-handlers policy makes a lot of sense.

I will change this patch to install a safeguard from https://reviews.llvm.org/D119009#3297591 then.

[JDevlieghere]

I believe it's generally considered bad practice to install signal handlers in a library. This is why the initialization currently happens in the driver. If you look at other tools in LLVM you'll notice they do the same thing.

Can we address this issue by (1) making sure we don't crash in lldbassert when calling PrintStackTrace without having initialized the signal handlers and (2) providing the ability to install the signal handlers through the SB API (if that doesn't already exist) if the user of libLLDB wants this functionality?

[werat]

In D119009#3302354, @JDevlieghere wrote:

I believe it's generally considered bad practice to install signal handlers in a library. This is why the initialization currently happens in the driver. If you look at other tools in LLVM you'll notice they do the same thing.

Can we address this issue by (1) making sure we don't crash in lldbassert when calling PrintStackTrace without having initialized the signal handlers and (2) providing the ability to install the signal handlers through the SB API (if that doesn't already exist) if the user of libLLDB wants this functionality?

I agree, this approach makes perfect sense to me. I've sent (1) as a separate patch -- https://reviews.llvm.org/D119181. Didn't want to modify this one further to preserve the discussion and the original commit message.
I will look into (2) a bit later.

[labath]

In D119009#3302354, @JDevlieghere wrote:

I believe it's generally considered bad practice to install signal handlers in a library. This is why the initialization currently happens in the driver. If you look at other tools in LLVM you'll notice they do the same thing.

Can we address this issue by (1) making sure we don't crash in lldbassert when calling PrintStackTrace without having initialized the signal handlers and (2) providing the ability to install the signal handlers through the SB API (if that doesn't already exist) if the user of libLLDB wants this functionality?

I think that's a very good approach to the problem.

That said, I want to add that, as far as I can tell, the windows-specific initialization is not actually an issue here. Loading a dll does not have the same global impact as installing a signal handler (if the dll is well-behaved, anyway). So, I can imagine a setup (though I don't know how it would work in detail) where where we initialize the windows-specific parts of this from within lldb, but avoid the parts which installs handlers on other platforms.

I guess it might be interesting to know how is this situation handled for other out-of-tree users of llvm libraries on windows (assuming we have any). Do they get this to work somehow, or is lldb the first one to want to print a backtrace in a non-fatal situation (the non-fatal lldbasserts are definitely... unique).

(I am assuming people won't notice the difference between a crash because the app hit a real assertion failure, or because it failed to print the backtrace of that failure, if they don't know that they should expect to see that backtrace.)

lldb/source/API/SystemInitializerFull.cpp

 #include "lldb/Host/Config.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Initialization/SystemInitializerCommon.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/ProcessTrace.h"
 #include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wglobal-constructors"
 #include "llvm/ExecutionEngine/MCJIT.h"
 #pragma clang diagnostic pop
 
 #include <string>
SystemInitializerFull::SystemInitializerFull()
     : SystemInitializerCommon(g_shlib_dir_helper) {}
 SystemInitializerFull::~SystemInitializerFull() = default;
 
 llvm::Error SystemInitializerFull::Initialize() {
   llvm::Error error = SystemInitializerCommon::Initialize();
   if (error)
     return error;
 
+  // On Windows certain functions from Signals.h require that the global state
+  // is initialized. For LLDB driver the initialization happens in the `main`
+  // function via the `llvm::InitLLVM` helper, but for LLDB dynamic library
+  // (liblldb) that doesn't happen.
+  llvm::sys::PrintStackTraceOnErrorSignal(/*Argv0=*/{});
+
   // Initialize LLVM and Clang
   llvm::InitializeAllTargets();
   llvm::InitializeAllAsmPrinters();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllDisassemblers();
   // Initialize the command line parser in LLVM. This usually isn't necessary
   // as we aren't dealing with command line options here, but otherwise some
   // other code in Clang/LLVM might be tempted to call this function from a

llvm/lib/Support/Unix/Signals.inc

 }
 
 void llvm::sys::DisableSystemDialogsOnCrash() {}
 
 /// When an error signal (such as SIGABRT or SIGSEGV) is delivered to the
 /// process, print a stack trace and then exit.
 void llvm::sys::PrintStackTraceOnErrorSignal(StringRef Argv0,
                                              bool DisableCrashReporting) {
+  // If ::Argv0 is non-empty, then this function has already been called and
+  // there's no need to initialize the error handling again.
+  if (!::Argv0.empty())
+    return;
+
   ::Argv0 = Argv0;
 
   AddSignalHandler(PrintStackTraceSignalHandler, nullptr);
 
 #if defined(__APPLE__) && ENABLE_CRASH_OVERRIDES
   // Environment variable to disable any kind of crash dialog.
   if (DisableCrashReporting || getenv("LLVM_DISABLE_CRASH_REPORT")) {
     mach_port_t self = mach_task_self();

llvm/lib/Support/Windows/Signals.inc

SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX |
                SEM_NOOPENFILEERRORBOX);
   _set_error_mode(_OUT_TO_STDERR);
 }
 
 /// When an error signal (such as SIGABRT or SIGSEGV) is delivered to the
 /// process, print a stack trace and then exit.
 void sys::PrintStackTraceOnErrorSignal(StringRef Argv0,
                                        bool DisableCrashReporting) {
+  // If ::Argv0 is non-empty, then this function has already been called and
+  // there's no need to initialize the error handling again.
+  if (!::Argv0.empty())
+    return;
+
   ::Argv0 = Argv0;
 
   if (DisableCrashReporting || getenv("LLVM_DISABLE_CRASH_REPORT"))
     Process::PreventCoreFiles();
 
   DisableSystemDialogsOnCrash();
   RegisterHandler();
   LeaveCriticalSection(&CriticalSection);