8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime #20566

adinn · 2024-08-13T11:35:59Z

Store the throw_exception and jfr stub code as blobs in class SharedRuntime, move the generation code to the the arch-specific generator classes and update client code to access them from their new location.

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-8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime (Sub-task - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) 🔄 Re-review required (review applies to d0ba9688)
Fei Yang (@RealFYang - Reviewer)
Yudi Zheng (@mur47x111 - Committer) 🔄 Re-review required (review applies to d0ba9688)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20566

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

Using diff file

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

Webrev

Link to Webrev Comment

… SharedRuntime

bridgekeeper · 2024-08-13T11:36:30Z

👋 Welcome back adinn! 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 · 2024-08-13T11:37:48Z

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

8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime

Reviewed-by: fyang, kvn, yzheng

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

15b20cb: 8337886: java/awt/Frame/MaximizeUndecoratedTest.java fails in OEL due to a slight color difference
56a007d: 8338488: Add screen capture for failure case
2f7ba78: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2024-08-13T11:38:22Z

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

graal
hotspot

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 · 2024-08-13T11:42:25Z

Webrevs

adinn · 2024-08-13T12:38:07Z

/label runtime

openjdk · 2024-08-13T12:39:09Z

@adinn
The label runtime is not a valid label.
These labels are valid:

graal
serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

adinn · 2024-08-13T14:43:21Z

/label hotspot-runtime

openjdk · 2024-08-13T14:43:39Z

@adinn
The hotspot-runtime label was successfully added.

vnkozlov

Please, add description to PR

vnkozlov · 2024-08-13T19:52:05Z

src/hotspot/cpu/x86/stubGenerator_x86_32.cpp

-    // Build this early so it's available for the interpreter
-    StubRoutines::_throw_StackOverflowError_entry          = generate_throw_exception("StackOverflowError throw_exception",
-                                                                                      CAST_FROM_FN_PTR(address, SharedRuntime::throw_StackOverflowError));
-    StubRoutines::_throw_delayed_StackOverflowError_entry  = generate_throw_exception("delayed StackOverflowError throw_exception",
-                                                                                      CAST_FROM_FN_PTR(address, SharedRuntime::throw_delayed_StackOverflowError));
+      CAST_FROM_FN_PTR(address, SharedRuntime::throw_delayed_StackOverflowError));


This does not seems correct. And I see linux-x86 (32 bits) corresponding build failure in GHA.

Sorry, that was an accidental paste while editing the code. Should be fixed now.

RealFYang · 2024-08-14T06:58:09Z

Hi Andrew, I find that we need following add-on change for riscv:

diff --git a/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp b/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
index dc89e489b24..bed24e442e8 100644
--- a/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
+++ b/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp
@@ -66,6 +66,12 @@

 #define __ masm->

+#ifdef PRODUCT
+#define BLOCK_COMMENT(str) /* nothing */
+#else
+#define BLOCK_COMMENT(str) __ block_comment(str)
+#endif
+
 const int StackAlignmentInSlots = StackAlignmentInBytes / VMRegImpl::stack_slot_size;

 class RegisterSaver {
@@ -2742,7 +2748,7 @@ static void jfr_epilogue(MacroAssembler* masm) {
 // For c2: c_rarg0 is junk, call to runtime to write a checkpoint.
 // It returns a jobject handle to the event writer.
 // The handle is dereferenced and the return value is the event writer oop.
-static RuntimeStub* SharedRuntime::generate_jfr_write_checkpoint() {
+RuntimeStub* SharedRuntime::generate_jfr_write_checkpoint() {
   enum layout {
     fp_off,
     fp_off2,
@@ -2780,7 +2786,7 @@ static RuntimeStub* SharedRuntime::generate_jfr_write_checkpoint() {
 }

 // For c2: call to return a leased buffer.
-static RuntimeStub* SharedRuntime::generate_jfr_return_lease() {
+RuntimeStub* SharedRuntime::generate_jfr_return_lease() {
   enum layout {
     fp_off,
     fp_off2,

adinn · 2024-08-14T09:19:24Z

Hi @RealFYang

Hi Andrew, I find that we need following add-on change for riscv:

Thanks for checking. Should be fixed by the latest push.

mur47x111 · 2024-08-14T10:00:19Z

src/hotspot/share/jvmci/vmStructs_jvmci.cpp

                                                                                                                                     \
  static_field(StubRoutines,                _verify_oop_count,                                jint)                                  \
                                                                                                                                     \
-  static_field(StubRoutines,                _throw_delayed_StackOverflowError_entry,          address)                               \


Please add the following symbol exporting

diff --git a/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp b/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp index 8fdb96a3038..0e96cea6596 100644 --- a/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp +++ b/src/hotspot/share/jvmci/jvmciCompilerToVM.hpp @@ -50,6 +50,7 @@ class CompilerToVM { static address SharedRuntime_deopt_blob_unpack_with_exception_in_tls; static address SharedRuntime_deopt_blob_uncommon_trap; static address SharedRuntime_polling_page_return_handler; + static address SharedRuntime_throw_delayed_StackOverflowError_blob; static address nmethod_entry_barrier; static int thread_disarmed_guard_value_offset; diff --git a/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp b/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp index 2116133e56e..27031bf55fe 100644 --- a/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp +++ b/src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp @@ -68,6 +68,7 @@ address CompilerToVM::Data::SharedRuntime_deopt_blob_unpack; address CompilerToVM::Data::SharedRuntime_deopt_blob_unpack_with_exception_in_tls; address CompilerToVM::Data::SharedRuntime_deopt_blob_uncommon_trap; address CompilerToVM::Data::SharedRuntime_polling_page_return_handler; +address CompilerToVM::Data::SharedRuntime_throw_delayed_StackOverflowError_blob; address CompilerToVM::Data::nmethod_entry_barrier; int CompilerToVM::Data::thread_disarmed_guard_value_offset; @@ -158,6 +159,7 @@ void CompilerToVM::Data::initialize(JVMCI_TRAPS) { SharedRuntime_deopt_blob_unpack_with_exception_in_tls = SharedRuntime::deopt_blob()->unpack_with_exception_in_tls(); SharedRuntime_deopt_blob_uncommon_trap = SharedRuntime::deopt_blob()->uncommon_trap(); SharedRuntime_polling_page_return_handler = SharedRuntime::polling_page_return_handler_blob()->entry_point(); + SharedRuntime_throw_delayed_StackOverflowError_blob = SharedRuntime::throw_delayed_StackOverflowError_entry(); BarrierSetNMethod* bs_nm = BarrierSet::barrier_set()->barrier_set_nmethod(); if (bs_nm != nullptr) { diff --git a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp index 130f1032e65..e0c56ccabee 100644 --- a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp +++ b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp @@ -68,6 +68,8 @@ static_field(CompilerToVM::Data, SharedRuntime_deopt_blob_uncommon_trap, address) \ static_field(CompilerToVM::Data, SharedRuntime_polling_page_return_handler, \ address) \ + static_field(CompilerToVM::Data, SharedRuntime_throw_delayed_StackOverflowError_blob, \ + address) \ \ static_field(CompilerToVM::Data, nmethod_entry_barrier, address) \ static_field(CompilerToVM::Data, thread_disarmed_guard_value_offset, int) \

Hi @mur47x111. Thanks for looking at this PR.

I deleted the static field declaration from vmStructs_jvmci.cpp because I found no other mention of throw_delayed_StackOverflowError_entry under src/hotspot/share/jvmci. So, I assumed it was not being used by any JVMCI clients.

I am happy to push a patch with your proposed changes. However, I just wanted to check whether you are sure you want to use the name SharedRuntime_throw_delayed_StackOverflowError_blob in the new declarations of the CompilerToVM field.

I am asking because I when I moved the exception stubs from StubGenerator to SharedRuntime I also made a change in the way the stubs are named, stored and accessed.

The original field StubGenerator::SharedRuntime_throw_delayed_StackOverflowError_entry stored the entry point for the throw routine and was of type address. The getter method that returns the entry address simply returns the field value.

The relocated field SharedRuntime::_throw_delayed_StackOverflowError_blob stores a pointer to a RuntimeBlob, the one that contains the throw routine. That is why the field name ends with _blob not _entry. The entry address is not stored directly in class SharedRuntime. In the new code the getter method computes the address by calling the blob's entry_point() method.

So, your patch is seems to be mixing up things by using the name SharedRuntime_throw_delayed_StackOverflowError_blob for the CompilerToVM field but declaring it with type address. Would it make more sense to use the name SharedRuntime_throw_delayed_StackOverflowError_entry for this field?

Right, SharedRuntime_throw_delayed_StackOverflowError_entry is better. Thanks for the explanation!

Ok, I pushed a version of your patch modified to use that name.

vnkozlov

Update is good.

RealFYang

LGTM. Thanks.

mur47x111

JVMCI changes look good to me. Thanks!

adinn · 2024-08-17T15:45:24Z

@vnkozlov @RealFYang @mur47x111
Thank you for the reviews!

adinn · 2024-08-19T08:29:45Z

@vnkozlov @RealFYang Could one of you please re-review. I had to merge with master to allow for a one-line include file change in sharedRuntime.cpp.

adinn · 2024-08-19T08:58:53Z

@RealFYang Thanks!

adinn · 2024-08-19T08:59:20Z

/integrate

openjdk · 2024-08-19T09:00:19Z

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

15b20cb: 8337886: java/awt/Frame/MaximizeUndecoratedTest.java fails in OEL due to a slight color difference
56a007d: 8338488: Add screen capture for failure case
2f7ba78: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-19T09:00:27Z

@adinn Pushed as commit f0374a0.

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

shqking · 2024-09-20T14:22:12Z

Hi @adinn , I encountered Client build failure on AArch64 after this commit. Could you help take a look at it when you have spare time? Thanks.

Here shows the configuration

====================================================
The existing configuration has been successfully updated in
/tmp/test123/build-release
using configure arguments '--with-debug-level=release --with-version-opt=git-fe80618bf3f --with-jvm-variants=client'.

Configuration summary:
* Name:           /tmp/test123/build-release
* Debug level:    release
* HS debug level: product
* JVM variants:   client
* JVM features:   client: 'cds compiler1 epsilongc g1gc jfr jni-check jvmti management parallelgc serialgc services shenandoahgc vm-structs zgc' 
* OpenJDK target: OS: linux, CPU architecture: aarch64, address length: 64
* Version string: 24-internal-git-fe80618bf3f (24-internal)
* Source date:    1726841146 (2024-09-20T14:05:46Z)

Tools summary:
* Boot JDK:       openjdk version "22.0.2" 2024-07-16 OpenJDK Runtime Environment (build 22.0.2+9-70) OpenJDK 64-Bit Server VM (build 22.0.2+9-70, mixed mode, sharing) (at /usr/lib/jvm/jdk-22.0.2)
* Toolchain:      gcc (GNU Compiler Collection)
* C Compiler:     Version 13.2.0 (at /usr/bin/gcc)
* C++ Compiler:   Version 13.2.0 (at /usr/bin/g++)

Build performance summary:
* Build jobs:     72
* Memory limit:   587068 MB

And here shows the error msg:

=== Output from failing command(s) repeated here ===
* For target hotspot_variant-client_libjvm_objs_sharedRuntime_aarch64.o:
/tmp/test123/jdk-src/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp: In static member function ‘static RuntimeStub* SharedRuntime::generate_throw_exception(const char*, address)’:
/tmp/test123/jdk-src/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:2809:3: error: ‘TraceTime’ was not declared in this scope; did you mean ‘traceid’?
 2809 |   TraceTime timer(timer_msg, TRACETIME_LOG(Info, startuptime));
      |   ^~~~~~~~~
      |   traceid

* All command lines available in /tmp/test123/build-release/make-support/failure-logs.
=== End of repeated output ===

adinn · 2024-09-24T09:50:53Z

@shqking Thanks for reporting this problem. I reproduced the same failure on aarch64 and have noticed that the problem also arises on arm. File sharedRuntime_aarch64.cpp needs to include the header that declares class TraceTime as does sharedRuntime_arm.cpp.

I raised JDK-8340793 and will push a patch to fix it.

8337987: Relocate jfr and throw_exception stubs from StubGenerator to…

7f9f0c4

… SharedRuntime

adinn marked this pull request as ready for review August 13, 2024 11:36

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review labels Aug 13, 2024

adinn marked this pull request as draft August 13, 2024 12:38

openjdk bot removed the rfr Pull request is ready for review label Aug 13, 2024

fix issues with ports

419c2e3

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 13, 2024

adinn added 4 commits August 13, 2024 16:25

fix some header includes and defintions

9614be2

fix throw exception stub generation on zero

a390a06

fix frame layouts for x86_32

58c4234

typo

0384e58

adinn marked this pull request as ready for review August 13, 2024 17:26

adinn marked this pull request as draft August 13, 2024 17:26

copy macro across

ea4ec0c

adinn force-pushed the JDK-8337987 branch from 75ab3c4 to ea4ec0c Compare August 13, 2024 17:29

adinn marked this pull request as ready for review August 13, 2024 17:30

openjdk bot added the rfr Pull request is ready for review label Aug 13, 2024

vnkozlov reviewed Aug 13, 2024

View reviewed changes

fix accidental paste

f12aea0

fix riscv port issues

1648205

mur47x111 reviewed Aug 14, 2024

View reviewed changes

graalvmbot mentioned this pull request Aug 14, 2024

[GR-57271] Adapt 8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime oracle/graal#9500

Closed

fix up jvmci static field declarations

d0ba968

vnkozlov approved these changes Aug 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 15, 2024

RealFYang approved these changes Aug 16, 2024

View reviewed changes

mur47x111 approved these changes Aug 16, 2024

View reviewed changes

merge

6307695

openjdk bot removed the ready Pull request is ready to be integrated label Aug 17, 2024

RealFYang approved these changes Aug 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 19, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 19, 2024

openjdk bot closed this Aug 19, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 19, 2024

adinn deleted the JDK-8337987 branch August 19, 2024 09:01

graalvmbot mentioned this pull request Aug 23, 2024

[GR-57299] Update labsjdk to 24+12-jvmci-b01 oracle/graal#9555

Closed

adinn mentioned this pull request Sep 24, 2024

8340793: Fix client builds after JDK-8337987 #21153

Closed

3 tasks

8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime #20566

8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime #20566

Uh oh!

Conversation

adinn commented Aug 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 13, 2024

Uh oh!

openjdk bot commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 13, 2024

Uh oh!

mlbridge bot commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

adinn commented Aug 13, 2024

Uh oh!

openjdk bot commented Aug 13, 2024

Uh oh!

adinn commented Aug 13, 2024

Uh oh!

openjdk bot commented Aug 13, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

adinn Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

RealFYang commented Aug 14, 2024

Uh oh!

adinn commented Aug 14, 2024

Uh oh!

mur47x111 Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

adinn Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

mur47x111 Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

adinn Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

RealFYang left a comment

Choose a reason for hiding this comment

Uh oh!

mur47x111 left a comment

Choose a reason for hiding this comment

Uh oh!

adinn commented Aug 17, 2024

Uh oh!

adinn commented Aug 19, 2024

Uh oh!

adinn commented Aug 19, 2024

Uh oh!

adinn commented Aug 19, 2024

Uh oh!

openjdk bot commented Aug 19, 2024

Uh oh!

openjdk bot commented Aug 19, 2024

Uh oh!

shqking commented Sep 20, 2024

Uh oh!

adinn commented Sep 24, 2024

adinn commented Aug 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 13, 2024 •

edited

Loading

mlbridge bot commented Aug 13, 2024 •

edited

Loading