8371458: [REDO] Remove exception handler stub code in C2 #28192

ruben-arm · 2025-11-07T11:07:40Z

The original fix JDK-8365047 was backed out by JDK-8371388, this is the REDO.

The C2 exception handler stub code is only a trampoline to the generated exception handler blob. This change removes the extra step on the way to the generated blob.

According to some comments in the source code, the exception handler stub code used to be patched upon deoptimization, however presumably these comments are outdated as the patching upon deoptimization happens for post-call NOPs only.

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 2 Reviewers)

Issue

JDK-8371458: [REDO] Remove exception handler stub code in C2 (Enhancement - P4)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)
Dean Long (@dean-long - Reviewer)

Contributors

Martin Doerr <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28192

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

Using diff file

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

Using Webrev

Link to Webrev Comment

The C2 exception handler stub code is only a trampoline to the generated exception handler blob. This change removes the extra step on the way to the generated blob. According to some comments in the source code, the exception handler stub code used to be patched upon deoptimization, however presumably these comments are outdated as the patching upon deoptimization happens for post-call NOPs only.

Change-Id: I596317ec6a364b341e4642636fa5cf08f87ed722

Change-Id: I9ef51b426d34e9b44a3891f6a45307232f900e5a

Change-Id: I5f5fd681ba699f96859180d984841dd6df8524b5

ruben-arm · 2025-11-07T11:07:59Z

/contributor add @TheRealMDoerr

bridgekeeper · 2025-11-07T11:08:56Z

👋 Welcome back ruben-arm! 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 · 2025-11-07T11:08:58Z

@ruben-arm 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:

8371458: [REDO] Remove exception handler stub code in C2

Co-authored-by: Martin Doerr <[email protected]>
Reviewed-by: mdoerr, dlong

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

2d815ac: 8349535: Refactor ./pkcs11/Provider/MultipleLogins.sh to java test
2b5cd14: 8282441: [LOOM] The debug agent should attempt to free vthread ThreadNodes
3a45e61: 8369129: Raster createPackedRaster methods specification clean up
... and 303 more: https://git.openjdk.org/jdk/compare/1922c4fd6f10e6eac121462d509d6990ae4f9acd...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr, @dean-long) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-11-07T11:09:55Z

@ruben-arm
Contributor Martin Doerr <[email protected]> successfully added.

openjdk · 2025-11-07T11:10:42Z

@ruben-arm The following labels will be automatically applied to this pull request:

hotspot
serviceability

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 · 2025-11-07T11:14:57Z

Webrevs

ruben-arm · 2025-11-07T11:16:20Z

Thanks @ruben-arm. Let's go with this clean back out for now - testing just came back clean. Once you have the REDO ready, I can submit some more sophisticated testing to make sure it addresses the issues we're seeing.

@TobiHartmann, I would appreciate it if you could initiate the extended testing for this PR.

TobiHartmann · 2025-11-07T12:00:23Z

Sure, I submitted testing.

vnkozlov · 2025-11-07T15:31:27Z

@ruben-arm what was the issue in the original changes?

ruben-arm · 2025-11-07T15:47:04Z

@vnkozlov, the NativePostCallNop::check

jdk/src/hotspot/cpu/x86/nativeInst_x86.hpp

Line 584 in e34a831

bool check() const { return int_at(0) == 0x841f0f; }

was reading 4 bytes from the perceived call site.

That works when the check happens for an actual call site, as the post-call NOP sequence is always longer than 4 bytes.
However, it fails in case the return address in the stack frame, during deoptimization, is patched to point to the deoptimization stub code entry point.

With the change, the distance between the entry point and end of code blob can now be just 2 bytes - consequently the 4-bytes read would read outside the code blob.
The proposed fix is to split the read in the post-call NOP check into two 2-byte reads. If first read+comparison doesn't confirm it might be a post-call NOP sequence (it never will for the deoptimization stub code entry point) the second read wouldn't happen.

I initially missed this issue - having incorrectly concluded that the jmp at the entry point will take 5 bytes in size instead of 2 bytes.

7bb4352 should address this issue by splitting the faulting read into two. It also adjusts the expected size of deoptimization handler stub code to 7 bytes in total instead of the incorrect estimate of 10 bytes.

TobiHartmann · 2025-11-10T10:13:20Z

Sure, I submitted testing.

All testing passed. I leave it to the original reviewers to re-review.

TheRealMDoerr

I haven't checked the deopt handler size on x86. Otherwise, it still looks good to me.

TheRealMDoerr · 2025-11-10T10:50:13Z

src/hotspot/cpu/x86/nativeInst_x86.hpp

  };

-  bool check() const { return int_at(0) == 0x841f0f; }
+  bool check() const { return short_at(0) == 0x1f0f && short_at(2) == 0x0084; }


Maybe a comment would be nice.

Thanks for the suggestion. Would the comments added at 20cc58a be suitable?

openjdk · 2025-11-10T10:52:16Z

⚠️ @ruben-arm the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout pr-8365047
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push

ruben-arm · 2025-11-10T14:25:29Z

Thanks, @TobiHartmann, for the extra tests, and apologies for missing this in the original PR.

@TheRealMDoerr, I will update the PR to add comments.

I am exploring a possibility to add a unit test for this issue, but I have not identified a deterministic way to make the deopt handler stub end at a page boundary.

TobiHartmann · 2025-11-13T06:20:44Z

/reviewers 2 reviewer

openjdk · 2025-11-13T06:21:10Z

@TobiHartmann
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 2 Reviewers).

ruben-arm · 2025-11-13T07:54:32Z

I am planning to update this PR today to include the comments.
However, I still have not identified a way to ensure the deopt handler stub ends at a page boundary in a unit test.

ruben-arm · 2025-11-17T12:10:45Z

However, I still have not identified a way to ensure the deopt handler stub ends at a page boundary in a unit test.

The latest update implements an alternative way to detect the failure early during testing - via the newly added assertion in the emit_deopt_handler.

@adinn, @dean-long, @TheRealMDoerr, would it be possible for you to review the latest version of the PR?

Is there any additional testing you would recommend to perform before this can be integrated?

TheRealMDoerr

I think assertions would be sufficient in C1 instead of guarantee. But, ok. I'll put it into our test queue.

TheRealMDoerr · 2025-11-18T10:05:48Z

Our tests haven't revealed any new issues related to this PR.

ruben-arm · 2025-11-18T14:15:58Z

Thank you, @TheRealMDoerr,

I think assertions would be sufficient in C1 instead of guarantee.

Sure, I will change these to assertions.

dean-long · 2025-11-19T04:18:18Z

src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp

+    // The two parts should be checked separately to prevent out of bounds access in case
+    // the return address points to the deopt handler stub code entry point which could be
+    // at the end of page.
+    first_check_size = 4


Suggested change

first_check_size = 4

first_check_size = instruction_size

dean-long · 2025-11-19T04:23:20Z

src/hotspot/cpu/s390/nativeInst_s390.hpp

+    // read. If the check would read beyond size of the instruction at the deopt handler stub
+    // code entry point, then it has to happen in two stages - to prevent out of bounds access
+    // in case the return address points to the entry point which could be at the end of page.
+    first_check_size = 0


Suggested change

first_check_size = 0

first_check_size = 0 // check is unimplemented

dean-long · 2025-11-19T04:28:04Z

Is there any additional testing you would recommend to perform before this can be integrated?

Oracle likes to make sure the final version passes in our CI. I got burned last time testing an earlier version and not the final version.

adinn · 2025-11-19T09:31:18Z

@ruben-arm I'm ok with this version assuming it passes tests.

ruben-arm · 2025-11-20T23:58:56Z

Thank you for the reviews, @dean-long, @adinn, @TheRealMDoerr.
@dean-long, I've updated the patch according to your suggestions - also applied the same to other nativeInst_* files except nativeInst_x86.hpp as the instruction size is not fixed in this case.

the final version passes in our CI

If this version of the patch looks suitable, would it be possible to start the CI testing please?

TheRealMDoerr

Still good.

dean-long

CI testing results are good.

ruben-arm · 2025-11-24T09:41:41Z

Thank you both. I am planning to submit the /integrate request in a few hours.

ruben-arm added 16 commits August 7, 2025 16:45

Address review comments

2d8012a

Ensure stub code is not adjacent to a call

1f6546e

Revert "Ensure stub code is not adjacent to a call"

7d1ece5

Offset the deoptimization handler entry point

4717824

Change-Id: I596317ec6a364b341e4642636fa5cf08f87ed722

The patch is contributed by @TheRealMDoerr

fe7db04

Address review comments

5107d02

Address review comments

18e36ec

Address review comments

6e24110

Merge from the main branch

60279c3

Test for deoptimization in virtual threads

e9c817c

Change-Id: I9ef51b426d34e9b44a3891f6a45307232f900e5a

Check for NOP and MOVK separately in NativePostCallNop

de9bec5

Address review comments and fix a mistype

462dbf6

Merge from the main branch

dc32139

Rename deoptHandlerOffsetField -> deoptHandlerEntryOffsetField

359c2f1

x86: Fix post-call NOP check access outside code blob

7bb4352

Change-Id: I5f5fd681ba699f96859180d984841dd6df8524b5

openjdk bot added serviceability [email protected] hotspot [email protected] labels Nov 7, 2025

openjdk bot added the rfr Pull request is ready for review label Nov 7, 2025

ruben-arm changed the title ~~8371458: [REDO] - Remove exception handler stub code in C2~~ 8371458: [REDO] Remove exception handler stub code in C2 Nov 7, 2025

TheRealMDoerr approved these changes Nov 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Nov 13, 2025

Add an assertion to detect out of bounds access in post-call NOP checks

20cc58a

TheRealMDoerr approved these changes Nov 17, 2025

View reviewed changes

Replace guarantee with assert in the C1 emit_deopt_handler

3a01437

dean-long reviewed Nov 19, 2025

View reviewed changes

TheRealMDoerr approved these changes Nov 19, 2025

View reviewed changes

Refine first_check_size definitions

00ea0e1

TheRealMDoerr approved these changes Nov 21, 2025

View reviewed changes

dean-long approved these changes Nov 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2025

	first_check_size = 0
	first_check_size = 0 // check is unimplemented

8371458: [REDO] Remove exception handler stub code in C2 #28192

Are you sure you want to change the base?

8371458: [REDO] Remove exception handler stub code in C2 #28192

Conversation

ruben-arm commented Nov 7, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

ruben-arm commented Nov 7, 2025

Uh oh!

bridgekeeper bot commented Nov 7, 2025

Uh oh!

openjdk bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 7, 2025

Uh oh!

openjdk bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

ruben-arm commented Nov 7, 2025

Uh oh!

TobiHartmann commented Nov 7, 2025

Uh oh!

vnkozlov commented Nov 7, 2025

Uh oh!

ruben-arm commented Nov 7, 2025

Uh oh!

TobiHartmann commented Nov 10, 2025

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

ruben-arm Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 10, 2025

Uh oh!

ruben-arm commented Nov 10, 2025

Uh oh!

TobiHartmann commented Nov 13, 2025

Uh oh!

openjdk bot commented Nov 13, 2025

Uh oh!

ruben-arm commented Nov 13, 2025

Uh oh!

ruben-arm commented Nov 17, 2025

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealMDoerr commented Nov 18, 2025

Uh oh!

ruben-arm commented Nov 18, 2025

Uh oh!

dean-long Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

dean-long Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

dean-long commented Nov 19, 2025

Uh oh!

adinn commented Nov 19, 2025

Uh oh!

ruben-arm commented Nov 20, 2025

Uh oh!

TheRealMDoerr left a comment

Choose a reason for hiding this comment

ruben-arm commented Nov 7, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 7, 2025 •

edited

Loading

openjdk bot commented Nov 7, 2025 •

edited

Loading

mlbridge bot commented Nov 7, 2025 •

edited

Loading