[PATCH] D86657: Add new hidden option -print-crash-IR that prints out IR that caused opt pipeline to crash
Arthur Eubanks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Sep 6 17:56:12 PDT 2020
aeubanks added inline comments.
================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:205-207
+std::mutex mtx;
+// All of the crash reporters that will report on a crash.
+std::vector<PrintCrashIRInstrumentation *> CrashReporters;
----------------
yrouban wrote:
> aeubanks wrote:
> > yrouban wrote:
> > > aeubanks wrote:
> > > > yrouban wrote:
> > > > > please make these both static private fields of //PrintIRInstrumentation//.
> > > > > rename //mtx// with first uppercase.
> > > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
> > > Please elaborate. Do you mean you disagree agree with my suggestion?
> > > I just proposed to hide the //mtx// and //CrashReporters// making them static fields of //PrintCrashIRInstrumentation//:
> > >
> > > StandardInstrumentations.h:
> > > class PrintCrashIRInstrumentation {
> > > ...
> > > // Avoid races when creating/destroying the crash IR printers.
> > > static std::mutex mtx;
> > > // All of the crash reporters that will report on a crash.
> > > static std::vector<PrintCrashIRInstrumentation *> CrashReporters;
> > > }
> > >
> > > StandardInstrumentations.cpp:
> > > std::mutex PrintCrashIRInstrumentation::mtx;
> > > std::vector<PrintCrashIRInstrumentation *> PrintCrashIRInstrumentation::CrashReporters;
> > >
> > > This is essentially the same as
> > > std::mutex mtx;
> > > std::vector<PrintCrashIRInstrumentation *> CrashReporters;
> > >
> > These are global variables with constructors right? LLVM coding guide doesn't allow global constructors. I think that also applies to static members of classes.
> >
> > And I think adding a signal handler per StandardInstrumentation is fine. Apparently LLVM only allows 8 max signal handlers (Signals.cpp), but we probably won't hit that, especially since this is a hidden option and likely we'll only have one StandardInstrumentation in most cases. We can always revisit later if necessary.
> //global variables// ...
> literally yes, but we have plenty of those of type //cl::opt//.
> Another option I see is to create a singleton at the very first use. But it would complicate things.
>
> Adding a signal handler for every instance of StandardInstrumentation is unacceptable for compilers that compile many units in one process. A new instance is created for every call of //llvm::runPassPipeline()//.
It seems you have some experience in globals based on some older llvm-dev threads you were involved in, seems like you have more background on this than me so I won't block on the mutex global (although of course a non-global way would be better if possible and relatively clean).
Ok multiple runs creating multiple StandardInstrumentations makes sense.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86657/new/
https://reviews.llvm.org/D86657
More information about the llvm-commits
mailing list