8372617: Save and restore stubgen stubs when using an AOT code cache #28433

adinn · 2025-11-20T14:59:02Z

This PR adds save and restore of all generated stubs to the AOT code cache on x86 and aarch64. Other arches are modified to deal with the related generic PAI changes.

Small changes were required to the aarch64 and x86_64 generator code in order to meet two key constraints:

the first declared entry of every stub starts at the first instruction in the stub code range
all data/code cross-references from one stub to another target a declared stub entry

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-8372617: Save and restore stubgen stubs when using an AOT code cache (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28433

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-20T15:00:51Z

👋 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 · 2025-11-20T15:01:39Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-11-20T15:02:21Z

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

hotspot

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.

adinn · 2025-11-20T15:10:41Z

This PR still needs to ensure that pre-universe stub entries are registered in the AOT address table. These stubs cannot be saved and restored because the AOT cache does not exist before universe init. As a consequence their addresses cannot be registered using the normal generate time mechanism. The required fix will be to add them via special case handling as soon as the AOT cache and address table have been initialised (preferably at the point where the majority of external addresses are registered).

This omission is not a problem for this patch i.e. as far as reference from existing stubs is concerned since none of the saved stubs targets to a pre-universe stub entry. However, they ought to be registered in case that situation changes or in case an nmethod that gets saved to and reloaded from the AOT cache needs at some point to target a pre-universe stub.

vnkozlov · 2025-11-21T00:06:22Z

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

+    int entry_count = StubInfo::entry_count(stub_id);
+    assert(entry_count == 1, "sanity check");
+    if (find_archive_data(stub_id)) {
+      address start = nullptr;
+      address end = nullptr;
+      load_archive_data(stub_id, start, end);
+      return start;
+    }
+


This code pattern repeats a lot. Can we move it into load_archive_data()?

address start = load_archive_data(stub_id); if (start != nullptr) { return start; }

And have an other specialized load_archive_data if you need end value.

Yes, I think I can probably refactor this to make it simpler and avoid returning end. I'll push a revised version soon. Return of end is mostly a hangover from an older version but it does still serve a purpose (see below). Originally, it was not clear what all the other specialized cases were going to be but I thinkit is now fairly clear that we only need to cater for:

multiple entries other than the start address

multiple extra addresses

both the above.

n.b. entries and extras are distinguished by one simple criterion: the former can be accessed from AOT code while the latter cannot. So, entries need to be registered with the AOT address table beneath load_entry while extras can be omitted (although they almost certainly need to be registered with the current runtime by the caller). That distinction ought to be written down in a comment somewhere, also the acceptable values for both:

start clearly must be non-null. Additional entries may be nullptr or in the range [start, end). Extra addresses may be nullptr or in the range [start, end] i.e. extras can address the first byte after the end of the stub.

This is needed so that an extra can be used to mark the (exclusive) end address of a range that covers the tail of a stub. So far, it has always been the case that extra addresses occur in triples identifying UnsafeMemoryAccess handler regions, (open, close, handler). I'm leaving it open as to whether any other uses might turn up which is why processing of extras happens in the caller rather than under load_entry.

load_entry now knows and verifies how many entries it ought to be retrieving and accepts an extras array if and only if there are extra addresses in the archive. It also range checks the entry/extra addresses. So, all a caller needs to do is assign extra entries to the outputs passed by the caller then validate the extras count and process them appropriate to whatever the stub understands them to mean.

end was originally being returned by load_entry to allow the caller to range check both entry and extra addresses but that check is now redundant. It is still needed because of an unnecessary complication in the current code. Addresses are converted to offsets before saving in the archive and translated back to addresses when loading. store_entry does ensure that all entry or extra offsets addresses lie within the range [start, end) and [start, end] respectively. However, nullptr and end both get saved to the archive as start - end. That means both of them get restored as nullptr. Clients are expected to know whether a restored nullptr at some offset in the extras array is really an end address and substitute end in its place (e.g. in the case where close == nullptr really means close == end). Because of that load_entry has to make end visible to the caller. If I change things so that nullptr is saved as UINT_MAX in that will avoid the ambiguity, allowing the client to use the value without an extra check.

@vnkozlov ^^

vnkozlov · 2025-11-21T00:16:16Z

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

+void StubGenerator_AOTAddressTable_init() {
+  ResourceMark rm;
+  GrowableArray<address> external_addresses;
+  // publish static addresses referred to by aarch64 generator
+  StubGenerator::init_AOTAddressTable(external_addresses);
+  // publish external data addresses defined in nested aarch64 class
+  StubRoutines::aarch64::init_AOTAddressTable(external_addresses);
+  AOTCodeCache::publish_external_addresses(external_addresses);
+}


This and *init_AOTAddressTable() methods should be under #if INCLUDE_CDS

vnkozlov · 2025-11-21T00:23:00Z

src/hotspot/share/runtime/stubRoutines.cpp

+
+// Non-generated init method
+
+void StubRoutines::init_AOTAddressTable() {


Can this body be in platform specific file.

Yes, I'll add an implementation to file stubRoutines_xxx.cpp for each platform.

vnkozlov · 2025-11-21T00:34:14Z

I noticed that we have to add a lot of local data tables addresses for stubs (math intrinsics).
May be we should consider to have platform specific AOT address tables for such address (and platform specific stubs). To avoid search in one big table.

src/hotspot/cpu/aarch64/stubDeclarations_aarch64.hpp

…llptr

openjdk · 2025-11-26T10:13:04Z

@adinn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout save-stubgen-stubs
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mlbridge · 2025-11-28T10:41:08Z

Webrevs

00: Full (0597bc6b)

ashu-mehra

Looks good. I only have minor comments, some typos and unused code.

ashu-mehra · 2025-12-02T16:43:19Z

src/hotspot/share/runtime/stubCodeGenerator.cpp

+// handler pc may be null in which case it defaults to the
+// default_handler.
+
+void StubCodeGenerator::register_unsafe_access_handlers(GrowableArray<address> &entries, int begin, int count) {


Is there a case or is it possible that begin is non -zero? I think it is same to assume we need to register all the values in entries in the UnsafeMemoryAccess::_table, right?

ashu-mehra · 2025-12-02T16:43:26Z

src/hotspot/share/code/aotCodeCache.hpp

-  static void init_early_stubs_table() NOT_CDS_RETURN;
-  static void init_shared_blobs_table() NOT_CDS_RETURN;
-  static void init_early_c1_table() NOT_CDS_RETURN;
+  static void set_shared_stubs_complete() NOT_CDS_RETURN;


I don't see set_shared_stubs_complete, set_c1_stubs_complete and set_c2_stubs_complete called from anywhere.

ashu-mehra · 2025-12-02T16:43:29Z

src/hotspot/share/code/aotCodeCache.hpp

+
+  ~AOTStubData()    CDS_ONLY({FREE_C_HEAP_ARRAY(StubAddrRange, _ranges);}) NOT_CDS({})
+
+    bool is_open()  CDS_ONLY({ return (_flags & OPEN) != 0; }) NOT_CDS_RETURN_(false);


extra indentation

ashu-mehra · 2025-12-02T16:43:41Z

src/hotspot/share/code/aotCodeCache.cpp

-  _complete = true;
+void AOTCodeAddressTable::set_c1_stubs_complete() {
+  assert(!_c1_stubs_complete, "repeated close for c1 stubs!");
+  _c2_stubs_complete = true;


should be setting _c1_stubs_complete

ashu-mehra · 2025-12-02T16:46:34Z

src/hotspot/share/code/aotCodeCache.cpp

+  for (int i = 0; i < addresses.length(); i++) {
+    SET_ADDRESS(_extrs, addresses.at(i));
+  }
+  log_debug(aot, codecache, init)("External addresses recorded");


This log message is not very helpful. Following the code I now know it refers to the step where arch specific external address are added. So it does seem relevant, however the current message does not convey the context. But I don't know how to improve it either :).

ashu-mehra · 2025-12-02T17:03:38Z

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp

@@ -2383,6 +2736,9 @@ class StubGenerator: public StubCodeGenerator {
    __ leave();   // required for proper stackwalking of RuntimeStub frame
    __ ret(lr);

+    // record the stub entry and end plus any no_push entry


Same as before, comments refers to "no_push entry".

ashu-mehra · 2025-12-02T20:46:58Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

-    vmovdqu(BYTE_FLIP_MASK, ExternalAddress(pshuffle_byte_flip_mask_addr +  0)); // PSHUFFLE_BYTE_FLIP_MASK wrt rip
+    assert(pshuffle_byte_flip_mask_addr + 32 == StubRoutines::x86::pshuffle_byte_flip_mask_ymm_lo_addr_sha512(), "sanity");
+
+  vmovdqu(BYTE_FLIP_MASK, ExternalAddress(pshuffle_byte_flip_mask_addr +  0)); // PSHUFFLE_BYTE_FLIP_MASK wrt rip


Indentation is off

ashu-mehra · 2025-12-02T20:49:37Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

+  assert(pshuffle_byte_flip_mask_addr + 64 == StubRoutines::x86::pshuffle_byte_flip_mask_dc00_addr(), "sanity");
+
  vmovdqu(BYTE_FLIP_MASK, ExternalAddress(pshuffle_byte_flip_mask_addr +  0)); // [PSHUFFLE_BYTE_FLIP_MASK wrt rip]
  vmovdqu(SHUF_00BA,      ExternalAddress(pshuffle_byte_flip_mask_addr + 32)); // [_SHUF_00BA wrt rip]


Now that we have StubRoutines::x86::pshuffle_byte_flip_mask_00ba_addr(), I think it should replace pshuffle_byte_flip_mask_addr + 32. Makes the usage of these addresses explicit.

ashu-mehra · 2025-12-02T20:52:51Z

src/hotspot/cpu/zero/stubRoutines_zero.cpp

 address StubRoutines::crc32c_table_addr() { ShouldNotCallThis(); return nullptr; }
+
+#if INCLUDE_CDS
+// nothing to do for xero


ashu-mehra · 2025-12-02T20:56:18Z

src/hotspot/share/code/aotCodeCache.cpp


  // Disable stubs caching until JDK-8357398 is fixed.
-  FLAG_SET_ERGO(AOTStubCaching, false);
+  // FLAG_SET_ERGO(AOTStubCaching, false);


I can't seem to access JDK-8357398. I am not sure if that bug is fixed or not. But if it is not fixed yet, I think we should keep this un-commented. I guess this was commented out for testing purpose.

adinn added 16 commits November 12, 2025 12:04

complete stub save and restore for aarch64

c294550

count and log stubgen blobs

53cf355

remove redundant publication of external addresses

1397164

fix missing external address

3f052ce

fix external address problem

32f1320

fix stub publish/save checks

b9adda3

move handler management routines to shared code

16f4d1f

first complete version of x86 stub save and restore

9f27b2b

fix x86 avx2 stub generation

24a7c88

fix zero/minimal build issues

1da6590

fix more zero issues

3774e5f

correct var init on x86

2043042

update zero port with required changes

351712a

update arm/ppc/riscv/s390/ ports with required changes

63d4d1c

fix typo

7cf62b2

fix format issue

042be3f

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 20, 2025

vnkozlov reviewed Nov 21, 2025

View reviewed changes

adinn commented Nov 24, 2025

View reviewed changes

src/hotspot/cpu/aarch64/stubDeclarations_aarch64.hpp Outdated Show resolved Hide resolved

adinn added 4 commits November 25, 2025 14:59

adjust extry/extra address offset encoding to distinguish end from nu…

9cd7376

…llptr

tighten up unsafe handler address checks

d20c218

use simpler AOT lookup in all x86 stub generators

e9442c3

add missing aot+codecache+init statistic

1aab99c

use simpler AOT lookup in all aarch64 stub generators

a6ea637

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 26, 2025

adinn added 6 commits November 26, 2025 10:31

fix typos

022cb3c

correct assert

29b478c

remove redundant comment

1a996eb

rmeove whitespace

af40c88

more whitespace

59cc4b2

merge

cbf7540

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 26, 2025

fix extras count to match number of unsafe handler regions

765d9d6

adinn changed the title ~~Save stubgen stubs~~ 8372617: Save and restore stubgen stubs when using an AOT code cache Nov 26, 2025

adinn added 3 commits November 26, 2025 17:12

put AOT address table init code under INCLUDE_CDS

f3d7bfa

move AOT address init impl into arch tree below StubRoutines

094e173

fix header declarations

0597bc6

adinn marked this pull request as ready for review November 28, 2025 10:35

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

ashu-mehra suggested changes Dec 2, 2025

View reviewed changes


		// Non-generated init method

		void StubRoutines::init_AOTAddressTable() {


		~AOTStubData() CDS_ONLY({FREE_C_HEAP_ARRAY(StubAddrRange, _ranges);}) NOT_CDS({})

		bool is_open() CDS_ONLY({ return (_flags & OPEN) != 0; }) NOT_CDS_RETURN_(false);

8372617: Save and restore stubgen stubs when using an AOT code cache #28433

Are you sure you want to change the base?

8372617: Save and restore stubgen stubs when using an AOT code cache #28433

Conversation

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

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Nov 20, 2025

Uh oh!

openjdk bot commented Nov 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

adinn commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinn Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Nov 21, 2025

Uh oh!

Uh oh!

openjdk bot commented Nov 26, 2025

Uh oh!

mlbridge bot commented Nov 28, 2025

Webrevs

Uh oh!

ashu-mehra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

adinn commented Nov 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 20, 2025 •

edited

Loading

adinn Nov 24, 2025 •

edited

Loading