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

Tue Dec 2 21:10:03 UTC 2025

On Thu, 20 Nov 2025 14:59:02 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

> 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:
> 1. the first declared entry of every stub starts at the first instruction in the stub code range
> 2. all data/code cross-references from one stub to another target a declared stub entry

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

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 1846:

> 1844: 
> 1845:     bool add_extras = !is_oop && (!aligned || sizeof(jlong) == size);
> 1846:     int extra_count = ((add_extras ? 1 : 0) * 3);

I think it would read better if a macro is used instead of hard-coding 3 here, something like `UnsafeMemoryAccess::ADDRESS_COUNT"

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 2184:

> 2182:     // the normal stub provides a 2nd entry which omits the frame push
> 2183:     // for use when bailing out from a disjoint copy
> 2184:     // We need to protect memory accesses in certain cases

This comment doesn't seem to be applicable here. I don't see an `UnsafeMemoryAccessMark`.

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 2452:

> 2450:     __ b(RuntimeAddress(long_copy_entry));
> 2451: 
> 2452:     // record the stub entry and end plus any no_push entry

Comment refers to "no_push entry", but there is none here. I guess a result of copy-paste.

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 2739:

> 2737:     __ ret(lr);
> 2738: 
> 2739:     // record the stub entry and end plus any no_push entry

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

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp line 799:

> 797: 
> 798:   vmovdqu(BYTE_FLIP_MASK, ExternalAddress(pshuffle_byte_flip_mask_addr +  0)); // [PSHUFFLE_BYTE_FLIP_MASK wrt rip]
> 799:   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.

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp line 1358:

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

Indentation is off

src/hotspot/cpu/zero/stubRoutines_zero.cpp line 35:

> 33: 
> 34: #if INCLUDE_CDS
> 35: // nothing to do for xero

xero->zero

src/hotspot/share/code/aotCodeCache.cpp line 191:

> 189: 
> 190:   // Disable stubs caching until JDK-8357398 is fixed.
> 191:   // 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.

src/hotspot/share/code/aotCodeCache.cpp line 1742:

> 1740: // id as an index
> 1741: 
> 1742: #define SET_ENTRY_ADDRESS(type, addr, entry_id)           \

This macro seems to be unused.

src/hotspot/share/code/aotCodeCache.cpp line 1975:

> 1973:     SET_ADDRESS(_extrs, addresses.at(i));
> 1974:   }
> 1975:   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 :).

src/hotspot/share/code/aotCodeCache.cpp line 1998:

> 1996: void AOTCodeAddressTable::set_c1_stubs_complete() {
> 1997:   assert(!_c1_stubs_complete, "repeated close for c1 stubs!");
> 1998:   _c2_stubs_complete = true;

should be setting `_c1_stubs_complete`

src/hotspot/share/code/aotCodeCache.hpp line 245:

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

extra indentation

src/hotspot/share/code/aotCodeCache.hpp line 403:

> 401:   int store_strings();
> 402: 
> 403:   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.

src/hotspot/share/runtime/stubCodeGenerator.cpp line 118:

> 116: // default_handler.
> 117: 
> 118: 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?

-------------

Changes requested by asmehra (Committer).

PR Review: https://git.openjdk.org/jdk/pull/28433#pullrequestreview-3531142588
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582028698
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582087169
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582094005
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582097741
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582736962
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582730520
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582744339
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582752398
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582024777
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582024431
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582015259
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582014593
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582014427
PR Review Comment: https://git.openjdk.org/jdk/pull/28433#discussion_r2582013969