Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8310228: Improve error reporting for uncaught native exceptions on Windows #14523

Closed
wants to merge 9 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Jun 16, 2023

Prevents a stack overflow (or other fatal error) from occurring when handling an unknown exception. See JBS issue for full problem outline.

WRT implementation:

  • I've refactored Handle_FLT_Exception to check the exception code on 64 bit Windows as well. It now returns a boolean indicating whether the exception was recognized and handled.
  • For x86, I've created a new Uncaugh_Exception_Handler function which implements the uncaught exception handler. This new function replaces the previous use of Handle_FLT_Exception as uncaught exception handler. x86 also supports exception chaining through a previous exception handler stored in prev_uef_handler. But, on 64 bit windows this field is never set, so we don't need to try to execute the fallback handler on 64 bit. Hence, it seemed clearer to factor out that bit to a separate function. Both the x86 and 64 bit impls call the handle_FLT_exception function.

I've tested this patch locally on Windows x64, I will need some help testing this on Windows x86 (it seems some of the test libraries are failing to build?)

Testing: tier 1-4


Progress

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

Issue

  • JDK-8310228: Improve error reporting for uncaught native exceptions on Windows (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14523

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 16, 2023

👋 Welcome back jvernee! 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
Copy link

openjdk bot commented Jun 16, 2023

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

  • hotspot-runtime

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.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jun 16, 2023
@JornVernee JornVernee changed the title 8310228: Improve error reporting for uncaught natve exceptions on Windows 8310228: Improve error reporting for uncaught native exceptions on Windows Jun 16, 2023
@JornVernee JornVernee marked this pull request as ready for review June 21, 2023 12:25
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 21, 2023
ctx->MxCsr = MxCsr;
return true;
}
#endif // !_WIN64
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff looks a bit mixed up, but I've essentially just moved this _WIN64 code into the switch which checks the exception code, so that we only try to restore MXCSR when we see a FLT* exception.

@mlbridge
Copy link

mlbridge bot commented Jun 21, 2023

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an area I'm familiar with so still mulling over it - it sounds reasonable. In the meantime a few comments below.

Thanks.

@tstuefe
Copy link
Member

tstuefe commented Jun 22, 2023

Hi Jorn,

why do we add code for x86 if Windows 32-bit is deprecated and earmarked for being removed? I rather remove the remaining x86 code sections (e.g. calling convention declarations) than add new complexity.

Cheers, Thomas

@JornVernee
Copy link
Member Author

Hi Jorn,

why do we add code for x86 if Windows 32-bit is deprecated and earmarked for being removed? I rather remove the remaining x86 code sections (e.g. calling convention declarations) than add new complexity.

Cheers, Thomas

I'm not sure what the schedule is for the removal of the Windows x86 port, but I thought that until it is actually removed, it should be kept working?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, this all seems reasonable to me.

@openjdk
Copy link

openjdk bot commented Jun 23, 2023

@JornVernee 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:

8310228: Improve error reporting for uncaught native exceptions on Windows

Reviewed-by: dholmes, djelinski

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 20 new commits pushed to the master branch:

  • 6a4b665: 8316659: assert(LockingMode != LM_LIGHTWEIGHT || flag == CCR0) failed: bad condition register
  • 913e43f: 8316582: Minor startup regression in 22-b15 due JDK-8310929
  • 23ed890: 6415065: Submenu is shown on wrong screen in multiple monitor environment
  • ca47f5f: 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
  • 1749ba2: 8311084: Add typeSymbol() API for applicable constant pool entries
  • 9f5d2b9: 8316285: Opensource JButton manual tests
  • a35e96a: 8313612: Use JUnit in lib-test/jdk tests
  • bee7524: 8315786: [AIX] Build Disk Local Detection Issue with GNU-utils df on AIX
  • ceff47b: 8315082: [REDO] Generational ZGC: Tests crash with assert(index == 0 || is_power_of_2(index))
  • df4a25b: 8308762: Metaspace leak with Instrumentation.retransform
  • ... and 10 more: https://git.openjdk.org/jdk/compare/c43ebd34afeab9ece9dee05f0da184a20e487a12...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 23, 2023
@JornVernee
Copy link
Member Author

JornVernee commented Jun 26, 2023

@gdams Could you help test these changes on Windows x86_32? (I'm seeing you're the one that deprecated that port as well, so I figured you might know :))

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 24, 2023

@JornVernee This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@JornVernee
Copy link
Member Author

Still under review

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 21, 2023

@JornVernee This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 19, 2023

@JornVernee This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Sep 19, 2023
@JornVernee
Copy link
Member Author

/open

@openjdk openjdk bot reopened this Sep 20, 2023
@openjdk
Copy link

openjdk bot commented Sep 20, 2023

@JornVernee This pull request is now open

Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

src/hotspot/os/windows/os_windows.cpp Outdated Show resolved Hide resolved
src/hotspot/os/windows/os_windows.cpp Outdated Show resolved Hide resolved
@JornVernee
Copy link
Member Author

JornVernee commented Sep 20, 2023

can you make this method static?

Done

FWIW, the Windows 32 bit build seems to be broken in mainline atm:

...\src\hotspot\share\runtime\flags\jvmFlagLimit.cpp(120): error C2220: the following warning is treated as an error
...\src\hotspot\share\runtime\flags\jvmFlagLimit.cpp(120): warning C4305: 'argument': truncation from 'const jlong' to 'T'
        with
        [
            T=double
        ]
...\src\hotspot\share\runtime\flags\jvmFlagLimit.cpp(143): warning C4305: 'argument': truncation from 'const jlong' to 'T'
        with
        [
            T=double
        ]

I suppose nobody is building this anymore...

I'm still apprehensive about ripping out the 32-bit code as part of this PR. So, I'll leave it as it is. It can be removed together with the rest of the port in the future.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still good, though not certain about the utility (and stability) of using createTestJVM in this case. I guess we will see.

Thanks

Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thanks!

@JornVernee
Copy link
Member Author

not certain about the utility (and stability) of using createTestJVM in this case

Yeah. I saw the effort going on to use this everywhere, and I figured I'd be a good citizen. I suppose it could be useful to test certain stress flags on the forked VM too.

@JornVernee
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 21, 2023

Going to push as commit 38bf119.
Since your change was applied there have been 24 commits pushed to the master branch:

  • 349723c: 8315739: Missing null check in os::vm_min_address
  • 8cbe42b: 8316421: libjava should load shell32.dll eagerly
  • 378bcd5: 8316595: Alpine build fails after JDK-8314021
  • b3d75fe: 8310874: Runthese30m crashes with klass should be in the placeholders during verification
  • 6a4b665: 8316659: assert(LockingMode != LM_LIGHTWEIGHT || flag == CCR0) failed: bad condition register
  • 913e43f: 8316582: Minor startup regression in 22-b15 due JDK-8310929
  • 23ed890: 6415065: Submenu is shown on wrong screen in multiple monitor environment
  • ca47f5f: 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
  • 1749ba2: 8311084: Add typeSymbol() API for applicable constant pool entries
  • 9f5d2b9: 8316285: Opensource JButton manual tests
  • ... and 14 more: https://git.openjdk.org/jdk/compare/c43ebd34afeab9ece9dee05f0da184a20e487a12...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 21, 2023
@openjdk openjdk bot closed this Sep 21, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2023
@openjdk
Copy link

openjdk bot commented Sep 21, 2023

@JornVernee Pushed as commit 38bf119.

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

@JornVernee JornVernee deleted the UncaughtExWin branch September 21, 2023 13:57
@Rigner
Copy link

Rigner commented May 6, 2024

Hey,

Following the discussion in openjdk/jdk21u-dev#510, I'm still able to reproduce this issue on Java 22+.

I made a simple PoC at https://github.com/Rigner/jdk-21-crash, which would help you reproducing and investigating the issue. Everything is explained in the README, with example outputs, build & run commands and my personal investigation.

Hope that helps 🙏

@JornVernee
Copy link
Member Author

JornVernee commented May 21, 2024

Hey,

Following the discussion in openjdk/jdk21u-dev#510, I'm still able to reproduce this issue on Java 22+.

I made a simple PoC at https://github.com/Rigner/jdk-21-crash, which would help you reproducing and investigating the issue. Everything is explained in the README, with example outputs, build & run commands and my personal investigation.

Hope that helps 🙏

I had a quick look, but the issue you're seeing is unrelated to this PR. If you're still experiencing the issue, I suggest submitting a bug report to https://bugreport.java.com/bugreport/ or the bug tracker for your JDK vendor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
5 participants