[llvm] 33b6891 - [dsymutil] Automatically generate a reproducer when dsymutil crashes

Jonas Devlieghere via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 14 16:00:14 PDT 2022


Author: Jonas Devlieghere
Date: 2022-06-14T16:00:08-07:00
New Revision: 33b6891db24dd1e6702b4f04d2b08c1bf417dbee

URL: https://github.com/llvm/llvm-project/commit/33b6891db24dd1e6702b4f04d2b08c1bf417dbee
DIFF: https://github.com/llvm/llvm-project/commit/33b6891db24dd1e6702b4f04d2b08c1bf417dbee.diff

LOG: [dsymutil] Automatically generate a reproducer when dsymutil crashes

Automatically generate a reproducer when dsymutil crashes. We already
support generating reproducers with the --gen-reproducer flag, which
emits a reproducer on exit. This patch adds support for doing the same
on a crash and makes it the default behavior.

rdar://68357665

Differential revision: https://reviews.llvm.org/D127441

Added: 
    

Modified: 
    llvm/docs/CommandGuide/dsymutil.rst
    llvm/test/tools/dsymutil/X86/reproducer.test
    llvm/test/tools/dsymutil/cmdline.test
    llvm/tools/dsymutil/Options.td
    llvm/tools/dsymutil/Reproducer.cpp
    llvm/tools/dsymutil/Reproducer.h
    llvm/tools/dsymutil/dsymutil.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/docs/CommandGuide/dsymutil.rst b/llvm/docs/CommandGuide/dsymutil.rst
index a640c1456a711..7ee6566cad421 100644
--- a/llvm/docs/CommandGuide/dsymutil.rst
+++ b/llvm/docs/CommandGuide/dsymutil.rst
@@ -44,7 +44,8 @@ OPTIONS
 
 .. option:: --gen-reproducer
 
- Generate a reproducer consisting of the input object files.
+ Generate a reproducer consisting of the input object files. Alias for
+ --reproducer=GenerateOnExit.
 
 .. option:: --help, -h
 
@@ -110,6 +111,11 @@ OPTIONS
 
  Specify a directory to prepend the paths of the external remark files.
 
+.. option:: --reproducer <mode>
+
+ Specify the reproducer generation mode. Valid options are 'GenerateOnExit',
+ 'GenerateOnCrash', 'Use', 'Off'.
+
 .. option:: --statistics
 
  Print statistics about the contribution of each object file to the linked
@@ -142,7 +148,8 @@ OPTIONS
 
 .. option:: --use-reproducer <path>
 
- Use the object files from the given reproducer path.
+ Use the object files from the given reproducer path. Alias for
+ --reproducer=Use.
 
 .. option:: --verbose
 

diff  --git a/llvm/test/tools/dsymutil/X86/reproducer.test b/llvm/test/tools/dsymutil/X86/reproducer.test
index b2a9b65b6bd67..80db5bd2500e2 100644
--- a/llvm/test/tools/dsymutil/X86/reproducer.test
+++ b/llvm/test/tools/dsymutil/X86/reproducer.test
@@ -14,7 +14,7 @@ RUN: cp %p/../Inputs/basic3.macho.x86_64.o %t/Inputs
 RUN: dsymutil -f -o - -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 | llvm-dwarfdump -a - | FileCheck %s
 
 # Create a reproducer.
-RUN: env DSYMUTIL_REPRODUCER_PATH=%t.repro dsymutil -gen-reproducer -f -o %t.generate -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 | FileCheck %s --check-prefixes=REPRODUCER
+RUN: env DSYMUTIL_REPRODUCER_PATH=%t.repro dsymutil -gen-reproducer -f -o %t.generate -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 2>&1 | FileCheck %s --check-prefixes=REPRODUCER
 RUN: llvm-dwarfdump -a %t.generate | FileCheck %s
 
 # Remove the input files and verify that was successful.
@@ -24,8 +24,8 @@ RUN: not dsymutil -f -o %t.error -oso-prepend-path=%t %t/Inputs/basic.macho.x86_
 # Use the reproducer.
 RUN: dsymutil -use-reproducer %t.repro -f -o - -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 | llvm-dwarfdump -a - | FileCheck %s
 
-# Conflicting options.
-RUN: not dsymutil -gen-reproducer -use-reproducer %t.repro -f -o %t.error -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 2>&1 |  FileCheck %s --check-prefix=CONFLICT
+# Using a reproducer takes precedence.
+RUN: dsymutil -gen-reproducer -use-reproducer %t.repro -f -o - -oso-prepend-path=%t %t/Inputs/basic.macho.x86_64 | llvm-dwarfdump -a - | FileCheck %s
 
 CHECK: .debug_info
 CHECK:  DW_TAG_compile_unit
@@ -75,6 +75,5 @@ CHECK-NEXT:      DW_AT_encoding (DW_ATE_signed_char)
 CHECK-NEXT:      DW_AT_byte_size (0x01)
 CHECK:      NULL
 
-REPRODUCER: reproducer written
+REPRODUCER: Reproducer written
 ERROR: error: cannot parse the debug map
-CONFLICT: cannot combine --gen-reproducer and --use-reproducer

diff  --git a/llvm/test/tools/dsymutil/cmdline.test b/llvm/test/tools/dsymutil/cmdline.test
index 14707110825b4..03a57f62eedbc 100644
--- a/llvm/test/tools/dsymutil/cmdline.test
+++ b/llvm/test/tools/dsymutil/cmdline.test
@@ -23,6 +23,7 @@ CHECK: {{-o <filename>}}
 CHECK: -papertrail
 CHECK: -remarks-output-format <format>
 CHECK: -remarks-prepend-path <path>
+CHECK: -reproducer <mode>
 CHECK: -statistics
 CHECK: -symbol-map
 CHECK: -symtab

diff  --git a/llvm/tools/dsymutil/Options.td b/llvm/tools/dsymutil/Options.td
index 54f02ff1540b9..d500762fc830b 100644
--- a/llvm/tools/dsymutil/Options.td
+++ b/llvm/tools/dsymutil/Options.td
@@ -161,13 +161,19 @@ def: Separate<["-"], "j">,
   HelpText<"Alias for --num-threads">,
   Group<grp_general>;
 
+def reproducer: Separate<["--", "-"], "reproducer">,
+  MetaVarName<"<mode>">,
+  HelpText<"Specify the reproducer generation mode. Valid options are 'GenerateOnExit', 'GenerateOnCrash', 'Use', 'Off'.">,
+  Group<grp_general>;
+def: Joined<["--", "-"], "reproducer=">, Alias<reproducer>;
+
 def gen_reproducer: F<"gen-reproducer">,
-  HelpText<"Generate a reproducer consisting of the input object files.">,
+  HelpText<"Generate a reproducer consisting of the input object files. Alias for --reproducer=GenerateOnExit.">,
   Group<grp_general>;
 
 def use_reproducer: Separate<["--", "-"], "use-reproducer">,
   MetaVarName<"<path>">,
-  HelpText<"Use the object files from the given reproducer path.">,
+  HelpText<"Use the object files from the given reproducer path. Alias for --reproducer=Use.">,
   Group<grp_general>;
 def: Joined<["--", "-"], "use-reproducer=">, Alias<use_reproducer>;
 

diff  --git a/llvm/tools/dsymutil/Reproducer.cpp b/llvm/tools/dsymutil/Reproducer.cpp
index 4f2e0db297e5c..7b06078a104b4 100644
--- a/llvm/tools/dsymutil/Reproducer.cpp
+++ b/llvm/tools/dsymutil/Reproducer.cpp
@@ -26,21 +26,37 @@ static std::string createReproducerDir(std::error_code &EC) {
 Reproducer::Reproducer() : VFS(vfs::getRealFileSystem()) {}
 Reproducer::~Reproducer() = default;
 
-ReproducerGenerate::ReproducerGenerate(std::error_code &EC)
-    : Root(createReproducerDir(EC)) {
+ReproducerGenerate::ReproducerGenerate(std::error_code &EC, int Argc,
+                                       char **Argv, bool GenerateOnExit)
+    : Root(createReproducerDir(EC)), GenerateOnExit(GenerateOnExit) {
+  for (int I = 0; I < Argc; ++I)
+    Args.push_back(Argv[I]);
   if (!Root.empty())
     FC = std::make_shared<FileCollector>(Root, Root);
   VFS = FileCollector::createCollectorVFS(vfs::getRealFileSystem(), FC);
 }
 
 ReproducerGenerate::~ReproducerGenerate() {
+  if (GenerateOnExit && !Generated)
+    generate();
+}
+
+void ReproducerGenerate::generate() {
   if (!FC)
     return;
+  Generated = true;
   FC->copyFiles(false);
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
   FC->writeMapping(Mapping.str());
-  outs() << "reproducer written to " << Root << '\n';
+  errs() << "********************\n";
+  errs() << "Reproducer written to " << Root << '\n';
+  errs() << "Please include the reproducer and the following invocation in "
+            "your bug report:\n";
+  for (llvm::StringRef Arg : Args)
+    errs() << Arg << ' ';
+  errs() << "--use-reproducer " << Root << '\n';
+  errs() << "********************\n";
 }
 
 ReproducerUse::~ReproducerUse() = default;
@@ -60,26 +76,26 @@ ReproducerUse::ReproducerUse(StringRef Root, std::error_code &EC) {
 }
 
 llvm::Expected<std::unique_ptr<Reproducer>>
-Reproducer::createReproducer(ReproducerMode Mode, StringRef Root) {
+Reproducer::createReproducer(ReproducerMode Mode, StringRef Root, int Argc,
+                             char **Argv) {
+
+  std::error_code EC;
+  std::unique_ptr<Reproducer> Repro;
   switch (Mode) {
-  case ReproducerMode::Generate: {
-    std::error_code EC;
-    std::unique_ptr<Reproducer> Repro =
-        std::make_unique<ReproducerGenerate>(EC);
-    if (EC)
-      return errorCodeToError(EC);
-    return std::move(Repro);
-  }
-  case ReproducerMode::Use: {
-    std::error_code EC;
-    std::unique_ptr<Reproducer> Repro =
-        std::make_unique<ReproducerUse>(Root, EC);
-    if (EC)
-      return errorCodeToError(EC);
-    return std::move(Repro);
-  }
+  case ReproducerMode::GenerateOnExit:
+    Repro = std::make_unique<ReproducerGenerate>(EC, Argc, Argv, true);
+    break;
+  case ReproducerMode::GenerateOnCrash:
+    Repro = std::make_unique<ReproducerGenerate>(EC, Argc, Argv, false);
+    break;
+  case ReproducerMode::Use:
+    Repro = std::make_unique<ReproducerUse>(Root, EC);
+    break;
   case ReproducerMode::Off:
-    return std::make_unique<Reproducer>();
+    Repro = std::make_unique<Reproducer>();
+    break;
   }
-  llvm_unreachable("All cases handled above.");
+  if (EC)
+    return errorCodeToError(EC);
+  return Repro;
 }

diff  --git a/llvm/tools/dsymutil/Reproducer.h b/llvm/tools/dsymutil/Reproducer.h
index a53b97f7ca9f4..c88e1dd111912 100644
--- a/llvm/tools/dsymutil/Reproducer.h
+++ b/llvm/tools/dsymutil/Reproducer.h
@@ -17,7 +17,8 @@ namespace dsymutil {
 
 /// The reproducer mode.
 enum class ReproducerMode {
-  Generate,
+  GenerateOnExit,
+  GenerateOnCrash,
   Use,
   Off,
 };
@@ -33,9 +34,11 @@ class Reproducer {
 
   IntrusiveRefCntPtr<vfs::FileSystem> getVFS() const { return VFS; }
 
+  virtual void generate(){};
+
   /// Create a Reproducer instance based on the given mode.
   static llvm::Expected<std::unique_ptr<Reproducer>>
-  createReproducer(ReproducerMode Mode, StringRef Root);
+  createReproducer(ReproducerMode Mode, StringRef Root, int Argc, char **Argv);
 
 protected:
   IntrusiveRefCntPtr<vfs::FileSystem> VFS;
@@ -46,15 +49,27 @@ class Reproducer {
 /// dsymutil.
 class ReproducerGenerate : public Reproducer {
 public:
-  ReproducerGenerate(std::error_code &EC);
+  ReproducerGenerate(std::error_code &EC, int Argc, char **Argv,
+                     bool GenerateOnExit);
   ~ReproducerGenerate() override;
 
+  virtual void generate() override;
+
 private:
   /// The path to the reproducer.
   std::string Root;
 
   /// The FileCollector used by the FileCollectorFileSystem.
   std::shared_ptr<FileCollector> FC;
+
+  /// The input arguments to build the reproducer invocation.
+  llvm::SmallVector<llvm::StringRef, 0> Args;
+
+  /// Whether to generate the reproducer on destruction.
+  bool GenerateOnExit = false;
+
+  /// Whether we already generated the reproducer.
+  bool Generated = false;
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by

diff  --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index abdb4ca4d48ce..d26c0c04ef6f5 100644
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/llvm/tools/dsymutil/dsymutil.cpp
@@ -33,6 +33,7 @@
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileCollector.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
@@ -111,7 +112,7 @@ struct DsymutilOptions {
   std::vector<std::string> InputFiles;
   unsigned NumThreads;
   DWARFVerify Verify = DWARFVerify::None;
-  ReproducerMode ReproMode = ReproducerMode::Off;
+  ReproducerMode ReproMode = ReproducerMode::GenerateOnCrash;
   dsymutil::LinkOptions LinkOpts;
 };
 
@@ -228,6 +229,28 @@ getAccelTableKind(opt::InputArgList &Args) {
   return DwarfLinkerAccelTableKind::Default;
 }
 
+static Expected<ReproducerMode> getReproducerMode(opt::InputArgList &Args) {
+  if (Args.hasArg(OPT_gen_reproducer))
+    return ReproducerMode::GenerateOnExit;
+  if (opt::Arg *Reproducer = Args.getLastArg(OPT_reproducer)) {
+    StringRef S = Reproducer->getValue();
+    if (S == "GenerateOnExit")
+      return ReproducerMode::GenerateOnExit;
+    if (S == "GenerateOnCrash")
+      return ReproducerMode::GenerateOnCrash;
+    if (S == "Use")
+      return ReproducerMode::Use;
+    if (S == "Off")
+      return ReproducerMode::Off;
+    return make_error<StringError>(
+        "invalid reproducer mode: '" + S +
+            "'. Supported values are 'GenerateOnExit', 'GenerateOnCrash', "
+            "'Use', 'Off'.",
+        inconvertibleErrorCode());
+  }
+  return ReproducerMode::GenerateOnCrash;
+}
+
 static Expected<DWARFVerify> getVerifyKind(opt::InputArgList &Args) {
   if (Args.hasArg(OPT_verify))
     return DWARFVerify::Output;
@@ -280,11 +303,14 @@ static Expected<DsymutilOptions> getOptions(opt::InputArgList &Args) {
   if (opt::Arg *ReproducerPath = Args.getLastArg(OPT_use_reproducer)) {
     Options.ReproMode = ReproducerMode::Use;
     Options.ReproducerPath = ReproducerPath->getValue();
+  } else {
+    if (Expected<ReproducerMode> ReproMode = getReproducerMode(Args)) {
+      Options.ReproMode = *ReproMode;
+    } else {
+      return ReproMode.takeError();
+    }
   }
 
-  if (Args.hasArg(OPT_gen_reproducer))
-    Options.ReproMode = ReproducerMode::Generate;
-
   if (Expected<DwarfLinkerAccelTableKind> AccelKind = getAccelTableKind(Args)) {
     Options.LinkOpts.TheAccelTableKind = *AccelKind;
   } else {
@@ -569,8 +595,8 @@ int dsymutil_main(int argc, char **argv) {
   InitializeAllTargets();
   InitializeAllAsmPrinters();
 
-  auto Repro =
-      Reproducer::createReproducer(Options.ReproMode, Options.ReproducerPath);
+  auto Repro = Reproducer::createReproducer(Options.ReproMode,
+                                            Options.ReproducerPath, argc, argv);
   if (!Repro) {
     WithColor::error() << toString(Repro.takeError());
     return EXIT_FAILURE;
@@ -662,72 +688,88 @@ int dsymutil_main(int argc, char **argv) {
       VerifyOutput = false;
     }
 
-    SmallVector<MachOUtils::ArchAndFile, 4> TempFiles;
-    std::atomic_char AllOK(1);
-    for (auto &Map : *DebugMapPtrsOrErr) {
-      if (Options.LinkOpts.Verbose || Options.DumpDebugMap)
-        Map->print(outs());
+    // Set up a crash recovery context.
+    CrashRecoveryContext::Enable();
+    CrashRecoveryContext CRC;
+    CRC.DumpStackAndCleanupOnFailure = true;
 
-      if (Options.DumpDebugMap)
-        continue;
-
-      if (!Options.SymbolMap.empty())
-        Options.LinkOpts.Translator = SymMapLoader.Load(InputFile, *Map);
-
-      if (Map->begin() == Map->end())
-        WithColor::warning()
-            << "no debug symbols in executable (-arch "
-            << MachOUtils::getArchName(Map->getTriple().getArchName()) << ")\n";
-
-      // Using a std::shared_ptr rather than std::unique_ptr because move-only
-      // types don't work with std::bind in the ThreadPool implementation.
-      std::shared_ptr<raw_fd_ostream> OS;
-
-      std::string OutputFile = OutputLocationOrErr->DWARFFile;
-      if (NeedsTempFiles) {
-        TempFiles.emplace_back(Map->getTriple().getArchName().str());
+    std::atomic_char AllOK(1);
+    SmallVector<MachOUtils::ArchAndFile, 4> TempFiles;
 
-        auto E = TempFiles.back().createTempFile();
-        if (E) {
-          WithColor::error() << toString(std::move(E));
-          return EXIT_FAILURE;
+    const bool Crashed = !CRC.RunSafely([&]() {
+      for (auto &Map : *DebugMapPtrsOrErr) {
+        if (Options.LinkOpts.Verbose || Options.DumpDebugMap)
+          Map->print(outs());
+
+        if (Options.DumpDebugMap)
+          continue;
+
+        if (!Options.SymbolMap.empty())
+          Options.LinkOpts.Translator = SymMapLoader.Load(InputFile, *Map);
+
+        if (Map->begin() == Map->end())
+          WithColor::warning()
+              << "no debug symbols in executable (-arch "
+              << MachOUtils::getArchName(Map->getTriple().getArchName())
+              << ")\n";
+
+        // Using a std::shared_ptr rather than std::unique_ptr because move-only
+        // types don't work with std::bind in the ThreadPool implementation.
+        std::shared_ptr<raw_fd_ostream> OS;
+
+        std::string OutputFile = OutputLocationOrErr->DWARFFile;
+        if (NeedsTempFiles) {
+          TempFiles.emplace_back(Map->getTriple().getArchName().str());
+
+          auto E = TempFiles.back().createTempFile();
+          if (E) {
+            WithColor::error() << toString(std::move(E));
+            AllOK.fetch_and(false);
+            return;
+          }
+
+          auto &TempFile = *(TempFiles.back().File);
+          OS = std::make_shared<raw_fd_ostream>(TempFile.FD,
+                                                /*shouldClose*/ false);
+          OutputFile = TempFile.TmpName;
+        } else {
+          std::error_code EC;
+          OS = std::make_shared<raw_fd_ostream>(
+              Options.LinkOpts.NoOutput ? "-" : OutputFile, EC,
+              sys::fs::OF_None);
+          if (EC) {
+            WithColor::error() << OutputFile << ": " << EC.message();
+            AllOK.fetch_and(false);
+            return;
+          }
         }
 
-        auto &TempFile = *(TempFiles.back().File);
-        OS = std::make_shared<raw_fd_ostream>(TempFile.FD,
-                                              /*shouldClose*/ false);
-        OutputFile = TempFile.TmpName;
-      } else {
-        std::error_code EC;
-        OS = std::make_shared<raw_fd_ostream>(
-            Options.LinkOpts.NoOutput ? "-" : OutputFile, EC, sys::fs::OF_None);
-        if (EC) {
-          WithColor::error() << OutputFile << ": " << EC.message();
-          return EXIT_FAILURE;
-        }
+        auto LinkLambda = [&,
+                           OutputFile](std::shared_ptr<raw_fd_ostream> Stream,
+                                       LinkOptions Options) {
+          AllOK.fetch_and(
+              linkDwarf(*Stream, BinHolder, *Map, std::move(Options)));
+          Stream->flush();
+          if (VerifyOutput) {
+            AllOK.fetch_and(verifyOutput(
+                OutputFile, Map->getTriple().getArchName(), Options.Verbose));
+          }
+        };
+
+        // FIXME: The DwarfLinker can have some very deep recursion that can max
+        // out the (significantly smaller) stack when using threads. We don't
+        // want this limitation when we only have a single thread.
+        if (S.ThreadsRequested == 1)
+          LinkLambda(OS, Options.LinkOpts);
+        else
+          Threads.async(LinkLambda, OS, Options.LinkOpts);
       }
 
-      auto LinkLambda = [&, OutputFile](std::shared_ptr<raw_fd_ostream> Stream,
-                                        LinkOptions Options) {
-        AllOK.fetch_and(
-            linkDwarf(*Stream, BinHolder, *Map, std::move(Options)));
-        Stream->flush();
-        if (VerifyOutput) {
-          AllOK.fetch_and(verifyOutput(
-              OutputFile, Map->getTriple().getArchName(), Options.Verbose));
-        }
-      };
-
-      // FIXME: The DwarfLinker can have some very deep recursion that can max
-      // out the (significantly smaller) stack when using threads. We don't
-      // want this limitation when we only have a single thread.
-      if (S.ThreadsRequested == 1)
-        LinkLambda(OS, Options.LinkOpts);
-      else
-        Threads.async(LinkLambda, OS, Options.LinkOpts);
-    }
+      Threads.wait();
+    });
 
-    Threads.wait();
+    if (Crashed)
+      (*Repro)->generate();
 
     if (!AllOK)
       return EXIT_FAILURE;


        


More information about the llvm-commits mailing list