[PATCH] D101340: Allows for dsymutil crashes to generate reproduceable information
Jonas Devlieghere via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 28 11:02:31 PDT 2021
JDevlieghere added inline comments.
================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:108
+ Repro = std::make_unique<ReproducerGenerateForCrash>(EC);
+ LLVM_FALLTHROUGH;
+ case ReproducerMode::Generate:
----------------
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.
================
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;
----------------
Do we still need the ability to specify the mode?
================
Comment at: llvm/tools/dsymutil/Reproducer.h:82
+class ReproducerGenerateForCrash : public ReproducerGenerate {
+public:
----------------
================
Comment at: llvm/tools/dsymutil/Reproducer.h:85
+ ReproducerGenerateForCrash(std::error_code &EC);
+ ReproducerGenerateForCrash(ReproducerGenerate &&);
+ void dodump(bool DoDataDump);
----------------
Why do we need a move ctor?
================
Comment at: llvm/tools/dsymutil/Reproducer.h:95
+private:
+ bool DumpFiles = true;
+};
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101340/new/
https://reviews.llvm.org/D101340
More information about the llvm-commits
mailing list