[Support] Don't print stacktrace if DbgHelp.dll hasn't been loaded yet

[werat]

Summary

On Windows certain function from Signals.h require that DbgHelp.dll is loaded. This typically happens when the main program calls llvm::InitLLVM, however in some cases main program doesn't do that (e.g. when the application is using LLDB via liblldb.dll). This patch adds a safe guard to prevent crashes. More discussion in
https://reviews.llvm.org/D119009.

[aganea]

Do you think you can add a unit test (in SupportTests) that demonstrates the issue before your patch?

[aganea]

Maybe take a look at llvm/unittests/Support/ProgramTest.cpp (for example TestExecuteAndWaitTimeout). You should be able to start the executable again with an extra env. variable which prevents the call to PrintStackTraceOnErrorSignal in llvm/utils/unittest/UnitTestMain/TestMain.cpp.

[werat]

Added a unit test.

[werat]

In D119181#3302485, @aganea wrote:

Maybe take a look at llvm/unittests/Support/ProgramTest.cpp (for example TestExecuteAndWaitTimeout). You should be able to start the executable again with an extra env. variable which prevents the call to PrintStackTraceOnErrorSignal in llvm/utils/unittest/UnitTestMain/TestMain.cpp.

I've added a test and verified that on Windows it fails without the safeguard in Signals.inc. Thanks for the pointer!

[aganea]

LGTM with a comment:

llvm/utils/unittest/UnitTestMain/TestMain.cpp

25–30

I think it's more desirable to not have an empty block. You could could do if (!getenv...) and add the comment above the if statement.

[werat]

Remove empty if block

[werat]

Thanks! I've removed the empty if block.

llvm/lib/Support/Windows/Signals.inc

 static fpSymSetOptions fSymSetOptions;
 
 typedef BOOL (WINAPI *fpSymInitialize)(HANDLE, PCSTR, BOOL);
 static fpSymInitialize fSymInitialize;
 
 typedef BOOL (WINAPI *fpEnumerateLoadedModules)(HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID);
 static fpEnumerateLoadedModules fEnumerateLoadedModules;
 
+static bool isDebugHelpInitialized() {
+  return fStackWalk64 && fSymInitialize && fSymSetOptions && fMiniDumpWriteDump;
+}
+
 static bool load64BitDebugHelp(void) {
   HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
   if (hLib) {
     fMiniDumpWriteDump = (fpMiniDumpWriteDump)
                       ::GetProcAddress(hLib, "MiniDumpWriteDump");
     fStackWalk64 = (fpStackWalk64)
                       ::GetProcAddress(hLib, "StackWalk64");
     fSymGetModuleBase64 = (fpSymGetModuleBase64)
                       ::GetProcAddress(hLib, "SymGetModuleBase64");
     fSymGetSymFromAddr64 = (fpSymGetSymFromAddr64)
                       ::GetProcAddress(hLib, "SymGetSymFromAddr64");
     fSymGetLineFromAddr64 = (fpSymGetLineFromAddr64)
                       ::GetProcAddress(hLib, "SymGetLineFromAddr64");
     fSymGetModuleInfo64 = (fpSymGetModuleInfo64)
                       ::GetProcAddress(hLib, "SymGetModuleInfo64");
     fSymFunctionTableAccess64 = (fpSymFunctionTableAccess64)
                      ::GetProcAddress(hLib, "SymFunctionTableAccess64");
     fSymSetOptions = (fpSymSetOptions)::GetProcAddress(hLib, "SymSetOptions");
     fSymInitialize = (fpSymInitialize)::GetProcAddress(hLib, "SymInitialize");
     fEnumerateLoadedModules = (fpEnumerateLoadedModules)
       ::GetProcAddress(hLib, "EnumerateLoadedModules64");
   }
-  return fStackWalk64 && fSymInitialize && fSymSetOptions && fMiniDumpWriteDump;
+  return isDebugHelpInitialized();
 }
 
 using namespace llvm;
 
 // Forward declare.
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep);
 static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType);
 
static bool findModulesAndOffsets(void **StackTrace, int Depth,
   Data.StrPool = &StrPool;
   fEnumerateLoadedModules(GetCurrentProcess(), findModuleCallback, &Data);
   return true;
 }
 
 static void PrintStackTraceForThread(llvm::raw_ostream &OS, HANDLE hProcess,
                                      HANDLE hThread, STACKFRAME64 &StackFrame,
                                      CONTEXT *Context) {
+  // It's possible that DbgHelp.dll hasn't been loaded yet (e.g. if this
+  // function is called before the main program called `llvm::InitLLVM`).
+  // In this case just return, not stacktrace will be printed.
+  if (!isDebugHelpInitialized())
+    return;
+
   // Initialize the symbol handler.
   fSymSetOptions(SYMOPT_DEFERRED_LOADS | SYMOPT_LOAD_LINES);
   fSymInitialize(hProcess, NULL, TRUE);
 
   // Try llvm-symbolizer first. llvm-symbolizer knows how to deal with both PDBs
   // and DWARF, so it should do a good job regardless of what debug info or
   // linker is in use.
   if (printStackTraceWithLLVMSymbolizer(OS, hProcess, hThread, StackFrame,

llvm/unittests/Support/ProgramTest.cpp

 //===- unittest/Support/ProgramTest.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/Program.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
 #include "gtest/gtest.h"
 #include <stdlib.h>
 #include <thread>
 #if defined(__APPLE__)
 # include <crt_externs.h>
 #elif !defined(_MSC_VER)
 // Forward declare environ in case it's not provided by stdlib.h.
 extern char **environ;
TEST_F(ProgramEnvTest, TestLockFile) {
   ASSERT_NO_ERROR(fs::unlockFile(FD1));
   ProcessInfo WaitResult = llvm::sys::Wait(PI2, 5 /* seconds */, true, &Error);
   ASSERT_TRUE(Error.empty());
   ASSERT_EQ(0, WaitResult.ReturnCode);
   ASSERT_EQ(WaitResult.Pid, PI2.Pid);
   sys::fs::remove(LockedFile);
 }
 
+TEST_F(ProgramEnvTest, TestExecuteWithNoStacktraceHandler) {
+  using namespace llvm::sys;
+
+  if (getenv("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER")) {
+    sys::PrintStackTrace(errs());
+    exit(0);
+  }
+
+  std::string Executable =
+      sys::fs::getMainExecutable(TestMainArgv0, &ProgramTestStringArg1);
+  StringRef argv[] = {
+      Executable,
+      "--gtest_filter=ProgramEnvTest.TestExecuteWithNoStacktraceHandler"};
+
+  addEnvVar("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER=1");
+
+  std::string Error;
+  bool ExecutionFailed;
+  int RetCode = ExecuteAndWait(Executable, argv, getEnviron(), {}, 0, 0, &Error,
+                               &ExecutionFailed);
+  EXPECT_FALSE(ExecutionFailed) << Error;
+  ASSERT_EQ(0, RetCode);
+}
+
 } // end anonymous namespace

llvm/utils/unittest/UnitTestMain/TestMain.cpp

 //===--- utils/unittest/UnitTestMain/TestMain.cpp - unittest driver -------===//
 //
 // 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/CommandLine.h"
 #include "llvm/Support/Signals.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <stdlib.h>
 
 #if defined(_WIN32)
 # include <windows.h>
 # if defined(_MSC_VER)
 #   include <crtdbg.h>
 # endif
 #endif
 
 const char *TestMainArgv0;
 
 int main(int argc, char **argv) {
+  // Skip setting up signal handlers for tests that need to test things without
+  // them configured.
+  if (!getenv("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER")) {
     llvm::sys::PrintStackTraceOnErrorSignal(argv[0],
                                             true /* Disable crash reporting */);
+  }
 
   // Initialize both gmock and gtest.
   testing::InitGoogleMock(&argc, argv);
 
   llvm::cl::ParseCommandLineOptions(argc, argv);
 
   // Make it easy for a test to re-execute itself by saving argv[0].
   TestMainArgv0 = argv[0];