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

[Fred Grim via Phabricator via llvm-commits]

feg208 marked 3 inline comments as done.
feg208 added a comment.

I think this covers the comments



================
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 =
----------------
JDevlieghere wrote:
> Let's be consistent with all the surrounding code and the style guide. 
Oh right. Oops.


================
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 {
----------------
JDevlieghere wrote:
> 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? 
Yeah this makes sense and answers my confusion about the right way to go here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101340