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

[Fred Grim via Phabricator via llvm-commits]

feg208 added a comment.

I have a few outstanding questions



================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:108
+    Repro = std::make_unique<ReproducerGenerateForCrash>(EC);
+    LLVM_FALLTHROUGH;
+  case ReproducerMode::Generate:
----------------
JDevlieghere wrote:
> Why is this a fallthrough? It seems like now, if we fail to create the `ReproducerGenerateForCrash` instance, we'll try creating a regular `ReproducerGenerate` and only then print the error message? I think we should just handle the EC immediately. 
I don't think so. But maybe my read is incorrect? The unique_ptr would still get created and thus the nullptr check wouldn't pass and so the ec check below would still do its thing.


================
Comment at: llvm/tools/dsymutil/Reproducer.h:66-67
 public:
-  ReproducerGenerate(std::error_code &EC);
+  ReproducerGenerate(std::error_code &EC,
+                     ReproducerMode Mode = ReproducerMode::Generate);
   ~ReproducerGenerate() override;
----------------
JDevlieghere wrote:
> Do we still need the ability to specify the mode? 
thats there for the classof dyn_cast business.


================
Comment at: llvm/tools/dsymutil/Reproducer.h:85
+  ReproducerGenerateForCrash(std::error_code &EC);
+  ReproducerGenerateForCrash(ReproducerGenerate &&);
+  void dodump(bool DoDataDump);
----------------
JDevlieghere wrote:
> Why do we need a move ctor? 
So the answer is found around line 734 in dsymutil.cpp. Since we pass a unique_ptr into the main function for the Reproducer if the user selects -gen-reproducer and the program then crashes we need to move the ReproducerGenerate object into the GenerateForCrash so we get the expected messages.


================
Comment at: llvm/tools/dsymutil/Reproducer.h:95
+private:
+  bool DumpFiles = true;
+};
----------------
JDevlieghere wrote:
> I'm not sure if this is really something we want, with any real-world project the number of object files can quickly become overwhelming. Even if we do, this should probably be a property of the `ReproducerGenerate` and not be specific for the crash one. 
Since we are passing the Reproducer unique_ptr into the main function and then, in the case where no -gen-reproducer is passed and the program didn't crash we need to make sure we don't dump the files. That was the intent here. But I can (and did) move it to the parent class. So a question that arises from your comment...should we clean up the dumped files on a crash back to just the binary plus mapping file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101340