[PATCH] D86170: PrintStackTrace: don't symbolize if LLVM_DISABLE_SYMBOLIZATION is set

Tue Aug 18 14:38:37 PDT 2020

MaskRay added a comment.

In D86170#2224841 <https://reviews.llvm.org/D86170#2224841>, @dblaikie wrote:

> Rather than adding a new option, could we generalize LLVM_DISABLE_CRASH_REPORT to work on all platforms & wire that up to not --crash as you've done, without adding another name for this functionality?
>
> Actually it looks like if we removed the Apple-specific filtering of ENABLE_CRASH_OVERRIDE, then the handling in Signals.inc which also supports the runtime API parameter "DisableCrashReporting" would also do what I was proposing for unit tests (disabling crash reporting on gtest) - which has been the default on MacOS builds for 5 years now? I'm not sure I feel perfectly good about disabling crash reporting for all unit test failures, rather than only disabling it around death tests - but I think it's probably good to make the perf tradeoffs the same on different platforms, and that this has been the default for 5 years for a good chunk of the LLVM developers (those on mac) it's probably good enough to ship.

http://lists.llvm.org/pipermail/llvm-dev/2017-May/113292.html says the following. Your comment suggests the opposite. I am fine with either way.

I actually slightly prefer the name LLVM_DISABLE_DISABLE_SYMBOLIZATION over LLVM_DISABLE_CRASH_REPORT because the former appears to be more appropriate.

> We have the LLVM_DISABLE_CRASH_REPORT environment variable, but that's more

about whether we should do crash reporting or not.

> It would be pretty reasonable to have another one to disable all this

stuff. It would also be reasonable to have a cmake option that compiles
this stuff away, since it basically never works on user machines that don't
have debug info.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86170