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

Tue Aug 18 16:22:28 PDT 2020

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

Seems reasonable to me too.

The patch itself looks good to me.  Some others have feedback to answer, but I don't see anything that worries me.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86170