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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

I was thinking about it some more and a bunch of complexity here seems to come from the fact that some stuff happens within the CrashRecoveryContext (i.e. `dsymutilMain`). I believe we can avoid the majority of it by parsing the option, initializing the reproducer, and then running the remaining work in the `RunSafely` lamba.

I also think we should be able to avoid the `dyn_cast` and rely on polymorphism instead. For example we could have a virtual `generate(bool crashed)` method that is called after CRC.RunSafely. For `ReproducerGenerate` it would unconditionally generate the reproducer. For `ReproducerGenerateOnCrash` it would only do so if `crashed == true`.



================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:108
+    Repro = std::make_unique<ReproducerGenerateForCrash>(EC);
+    LLVM_FALLTHROUGH;
+  case ReproducerMode::Generate:
----------------
feg208 wrote:
> 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.
I would prefer to handle the error immediately and make every switch case self contained.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101340