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

Wed Aug 19 16:12:05 PDT 2020

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D86170#2227354 <https://reviews.llvm.org/D86170#2227354>, @MaskRay wrote:

> In D86170#2225377 <https://reviews.llvm.org/D86170#2225377>, @dblaikie wrote:
>
>> In D86170#2224853 <https://reviews.llvm.org/D86170#2224853>, @MaskRay wrote:
>>
>>> 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.
>>>
>>> @rnk wrote:
>>>
>>>> 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.
>>
>> Ah, I see - I misunderstood what the existing codepath does. I guess/think it's about those sort of OS-level pop-up crash dialogs that some operating systems have? So that makes sense that it'd be disabled for all tests, even the ones that aren't intended to crash because otherwise it could stall the whole test run, potentially, with a blocking pop-up dialog.
>>
>> & yeah, I'm onboard with the new/separate env variable name/functionality. Any chance there would be some way to disable this temporarily at runtime, though? So we could disable it during death tests (I think in the GTEST_DEATH_ macro, in the "DeathTest::EXECUTE_TEST" case in the switch there (in gtest-death-test-internal.h)) but not having to disable it for all unit test executions?
>
> I am very unfamiliar with gtest-death-test.cc, which additionally has some Windows related stuff for which I don't have a good way to test (I have no Windows machine).

Not sure the Windows stuff has too much to do with what would be needed for this work. If there was, say, a global variable/function to call to disable symbolization, that could be called from DeathTest::ReturnSentinel's constructor.

> I also think lit tests take most of the time, so I do not intend to address it in this patch.

It's pretty close - there are 530 "not --crash" RUN lines in the LLVM monorepo, and 606 EXPECT or ASSERT_DEATH tests.

I don't think it should be addressed in this patch - but if you're interested in the cost of symbolization on LLVM test execution, there's possibly more cost in unit tests (it'll depend on how big the stacks are - I guess the "not --crash" tests might crash deeper inside LLVM, but the gunit tests will have extra frames of gunit infrastructure calls, so it might be a toss up) than in "not --crash" tests.

>> Ah, I see I'm to blame for the existing DisableSymbolication flag & indeed in the initial review an env variable was speculated as another possible direction. So perhaps if you're feeling inclined, in a follow-up patch, you could remove the flag and migrate it to use the env variable. (I seem to have experimented with that direction at least a little here: https://reviews.llvm.org/D33804?id=101121#inline-296440 )
>
> Removing DisableSymbolication in a follow-up sounds good.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86170