[PATCH] D101340: Allows for dsymutil crashes to generate reproduceable information

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:53-63
   FC->writeMapping(Mapping.str());
-  outs() << "reproducer written to " << Root << '\n';
+  outs() << "Reproducer Directory: " << getRoot() << '\n';
+}
+
+ReproducerGenerateForCrash::~ReproducerGenerateForCrash() {
+  dbgs() << "\n********************\n\n"
+            "PLEASE ATTACH THE FILES IN THE FOLLOWING DIRECTORY TO THE BUG "
----------------
We need the whole directory, so might as well ask the reporter to include it as a whole. 

```
********************
PLEASE ATTACH THE FOLLOWING DIRECTORY TO THE BUG REPORT:
```



================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:85
 Reproducer::createReproducer(ReproducerMode Mode, StringRef Root) {
+  std::unique_ptr<Reproducer> Repro{nullptr};
+  std::error_code EC;
----------------
The `nullptr` here is redundant. 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:40
 
+  const auto &getRoot() const { return Root; }
+
----------------
Don't use auto here, it's not obvious from context what the type is. 


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:494
+static int dsymutilMain(int Argc, char **Argv) {
+  std::string ProgName{Argv[0]};
+  std::pair<DsymutilOptTable, llvm::opt::InputArgList> ArgPair =
----------------
Let's be consistent with all the surrounding code and the style guide. 


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:726-734
+  auto VFS = (*Repro)->getVFS();
+  for (auto &InputFile : Options.InputFiles) {
+    auto DebugMapPtrsOrErr =
+        parseDebugMap(VFS, InputFile, Options.Archs, {}, false, false, false);
+    if (auto EC = DebugMapPtrsOrErr.getError()) {
+      dbgs() << "cannot parse the debug map for '" << InputFile
+             << "': " << EC.message() << '\n';
----------------



================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:749-758
+    auto ArgPair = parseArguments(argc, argv);
+    auto &Args = ArgPair.second;
+    // Now generate all the diagnostic info into the temp directory
+    auto OptionsOrErr = getOptions(Args);
+    if (OptionsOrErr) {
+      generateDiagnosticFiles(*OptionsOrErr);
+    } else {
----------------
I actually don't think this is going to work. You're creating the reproducer after we've crashed and you're not telling dsymutil to use the VFS from that Reproducer instance. It's the VFS that records all access and is what will allow us to copy all the files in place if we decide we need to keep the reproducer.

The way I would imagine this to work is: 

1. Parse the command line options. If we crash at this point we won't have a reproducer, which I think is acceptable. 
2. Initialize the reproducer. 
3. Call CRC.RunSafely to do the actual work. We pass it the reproducer, so it captures all file accesses, which is everything we need for the reproducer. 
4a. If we crash, we generate the reproducer and print the dsymutil invocation to replay the reproducer.
4b. If we don't crash, but `--gen-reproducer` was passed, we do the same

Does that make sense? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101340/new/

https://reviews.llvm.org/D101340