8360707: Globally enumerate all blobs, stubs and entries

[adinn]

Use the blob, stub and entry declarations to generate a single global enumeration for all blobs, likewise for all stubs and all entries. Modify stub generators in shared runtime, c1 runtime, c2 runtime and stub generator subsystems and their clients to use those enumerations consistently.

---

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-8360707: Globally enumerate all blobs, stubs and entries (Sub-task - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Fei Yang (@RealFYang - Reviewer) Review applies to 29f02865
• Ashutosh Mehra (@ashu-mehra - Committer) Review applies to 776cbe90

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26004

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

8360707: Globally enumerate all blobs, stubs and entries

Reviewed-by: kvn, fyang, asmehra

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

• 03526e2: 8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors
• 1de2ace: 8361529: GenShen: Fix bad assert in swap card tables
• fa32bfe: 8358529: GenShen: Heuristics do not respond to changes in SoftMaxHeapSize
• ... and 32 more: https://git.openjdk.org/jdk/compare/f3e0588d0b825a68a4ad61ddf806877f46da69dc...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]

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

• hotspot
• shenandoah

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]

Webrevs

• 07: Full - Incremental (a14c9821)
• 06: Full - Incremental (dbc8394a)
• 05: Full - Incremental (59479dce)
• 04: Full - Incremental (776cbe90)
• 03: Full - Incremental (6d4eee55)
• 02: Full - Incremental (e42ec698)
• 01: Full - Incremental (7908e895)
• 00: Full (29f02865)

[offamitkumar]

@adinn I got one test failure on s390: test/hotspot/jtreg/runtime/ErrorHandling/MachCodeFramesInErrorFile.java

java.lang.RuntimeException: 1 < 2
	at jdk.test.lib.Asserts.fail(Asserts.java:715)
	at MachCodeFramesInErrorFile.run(MachCodeFramesInErrorFile.java:170)
	at MachCodeFramesInErrorFile.main(MachCodeFramesInErrorFile.java:108)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
	at java.base/java.lang.Thread.run(Thread.java:1474)

I didn't hs_err even in full verbose. But attaching overall run in txt file:
26004_test_failure.txt

[adinn]

@offamitkumar Thanks for testing. I'm looking into the issue and will get back to you.

@TheRealMDoerr @offamitkumar @RealFYang Would you be able to check this on ppc/riscv to ensure it builds (cross-compile is ok) and passes (at least) tier1?

Thanks for whatever help you can provide.

[TheRealMDoerr]

@offamitkumar Thanks for testing. I'm looking into the issue and will get back to you.

@TheRealMDoerr @offamitkumar @RealFYang Would you be able to check this on ppc/riscv to ensure it builds (cross-compile is ok) and passes (at least) tier1?

Thanks for whatever help you can provide.

hotspot tier1 has passed on PPC64. Thanks for the ping!

[adinn]

@vnkozlov My fastdebug build has passed the following test suites on Linux/aarch64 and Linux/x86:

tier1
jtreg/test/hotspot/jtreg/runtime
jtreg/test/hotspot/jtreg/compiler

Could you review the PR and maybe run it through internal testing?

[adinn]

@TheRealMDoerr Thanks for testing!

[ashu-mehra]

static_cast can be avoided if _stub_id type is changed from int to StubId. And there is a similar static_cast in output.cpp as well.

[adinn]

I think this conversion to int was done to avoid a problem with include dependencies. It is certainly not an issue now that StubId is statically generated in a header already included in all affected files. I have pushed a fix to remove the int casts and use StubId instead.

[ashu-mehra]

Looks good to me. I have just a minor comment. Thanks for these changes. This should help in simplifying the AOTCodeAddressTable.

[vnkozlov]

@vnkozlov Could you please rereview this?

This looks fine. Please merge latest jdk so I can do additional testing with AOTStubCaching enabled.

[adinn]

@vnkozlov

This looks fine. Please merge latest jdk so I can do additional testing with AOTStubCaching enabled.

Done.

[offamitkumar]

Hi @adinn,

could you add these changes fixing test failure on s390:

diff --git a/src/hotspot/cpu/s390/stubGenerator_s390.cpp b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
index 10e5abe9ce8..687fb764bd8 100644
--- a/src/hotspot/cpu/s390/stubGenerator_s390.cpp
+++ b/src/hotspot/cpu/s390/stubGenerator_s390.cpp
@@ -164,15 +164,16 @@ class StubGenerator: public StubCodeGenerator {
 
       // Save non-volatile registers to ABI of caller frame.
       BLOCK_COMMENT("save registers, push frame {");
-      __ z_stmg(Z_R6, Z_R14, 16, Z_SP);
-      __ z_std(Z_F8, 96, Z_SP);
-      __ z_std(Z_F9, 104, Z_SP);
-      __ z_std(Z_F10, 112, Z_SP);
-      __ z_std(Z_F11, 120, Z_SP);
-      __ z_std(Z_F12, 128, Z_SP);
-      __ z_std(Z_F13, 136, Z_SP);
-      __ z_std(Z_F14, 144, Z_SP);
-      __ z_std(Z_F15, 152, Z_SP);
+      __ save_return_pc();
+      __ z_stmg(Z_R6, Z_R13, 16, Z_SP);
+      __ z_std(Z_F8, 88, Z_SP);
+      __ z_std(Z_F9, 96, Z_SP);
+      __ z_std(Z_F10, 104, Z_SP);
+      __ z_std(Z_F11, 112, Z_SP);
+      __ z_std(Z_F12, 120, Z_SP);
+      __ z_std(Z_F13, 128, Z_SP);
+      __ z_std(Z_F14, 136, Z_SP);
+      __ z_std(Z_F15, 144, Z_SP);
 
       //
       // Push ENTRY_FRAME including arguments:
@@ -337,15 +338,16 @@ class StubGenerator: public StubCodeGenerator {
       __ z_lg(r_arg_result_type, result_type_offset, r_entryframe_fp);
 
       // Restore non-volatiles.
-      __ z_lmg(Z_R6, Z_R14, 16, Z_SP);
-      __ z_ld(Z_F8, 96, Z_SP);
-      __ z_ld(Z_F9, 104, Z_SP);
-      __ z_ld(Z_F10, 112, Z_SP);
-      __ z_ld(Z_F11, 120, Z_SP);
-      __ z_ld(Z_F12, 128, Z_SP);
-      __ z_ld(Z_F13, 136, Z_SP);
-      __ z_ld(Z_F14, 144, Z_SP);
-      __ z_ld(Z_F15, 152, Z_SP);
+      __ restore_return_pc();
+      __ z_lmg(Z_R6, Z_R13, 16, Z_SP);
+      __ z_ld(Z_F8, 88, Z_SP);
+      __ z_ld(Z_F9, 96, Z_SP);
+      __ z_ld(Z_F10, 104, Z_SP);
+      __ z_ld(Z_F11, 112, Z_SP);
+      __ z_ld(Z_F12, 120, Z_SP);
+      __ z_ld(Z_F13, 128, Z_SP);
+      __ z_ld(Z_F14, 136, Z_SP);
+      __ z_ld(Z_F15, 144, Z_SP);
       BLOCK_COMMENT("} restore");
 
       //

[adinn]

@offamitkumar

could you add these changes fixing test failure on s390:

I think these need to be included in a separate PR. Your patch fixes a problem that is independent from the current change (it was already there but only manifested after this change).

Separating these issues is important because we may decide to backport your patch (say, to jdk25) and the changes in this PR definitely should not be backported. It also matters because it avoids confusing maintainers over the purpose and scope of these two sets of changes -- this one is meant to be a removal of technical debt with no visible changes to functionality, whereas your patch is remedying a functional defect.

So, please raise a new JIRA indicating what problem the above patch fixes and then create a new PR for the patch.

[vnkozlov]

Thank you @adinn for merge. Unfortunately I was not able to enable Stubs caching for testing.
It failed my local testing. runtime/cds/appcds/aotCode/AOTCodeCompressedOopsTest.java test failed on x86 for multiply reasons. Some are ease to fix and some may be not. I gave up after hitting relocation patching issue.

I submitted testing with AOTStubCaching disabled as in current code.

[vnkozlov]

I found the cause of my local testing failure. I built with ubsan and hit JDK-8360942 which I am working on. After applying the fix the test passed. Here other changes you may consider to include because they fix stack overflow stub generation:

diff --git a/src/hotspot/share/code/aotCodeCache.cpp b/src/hotspot/share/code/aotCodeCache.cpp
index 08cd05a64ed..4c080b90acc 100644
--- a/src/hotspot/share/code/aotCodeCache.cpp
+++ b/src/hotspot/share/code/aotCodeCache.cpp
@@ -1298,6 +1314,7 @@ void AOTCodeAddressTable::init_extrs() {
     SET_ADDRESS(_extrs, SharedRuntime::resolve_opt_virtual_call_C);
     SET_ADDRESS(_extrs, SharedRuntime::resolve_virtual_call_C);
     SET_ADDRESS(_extrs, SharedRuntime::resolve_static_call_C);
+    SET_ADDRESS(_extrs, SharedRuntime::throw_StackOverflowError);
     SET_ADDRESS(_extrs, SharedRuntime::throw_delayed_StackOverflowError);
     SET_ADDRESS(_extrs, SharedRuntime::throw_AbstractMethodError);
     SET_ADDRESS(_extrs, SharedRuntime::throw_IncompatibleClassChangeError);
diff --git a/src/hotspot/share/runtime/init.cpp b/src/hotspot/share/runtime/init.cpp
index b54afd9f735..37c13d90e04 100644
--- a/src/hotspot/share/runtime/init.cpp
+++ b/src/hotspot/share/runtime/init.cpp
@@ -147,9 +150,9 @@ jint init_globals() {
   AsyncLogWriter::initialize();

   initial_stubs_init();      // initial stub routines
+  AOTCodeCache::init_early_stubs_table();  // need this after initial_stubs and before generate_initial_stub
   // stack overflow exception blob is referenced by the interpreter
   SharedRuntime::generate_initial_stubs();
-  AOTCodeCache::init_early_stubs_table();  // need this after initial_stubs
   gc_barrier_stubs_init();   // depends on universe_init, must be before interpreter_init
   continuations_init();      // must precede continuation stub generation
   continuation_stubs_init(); // depends on continuations_init

[vnkozlov]

So far testing is fine. I see only failures caused by known AOT stubs caching bug: JDK-8357593

[adinn]

@vnkozlov I've pushed the two changes you provided (on the assumption that we are not going to need to backport them to jdk25).
Is there any need for more testing, assuming the latest push passes GHA?
Is there anything I can do to help with JDK-8357593?

[vnkozlov]

My testing passed without new failures

[adinn]

/integrate

[adinn]

@erikj79 This PR seems to be stuck at the integrate stage -- any ideas why?

[openjdk]

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

• 83feb7a: 8361599: [PPC64] enable missing tests via jtreg requires
• 963b83f: 8357689: Refactor JVMCI to enable replay compilation in Graal
• 68b27b8: 8361349: Fix visibility of CollectedHeap::stop() and ::print_tracing_info()
• ... and 44 more: https://git.openjdk.org/jdk/compare/f3e0588d0b825a68a4ad61ddf806877f46da69dc...master

Your commit was automatically rebased without conflicts.

[openjdk]

@adinn Pushed as commit b1fa1ec.

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

[adinn]

@erikj79 Dunno what you did but that was quick ;-)
Ta!

[dholmes-ora]

After this integration we are seeing crashes on Linux x64 and Aarch64:

assert(idx2 >= 0 && idx2 >= idx1) failed: bad blob ids first -1 and second -1

[adinn]

After this integration we are seeing crashes on Linux x64 and Aarch64:
assert(idx2 >= 0 && idx2 >= idx1) failed: bad blob ids first -1 and second -1

@dholmes-ora Can you provide more detail as to what is failing? Is this a specific build?

[adinn]

@dholmes-ora It's ok I have identified what is going wrong.

The problem appears to occur when either C1 or C2 is omitted from the build. I have raised JDK-8361844 and have a PR ready which I am checking against tier1 (with a build that omits C1).