On Tue, 15 Dec 2020 08:16:51 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> Hi, >> >> May I have reviews please for the following patch. >> >> At the moment, if a crash happens on Windows in gtests, the gtest SEH >> handler may be invoked instead of our error handler, and we just see a >> one-line-warning "SEH happened blabala". No hs-err file. >> >> Even worse, if a crash happens inside the VM as part of the gtests, if our >> SEH handler does not get involved, this may interfere with VM functionality >> - e.g. SafeFetch. >> >> Whether or not our SEH handler gets involved currently depends on arbitrary >> factors: >> - whether the fault happens in a VM or Java thread - which have a >> __try/__except around their start function - or whether the fault happens >> directly in the thread running the test >> - Faults in generated code are not handled on x86 but are okay x64 (where a >> SEH handler is registered for the code cache region) or on aarch64 (which >> uses VEH). >> >> This patch consists of two parts >> >> A) It surrounds the gtestlauncher main function with a SEH catcher. For that >> to work I also need to export the SEH handler from the hotspot. Note: It is >> difficult to place the SEH catcher: SEH is mutually exclusive with C++ >> exceptions, and since googletest uses C++ exceptions to communicate test >> conditions, the only place to put those __try/__except is really up here, at >> the entry of the gtestlauncher main() function. >> >> B) This is unfortunately not sufficient since googletest uses its own SEH >> catcher to wrap each test (see gtest.cc). Since that catcher sits below our >> catcher on the stack, it superimposes ours. >> >> In JBS, @kimbarrett suggested to build gtests with GTEST_HAS_SEH switched >> off to prevent gtest from using SEH. Unfortunately that won't work since the >> use of death tests means we need SEH. If we switch GTEST_HAS_SEH off, the >> death tests don't build. I also do not like this suggestion since this >> configuration may have a higher chance of bitrotting upstream. >> >> The solution I found is to switch off exception catching from user code as >> described in [3] using `--gtest_catch_exceptions=0` or the environment >> variable `GTEST_CATCH_EXCEPTIONS=0`. Since we do not use C++ exceptions in >> the hotspot, this is fine. >> >> The only drawback is that this cannot be done from within the gtestlauncher >> itself. Setting the environment variable in the main function - or even >> during dynamic initialization - does not work because the gtestlauncher >> itself parses all arguments as part of dynamic initialization. So I did the >> next best thing and specified `--gtest_catch_exceptions=0` at the places >> where we run the gtests. This is not perfect, but better than nothing. >> >> Testing: manually on Windows x64, x86, GH actions (Linux errors seem >> unrelated to this patch). >> >> ---- >> >> Notes: >> - If we owned the googletest code - forked it off like we did before - (B) >> would be very simple to solve by changing the default for >> `gtest_catch_exceptions` to 1. I still believe maintaining a fork of >> googletest would have many benefits. >> >> - Using VEH would have saved us from using __try/__except here, so we would >> have not needed (A). ATM we use VEH on aarch, SEH on x64+x32. Uniformly >> switching to VEH has been discussed several times in the past, the last >> attempt has been by @luhenry (see [1], [2]), but this has not yet >> materialized. >> >> [1] >> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040228.html >> [2] >> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/040967.html >> [3] >> https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#disabling-catching-test-thrown-exceptions > > src/hotspot/os/windows/os_windows.cpp line 515: > >> 513: } >> 514: >> 515: JNIEXPORT > > I understand if you do not want to start a code cleanup, but it looks like > this should be in an external .hpp file, which could then have been included > by all the .cpp files, instead of having the declaration repeated. I agree, but I would like to leave this out for now. I think at some point we may switch to Vectored Exception Handling for all Windows architectures, then we can remove this hideous coding in the gtest launcher main function again. And remove the export. ------------- PR: https://git.openjdk.java.net/jdk/pull/1757
