JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution #1757

tstuefe · 2020-12-13T11:07:42Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Magnus Ihse Bursie (@magicus - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1757/head:pull/1757
$ git checkout pull/1757

bridgekeeper · 2020-12-13T11:08:09Z

👋 Welcome back stuefe! 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 · 2020-12-13T11:09:01Z

@tstuefe The following labels will be automatically applied to this pull request:

build
hotspot-runtime

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

mlbridge · 2020-12-13T15:56:01Z

Webrevs

00: Full (1d744d1)

dholmes-ora

Hi Thomas,
Not an expert but this all seems quite reasonable to me.
Thanks,
David

openjdk · 2020-12-14T04:52:33Z

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8185734: [Windows] Structured Exception Catcher missing around gtest execution

Reviewed-by: dholmes, ihse

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

3ab1dfe: 8257828: SafeFetch may crash if invoked in non-JavaThreads
381021a: Merge
afc4441: 8258094: AIX build fails after 8257602
164c8a6: 8258092: Link to early access platform documentation in TestHtmlTableTags.java
43dc3f7: 8254350: CompletableFuture.get may swallow InterruptedException
6d79ec8: 8258111: Problemlist compiler/blackhole tests for -Xcomp until JDK-8258101 is fixed
2001da3: 8257596: Clarify trusted final fields for record classes
b1afed7: 8257919: [JVMCI] profiling info didn't change after reprofile
b7ac32d: 8257598: Clarify what component values are used in Record::equals
a280182: 8258060: Update @jls tags for renamed/renumbered sections
... and 12 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

tstuefe · 2020-12-14T05:52:03Z

Hi Thomas,
Not an expert but this all seems quite reasonable to me.
Thanks,
David

Thanks David!

magicus · 2020-12-15T08:16:51Z

src/hotspot/os/windows/os_windows.cpp

@@ -512,6 +512,7 @@ struct tm* os::gmtime_pd(const time_t* clock, struct tm* res) {
  return NULL;
 }

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

magicus

Build changes look good.

I left a note on the hotspot code; feel free to ignore it if you want. :)

I agree with your assessment that restoring gtest to the code base would make life simpler. I'm not sure what needs to be done to do that; I suppose it needs someone to write and implement a JBS feature, and to publicly acknowledge that they intent to spend the time needed to keep it up to date with upstream going forward.

(I also agree with your assessment on VEH replacing SEH, as my recollection of working with the runtime system on JRockit was that VEH was superior for the needs of a VM, and much closer to the posix signal model.)

tstuefe · 2020-12-15T08:58:37Z

Build changes look good.

I left a note on the hotspot code; feel free to ignore it if you want. :)

I agree with your assessment that restoring gtest to the code base would make life simpler. I'm not sure what needs to be done to do that; I suppose it needs someone to write and implement a JBS feature, and to publicly acknowledge that they intent to spend the time needed to keep it up to date with upstream going forward.

Yes, and unfortunately I cannot do this - I think this has to be driven from inside Oracle.

(I also agree with your assessment on VEH replacing SEH, as my recollection of working with the runtime system on JRockit was that VEH was superior for the needs of a VM, and much closer to the posix signal model.)

I also agree. @luhenry 's work convinced me, lets see when it bears fruit.

Cheers, Thomas

tstuefe · 2020-12-15T09:02:09Z

/integrate

openjdk · 2020-12-15T09:03:12Z

@tstuefe Since your change was applied there have been 22 commits pushed to the master branch:

3ab1dfe: 8257828: SafeFetch may crash if invoked in non-JavaThreads
381021a: Merge
afc4441: 8258094: AIX build fails after 8257602
164c8a6: 8258092: Link to early access platform documentation in TestHtmlTableTags.java
43dc3f7: 8254350: CompletableFuture.get may swallow InterruptedException
6d79ec8: 8258111: Problemlist compiler/blackhole tests for -Xcomp until JDK-8258101 is fixed
2001da3: 8257596: Clarify trusted final fields for record classes
b1afed7: 8257919: [JVMCI] profiling info didn't change after reprofile
b7ac32d: 8257598: Clarify what component values are used in Record::equals
a280182: 8258060: Update @jls tags for renamed/renumbered sections
... and 12 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...master

Your commit was automatically rebased without conflicts.

Pushed as commit 568dc29.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org build build-dev@openjdk.org labels Dec 13, 2020

Make SEH work in gtests

1d744d1

tstuefe force-pushed the JDK-8185734-SEH-in-gtests branch from 87d2fc6 to 1d744d1 Compare December 13, 2020 11:15

tstuefe marked this pull request as ready for review December 13, 2020 15:52

openjdk bot added the rfr Pull request is ready for review label Dec 13, 2020

dholmes-ora approved these changes Dec 14, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 14, 2020

magicus reviewed Dec 15, 2020

View reviewed changes

magicus approved these changes Dec 15, 2020

View reviewed changes

openjdk bot closed this Dec 15, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 15, 2020

tstuefe deleted the JDK-8185734-SEH-in-gtests branch January 15, 2021 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution #1757

JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution #1757

Uh oh!

tstuefe commented Dec 13, 2020 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Dec 13, 2020

Uh oh!

openjdk bot commented Dec 13, 2020

Uh oh!

mlbridge bot commented Dec 13, 2020

Uh oh!

dholmes-ora left a comment

Uh oh!

openjdk bot commented Dec 14, 2020 •

edited

Loading

Uh oh!

tstuefe commented Dec 14, 2020

Uh oh!

magicus Dec 15, 2020

Uh oh!

tstuefe Dec 15, 2020

Uh oh!

magicus left a comment

Uh oh!

tstuefe commented Dec 15, 2020

Uh oh!

tstuefe commented Dec 15, 2020

Uh oh!

openjdk bot commented Dec 15, 2020

Uh oh!

Uh oh!

JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution #1757

JDK-8185734: [Windows] Structured Exception Catcher missing around gtest execution #1757

Uh oh!

Conversation

tstuefe commented Dec 13, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Dec 13, 2020

Uh oh!

openjdk bot commented Dec 13, 2020

Uh oh!

mlbridge bot commented Dec 13, 2020

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe commented Dec 14, 2020

Uh oh!

magicus Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

tstuefe Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Dec 15, 2020

Uh oh!

tstuefe commented Dec 15, 2020

Uh oh!

openjdk bot commented Dec 15, 2020

Uh oh!

Uh oh!

tstuefe commented Dec 13, 2020 •

edited by openjdk bot

Loading

openjdk bot commented Dec 14, 2020 •

edited

Loading