8364664: gtest death tests failing on Windows

[swesonga]

0054bbe (from https://bugs.openjdk.org/browse/JDK-8343756) caused the gtest death tests to fail on Windows with the error message "Caught std::exception-derived exception escaping the death test statement. Exception message: unknown file: error: SEH exception with code 0xc0000005 thrown in the test body." The error message is from the catch block in https://github.com/google/googletest/blob/v1.14.0/googletest/include/gtest/internal/gtest-death-test-internal.h#L198-L212

In the failing death tests, the gtests start another process and expect the child process to be terminated by JVM error handling code. However, the structured exception handling code in the googletest code ends up getting executed instead. The death tests expect execution to continue after the instruction that triggered the exception by writing to the poissoned page. This change proposes build Windows gtests without structured exception handling to avoid changing the flow of exceptions in OpenJDK test code. The effect of this change is to stop using the SEH path in the HandleSehExceptionsInMethodIfSupported method and directly start the test.

All the Windows gtests now pass with this change.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Warning

 ⚠️ Found leading lowercase letter in issue title for 8364664: gtest death tests failing on Windows

Issue

• JDK-8364664: gtest death tests failing on Windows (Bug - P4)

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26661/head:pull/26661
$ git checkout pull/26661

Update a local copy of the PR:
$ git checkout pull/26661
$ git pull https://git.openjdk.org/jdk.git pull/26661/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26661

View PR using the GUI difftool:
$ git pr show -t 26661

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26661.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back swesonga! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@swesonga This change is no longer ready for integration - check the PR body for details.

[openjdk]

@swesonga The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (7ff47851)

[erikj79]

From a build/makefile point of view, this looks fine, but I would like input from someone more familiar with GTest as well.

/reviewers 2

[openjdk]

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[kimbarrett]

How is it that we (Oracle) don't see these gtest death test failures in our
CI? I'm guessing others (like SAP - @tstuefe ?) aren't either, since this
issue is newly reported while the causing change was made more than 8 months
ago.

The code in gtest-death-test-internal.h#L198-L212 referenced in the PR
description is conditionalized on GTEST_HAS_EXCEPTIONS, which leads me to
wonder why exceptions are enabled. If not for that, we wouldn't be in that
code.

That led me to wonder why, on Windows, we build libgtest and rebuild libjvm
with exceptions enabled, by using -EHsc instead of no -EH option as done for
the non-gtest libjvm?

https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L71
https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L101

That decision seems pretty old, like maybe from the initial introduction of
gtest. I haven't tracked down why, or whether the reasons are still valid. I
think it would be better to change that, assuming that's possible.

I was concerned that this might effectively undo JDK-8185734, since the
suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being true.
But it looks like the actual change didn't do that.

[swesonga]

The code in gtest-death-test-internal.h#L198-L212 referenced in the PR description is conditionalized on GTEST_HAS_EXCEPTIONS, which leads me to wonder why exceptions are enabled. If not for that, we wouldn't be in that code.

That led me to wonder why, on Windows, we build libgtest and rebuild libjvm with exceptions enabled, by using -EHsc instead of no -EH option as done for the non-gtest libjvm?

https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L71 https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L101

That decision seems pretty old, like maybe from the initial introduction of gtest. I haven't tracked down why, or whether the reasons are still valid. I think it would be better to change that, assuming that's possible.

This warning from the C++ compiler (after removing the 2 -EHsc lines) indicates that gtest code is using C++ exception handling:

c:\progra~1\mib055~1\2022\enterprise\vc\tools\msvc\14.44.35207\include\__msvc_ostream.hpp(781): error C2220: the following warning is treated as an error
c:\progra~1\mib055~1\2022\enterprise\vc\tools\msvc\14.44.35207\include\__msvc_ostream.hpp(781): warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc
c:\progra~1\mib055~1\2022\enterprise\vc\tools\msvc\14.44.35207\include\__msvc_ostream.hpp(781): note: the template instantiation context (the oldest one first) is
c:\repos\googletest\googletest\include\gtest/gtest-message.h(118): note: see reference to function template instantiation 'std::basic_ostream<char,std::char_traits<char>> &std::operator <<<std::char_traits<char>>(std::basic_ostream<char,std::char_traits<char>> &,const char *)' being compiled

[swesonga]

How is it that we (Oracle) don't see these gtest death test failures in our CI? I'm guessing others (like SAP - @tstuefe ?) aren't either, since this issue is newly reported while the causing change was made more than 8 months ago.

Should gtests be enabled in the pre-checkin GitHub actions to prevent regressions? I think they take a few minutes to execute.

[swesonga]

I was concerned that this might effectively undo JDK-8185734, since the suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being true. But it looks like the actual change didn't do that.

Thanks for pointing this out. Let me investigate.

[tstuefe]

How is it that we (Oracle) don't see these gtest death test failures in our CI? I'm guessing others (like SAP - @tstuefe ?) aren't either, since this issue is newly reported while the causing change was made more than 8 months ago.

The code in gtest-death-test-internal.h#L198-L212 referenced in the PR description is conditionalized on GTEST_HAS_EXCEPTIONS, which leads me to wonder why exceptions are enabled. If not for that, we wouldn't be in that code.

That led me to wonder why, on Windows, we build libgtest and rebuild libjvm with exceptions enabled, by using -EHsc instead of no -EH option as done for the non-gtest libjvm?

https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L71 https://github.com/openjdk/jdk/blame/f95af744b07a9ec87e2507b3d584cbcddc827bbd/make/hotspot/lib/CompileGtest.gmk#L101

That decision seems pretty old, like maybe from the initial introduction of gtest. I haven't tracked down why, or whether the reasons are still valid. I think it would be better to change that, assuming that's possible.

I was concerned that this might effectively undo JDK-8185734, since the suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being true. But it looks like the actual change didn't do that.

This all sounds very familiar; let me check.

[tstuefe]

So, we run gtests on Windows with --gtest_catch_exceptions=0 which disables the inbuilt exception handler.

See issues

• https://bugs.openjdk.org/browse/JDK-8185734
• https://bugs.openjdk.org/browse/JDK-8267138 (fixed an oversight in 8185734)

See more details in my PR description for 8185734, that's why this all sounded so familiar.

The mechanism in place since 8267138 should be enough for my understanding. @swesonga, can you find out why this is not sufficient in your case? Is this option not passed on to your test?

That led me to wonder why, on Windows, we build libgtest and rebuild libjvm with exceptions enabled, by using -EHsc instead of no -EH option as done for the non-gtest libjvm?

In the libjvm code itself, we don't use C++ exceptions, and we catch all Windows SEH with __try/__except. We also don't want standard stack unwinding with these signals. So no need for /EHxx. My assumption is that the gtest framework itself uses C++ exceptions and therefore needs /EHsc. The documentation is not super clear on some aspects of this (https://learn.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model), but it says that for standard C++ exceptions to work /EHsc is needed.

I was concerned that this might effectively undo JDK-8185734, since the suggestion in JBS was to conditionalize some code on GTEST_HAS_SEH being true. But it looks like the actual change didn't do that.

In my PR for 8184734, I wrote:

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

So it looks this was the first thing I tried back then, and it failed.

[tstuefe]

How is it that we (Oracle) don't see these gtest death test failures in our CI? I'm guessing others (like SAP - @tstuefe ?) aren't either, since this issue is newly reported while the causing change was made more than 8 months ago.

Should gtests be enabled in the pre-checkin GitHub actions to prevent regressions? I think they take a few minutes to execute.

They are already executed :-) https://github.com/swesonga/jdk/actions/runs/16735029827/job/47374721173

They are launched from the GtestWrapper.java with the --gtest_catch_exceptions=0 option. That's why it is so strange that this does not work for your case.

[magicus]

That led me to wonder why, on Windows, we build libgtest and rebuild libjvm with exceptions enabled, by using -EHsc instead of no -EH option as done for the non-gtest libjvm?

That decision seems pretty old, like maybe from the initial introduction of gtest. I haven't tracked down why, or whether the reasons are still valid. I think it would be better to change that, assuming that's possible.

Your assumption is correct. This is from the initial JDK-8148244. I don't know if there is any rationale, or if this was just added by following gtest documentation.

I guess it can be removed. I'm starting a test run of hs-tier1 without it right now, but I'm not sure if that is enough to provoke any potential problems.

[magicus]

Well that did not work:

 warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc

when compiling googletest-1.14.0/googlemock/src/gmock-spec-builders.cc.

I'm not sure really of the effect of including, or not including, /EHsc when building gtest. An alternative is to disable C4530; I can try that as well.

[magicus]

An alternative is to disable C4530; I can try that as well.

Well, that built and tested successfully at least. I've opened JDK-8365231, so we can continue further discussion about that change separate from this PR.