[Driver] When forcing a crash print the bug report message

[john.brawn]

Summary

Commit a945037e8fd0c30e250a62211469eea6765a36ae moved the printing of the "PLEASE submit a bug report" message to the crash handler, but that means we don't print it when forcing a crash using FORCE_CLANG_DIAGNOSTICS_CRASH. Fix this by adding a function to get the bug report message and printing it when forcing a crash.

[jhenderson]

This seems sensible to me, but I have zero experience with the CrashRecoveryContext class. It might be best to get someone who has more knowledge of it to look at it.

[john.brawn]

Added a couple of reviewers that have recently worked on CrashRecoveryContext.

[aganea]

clang/tools/driver/driver.cpp

518

The only concern I have is that a unrelated call stack will be printed.
Could you possibly add (and use here) a function along the lines of emitBugReportMsg() { errs() << BugReportMsg; }?

[john.brawn]

clang/tools/driver/driver.cpp

518

I had a go at doing that, but then realised that currently the bug report message only exists, and is only printed, when llvm is built with LLVM_ENABLE_BACKTRACES=ON which I don't think is what we want. I've instead adjusted things so that the bug report message is printed in CrashRecoveryContext instead of in the backtrace handler, which also means we can get the message without the backtrace here.

[john.brawn]

Moved BugReportMsg printing into CrashRecoveryContext, and stopped printing of backtrace when forcing a crash.

[aganea]

But that won't work when compiling & crashing with -fno-integrated-cc1, would it? (or if building with cmake ... -DCLANG_SPAWN_CC1=1). In that case, normal crashes (not -gen-reproducer) won't go through the CrashRecoveryContext.
Try running:

$ CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1
$ py your_build_folder/bin/llvm-lit.py -vv -a clang/test/Driver/crash-report.c

See if the commands issued by the test display the "PLEASE submit a bug report" message.

[jhenderson]

In D81672#2101513, @aganea wrote:

But that won't work when compiling & crashing with -fno-integrated-cc1, would it? (or if building with cmake ... -DCLANG_SPAWN_CC1=1). In that case, normal crashes (not -gen-reproducer) won't go through the CrashRecoveryContext.
Try running:

$ CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1
$ py your_build_folder/bin/llvm-lit.py -vv -a clang/test/Driver/crash-report.c

See if the commands issued by the test display the "PLEASE submit a bug report" message.

If I'm following it correctly (I could easily not be), the latest version of the patch will also no longer print the bug report submission message for crashes in other parts of LLVM, not just clang, which was rather the motivation of @gbreynoo's recent changes.

[john.brawn]

Print the bug report message when forcing a crash, but also have the message available even with LLVM_ENABLE_BACKTRACES=OFF.

[MaskRay]

llvm/lib/Support/PrettyStackTrace.cpp

36

This variable is mutable. Please use const char BugReportMsg[]

[jhenderson]

LGTM, but wait for others too, please.

[john.brawn]

llvm/lib/Support/PrettyStackTrace.cpp

36

It needs to be mutable because it is set by llvm::setBugReportMsg.

[MaskRay]

clang/tools/driver/driver.cpp

515

Why llvm::dbgs() << llvm::getBugReportMsg(); when -gen-reproducer is specified? The user requests to generate a reproduce file, but this does not suggest that clang has a bug. Dumping an URL is not very appropriate.

[john.brawn]

clang/tools/driver/driver.cpp

515

Emitting the bug report message is primarily for when FORCE_CLANG_DIAGNOSTICS_CRASH is set (because we can get here either due to that or -gen-reproducer). I could adjust this to only emit the message when that is set if you'd like? Though either way we get the "PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:" message.

[MaskRay]

clang/tools/driver/driver.cpp

515

When FORCE_CLANG_DIAGNOSTICS_CRASH is set or -gen-reproducer is specified, I feel that a URL or "PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT" is not very appropriate because this is not a real crash......

[john.brawn]

clang/tools/driver/driver.cpp

515

FORCE_CLANG_DIAGNOSTICS_CRASH exists exactly to test the diagnostics (at least according to 681e4b8d962ed848d2bd443642e53909af53a6cd which added it) so we should be emitting it them. I'll adjust it to not do so when using -gen-reproducer.

[john.brawn]

Don't print the diagnostic with -gen-reproducer.

[MaskRay]

Thanks!

[jhenderson]

Latest version LGTM too.

clang/test/Driver/crash-report-crashfile.m

 // RUN:   -I %S/Inputs/module -isysroot %/t/i/ \
 // RUN:   -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | \
 // RUN:   FileCheck -check-prefix=CRASH_FLAG %s
 
 @import simple;
 const int x = MODULE_MACRO;
 
 // CRASH_ENV: failing because environment variable 'FORCE_CLANG_DIAGNOSTICS_CRASH' is set
+// CRASH_ENV: PLEASE submit a bug report to {{.*}} and include the crash backtrace, preprocessed source, and associated run script.
 // CRASH_ENV: Preprocessed source(s) and associated run script(s) are located at:
 // CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.m
 // CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.cache
 // CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.sh
 // CRASH_ENV-NEXT: note: diagnostic msg: Crash backtrace is located in
 // CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
 
 // CRASH_FLAG: failing because '-gen-reproducer' is used
+// CRASH_FLAG: PLEASE submit a bug report to {{.*}} and include the crash backtrace, preprocessed source, and associated run script.
 // CRASH_FLAG: Preprocessed source(s) and associated run script(s) are located at:
 // CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.m
 // CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.cache
 // CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.sh
 // CRASH_FLAG-NEXT: note: diagnostic msg: Crash backtrace is located in
 // CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}

clang/test/Driver/crash-report-modules.m

 
 // FIXME: This test creates excessively deep directory hierarchies that cause
 // problems on Windows.
 // UNSUPPORTED: system-windows
 
 @import simple;
 const int x = MODULE_MACRO;
 
+// CHECK: PLEASE submit a bug report to {{.*}} and include the crash backtrace, preprocessed source, and associated run script.
 // CHECK: Preprocessed source(s) and associated run script(s) are located at:
 // CHECK-NEXT: note: diagnostic msg: {{.*}}.m
 // CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
 
 // CHECKSRC: @import simple;
 // CHECKSRC: const int x = 10;
 
 // CHECKSH: # Crash reproducer

clang/test/Driver/crash-report-null.test

 // RUN: env FORCE_CLANG_DIAGNOSTICS_CRASH=1 not %clang -fsyntax-only -x c /dev/null -lstdc++ 2>&1 | FileCheck %s
 
 // FIXME: Investigating. "fatal error: file 'nul' modified since it was first processed"
 // XFAIL: windows-gnu
 
+// CHECK: PLEASE submit a bug report to {{.*}} and include the crash backtrace, preprocessed source, and associated run script.
 // CHECK: Preprocessed source(s) and associated run script(s) are located at:
 // CHECK-NEXT: note: diagnostic msg: {{.*}}null-{{.*}}.c

clang/tools/driver/driver.cpp

if (TheDriver.GenReproducer) {
       Diags.Report(diag::err_drv_force_crash)
         << !::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH");
 
       // Pretend that every command failed.
       FailingCommands.clear();
       for (const auto &J : C->getJobs())
         if (const Command *C = dyn_cast<Command>(&J))
           FailingCommands.push_back(std::make_pair(-1, C));
+
+      llvm::dbgs() << llvm::getBugReportMsg();
     }
 
     for (const auto &P : FailingCommands) {
       int CommandRes = P.first;
       const Command *FailingCommand = P.second;
       if (!Res)
         Res = CommandRes;
 
       // If result status is < 0, then the driver command signalled an error.
       // If result status is 70, then the driver command reported a fatal error.
       // On Windows, abort will return an exit code of 3.  In these cases,

llvm/include/llvm/Support/PrettyStackTrace.h

namespace llvm {
   /// \see EnablePrettyStackTrace
   /// \see PrettyStackTraceEntry
   void EnablePrettyStackTraceOnSigInfoForThisThread(bool ShouldEnable = true);
 
   /// Replaces the generic bug report message that is output upon
   /// a crash.
   void setBugReportMsg(const char *Msg);
 
+  /// Get the bug report message that will be output upon a crash.
+  const char *getBugReportMsg();
+
   /// PrettyStackTraceEntry - This class is used to represent a frame of the
   /// "pretty" stack trace that is dumped when a program crashes. You can define
   /// subclasses of this and declare them on the program stack: when they are
   /// constructed and destructed, they will add their symbolic frames to a
   /// virtual stack trace.  This gets dumped out if the program crashes.
   class PrettyStackTraceEntry {
     friend PrettyStackTraceEntry *ReverseStackTrace(PrettyStackTraceEntry *);
 

llvm/lib/Support/PrettyStackTrace.cpp

 #include <tuple>
 
 #ifdef HAVE_CRASHREPORTERCLIENT_H
 #include <CrashReporterClient.h>
 #endif
 
 using namespace llvm;
 
+static const char *BugReportMsg =
+    "PLEASE submit a bug report to " BUG_REPORT_URL
+    " and include the crash backtrace.\n";
+
 // If backtrace support is not enabled, compile out support for pretty stack
 // traces.  This has the secondary effect of not requiring thread local storage
 // when backtrace support is disabled.
 #if ENABLE_BACKTRACES
 
 // We need a thread local pointer to manage the stack of our stack trace
 // objects, but we *really* cannot tolerate destructors running and do not want
 // to pay any overhead of synchronizing. As a consequence, we use a raw

 #ifdef __APPLE__
 using CrashHandlerString = SmallString<2048>;
 using CrashHandlerStringStorage =
     std::aligned_storage<sizeof(CrashHandlerString),
                          alignof(CrashHandlerString)>::type;
 static CrashHandlerStringStorage crashHandlerStringStorage;
 #endif
 
-static const char *BugReportMsg =
-    "PLEASE submit a bug report to " BUG_REPORT_URL
-    " and include the crash backtrace.\n";
-
 /// This callback is run if a fatal signal is delivered to the process, it
 /// prints the pretty stack trace.
 static void CrashHandler(void *) {
   errs() << BugReportMsg ;
 
 #ifndef __APPLE__
   // On non-apple systems, just emit the crash stack trace to stderr.
   PrintCurStackTrace(errs());
static void printForSigInfoIfNeeded() {
 
   PrintCurStackTrace(errs());
   ThreadLocalSigInfoGenerationCounter = CurrentSigInfoGeneration;
 }
 
 #endif // ENABLE_BACKTRACES
 
 void llvm::setBugReportMsg(const char *Msg) {
-#if ENABLE_BACKTRACES
   BugReportMsg = Msg;
-#endif
+}
+
+const char *llvm::getBugReportMsg() {
+  return BugReportMsg;
 }
 
 PrettyStackTraceEntry::PrettyStackTraceEntry() {
 #if ENABLE_BACKTRACES
   // Handle SIGINFO first, because we haven't finished constructing yet.
   printForSigInfoIfNeeded();
   // Link ourselves.
   NextEntry = PrettyStackTraceHead;