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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:38
 
+Reproducer::Reproducer(StringRef Rt)
+    : VFS{vfs::getRealFileSystem()}, Root{std::move(Rt)} {}
----------------
This should be consistent with the header. It's fine (and common) to use the same name for the argument and member variable.


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:39
+Reproducer::Reproducer(StringRef Rt)
+    : VFS{vfs::getRealFileSystem()}, Root{std::move(Rt)} {}
+
----------------
LLVM generally does not prefer braced initializer lists (https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor) 


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:89
+    Repro = std::make_unique<ReproducerGenerateForCrash>(EC);
+    [[clang::fallthrough]];
+  case ReproducerMode::Generate:
----------------
You should use `LLVM_FALLTHROUGH`, otherwise this won't work if the host compiler is not clang. 


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:91-93
+    if (Repro == nullptr) {
+      Repro = std::make_unique<ReproducerGenerate>(EC);
+    }
----------------
No braces around single line statements (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements)


================
Comment at: llvm/tools/dsymutil/Reproducer.h:22
   Generate,
+  GenerateForCrash,
   Use,
----------------
Maybe `GenerateOnCrash`? 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:38
 
+  const auto &getRoot() const noexcept { return Root; }
+
----------------
LLVM doesn't use exceptions (and generally builds without them). 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:45
 protected:
+  explicit Reproducer(StringRef Root);
+
----------------
If we're going to store the root, this might as well take a `std::string` too and move it into `Root`. 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:47
+
+  SmallString<128> getMappingFileName() const;
+
----------------
Rather than returning a small string with arbitrary lengths, the common pattern in llvm is to take let the caller decide on the storage type and pass in a `llvm::SmallVectorImpl<char>&` that the function then populates. This also avoids any potential copies.


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:490-492
+  auto ArgPair = parseArguments(Argc, Argv);
+  auto &T = ArgPair.first;
+  auto &Args = ArgPair.second;
----------------
I don't think `auto` is a good fit here (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:680-682
     }
-
     Threads.wait();
     if (!AllOK)
----------------
Unrelated whitespace change?


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:706
+  if (!Repro) {
+    dbgs() << "Unable to create reproducer due to "
+           << toString(Repro.takeError()) << '\n';
----------------



================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:710-718
+  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';
----------------
Why is this needed? 


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:740
+    } else {
+      dbgs() << "Failed to parse options in crash dump due to "
+             << toString(OptionsOrErr.takeError()) << '\n';
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101340