Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

[probinson]

Summary

There's no automated test for this, as I don't see any existing tests for creating minidumps. I have tried it locally, and it works fine even when the directory name has a space in it.

[rnk]

Seems reasonable, but I have a concern about the cc1 line being replicable

clang/lib/Driver/ToolChains/Clang.cpp

5159

I guess I would remove this ifdef as well and adjust the comment to indicate that this only controls the location of some kinds of crash dumps for some platforms.

llvm/lib/Support/Signals.cpp

46

I guess the one question I have is, should we have these ifdefs? Thinking about it, we generally want the -cc1 line to be platform independent. You should be able to take a cc1 line from Windows and run it on Linux. That is our typical crash reproduction work flow. With that in mind, I think we need this cl::opt on all platforms, even if it does nothing on Linux.

[probinson]

Remove ifdefs, tweak descriptions/comments.

[probinson]

Addressed comments.

[probinson]

Ping.

[rnk]

lgtm

clang/lib/Driver/ToolChains/Clang.cpp

CmdArgs.push_back(!D.CCLogDiagnosticsFilename.empty()
                           : "-");
   }
 
   // Give the gen diagnostics more chances to succeed, by avoiding intentional
   // crashes.
   if (D.CCGenDiagnostics)
     CmdArgs.push_back("-disable-pragma-debug-crash");
 
+  // Allow backend to put its diagnostic files in the same place as frontend
+  // crash diagnostics files.
+  if (Args.hasArg(options::OPT_fcrash_diagnostics_dir)) {
+    StringRef Dir = Args.getLastArgValue(options::OPT_fcrash_diagnostics_dir);
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-crash-diagnostics-dir=" + Dir));
+  }
+
   bool UseSeparateSections = isUseSeparateSections(Triple);
 
   if (Args.hasFlag(options::OPT_ffunction_sections,
                    options::OPT_fno_function_sections, UseSeparateSections)) {
     CmdArgs.push_back("-ffunction-sections");
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {

clang/test/Driver/crash-diagnostics-dir-2.c

+// RUN: %clang -### -fcrash-diagnostics-dir=mydumps -c %s 2>&1 \
+// RUN:     | FileCheck %s --check-prefix=OPTION
+// OPTION: "-crash-diagnostics-dir=mydumps"
+// RUN: %clang -### -c %s 2>&1 | FileCheck %s --check-prefix=NOOPTION
+// NOOPTION-NOT: "-crash-diagnostics-dir

llvm/lib/Support/Signals.cpp

 using namespace llvm;
 
 // Use explicit storage to avoid accessing cl::opt in a signal handler.
 static bool DisableSymbolicationFlag = false;
 static cl::opt<bool, true>
     DisableSymbolication("disable-symbolication",
                          cl::desc("Disable symbolizing crash backtraces."),
                          cl::location(DisableSymbolicationFlag), cl::Hidden);
+static std::string CrashDiagnosticsDirectory;
+static cl::opt<std::string, true>
+    CrashDiagnosticsDir("crash-diagnostics-dir", cl::value_desc("directory"),
+                        cl::desc("Directory for crash diagnostic files."),
+                        cl::location(CrashDiagnosticsDirectory), cl::Hidden);
 
 constexpr char DisableSymbolizationEnv[] = "LLVM_DISABLE_SYMBOLIZATION";
 constexpr char LLVMSymbolizerPathEnv[] = "LLVM_SYMBOLIZER_PATH";
 
 // Callbacks to run in signal handler must be lock-free because a signal handler
 // could be running as we add new callbacks. We don't add unbounded numbers of
 // callbacks, an array is therefore sufficient.
 struct CallbackAndCookie {

llvm/lib/Support/Windows/Signals.inc

WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
   // app-specific key and failing that with the global key.  If none are found
   // default to a normal dump (GetDumpType will return false either if the key
   // is NULL or if there is no valid DumpType value at its location).
   MINIDUMP_TYPE DumpType;
   if (!GetDumpType(AppSpecificKey, DumpType))
     if (!GetDumpType(DefaultLocalDumpsKey, DumpType))
       DumpType = MiniDumpNormal;
 
-  // Look to see if a dump location is specified in the registry; first with the
+  // Look to see if a dump location is specified on the command line.  If not,
+  // look to see if a dump location is specified in the registry; first with the
   // app-specific key and failing that with the global key.  If none are found
   // we'll just create the dump file in the default temporary file location
   // (GetDumpFolder will return false either if the key is NULL or if there is
   // no valid DumpFolder value at its location).
   bool ExplicitDumpDirectorySet = true;
-  SmallString<MAX_PATH> DumpDirectory;
+  SmallString<MAX_PATH> DumpDirectory(CrashDiagnosticsDirectory);
+  if (DumpDirectory.empty())
     if (!GetDumpFolder(AppSpecificKey, DumpDirectory))
       if (!GetDumpFolder(DefaultLocalDumpsKey, DumpDirectory))
         ExplicitDumpDirectorySet = false;
 
   int FD;
   SmallString<MAX_PATH> DumpPath;
 
   if (ExplicitDumpDirectorySet) {
     if (std::error_code EC = fs::create_directories(DumpDirectory))
       return EC;
     if (std::error_code EC = fs::createUniqueFile(