[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]

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:
> > > 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.


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

https://reviews.llvm.org/D86657