Hi Nick, Looks good to me (although it's been years since I worked on any ARM ports). The detailed background and explanation are very nice! One nit for consistency. The 'KlassDecodeMovk' case returns, while the rest of the cases in MacroAssembler::encode_klass_not_null use break. 3963 return; Best regards, Jiangli On Tue, Dec 10, 2019 at 11:09 PM Nick Gasson <nick.gasson@arm.com> wrote:

Hi,

Please help to review this patch to fix some issues uncovered by CDS archive relocation.

Bug: https://bugs.openjdk.java.net/browse/JDK-8234794 Webrev: http://cr.openjdk.java.net/~ngasson/8234794/webrev.01/

As a bit of background, on AArch64 there are a few different ways we can decode/encode compressed klass pointers, depending on the base address alignment and shift.

(1) If the base is NULL then it's a no-op or shift.

(2) If we can encode the the base as a logical immediate, use an EOR instruction to encode/decode.

(3) If the base is 4G aligned and the shift is zero, use a MOVK instruction to combine the base and offset.

(4) Otherwise emit an inefficient sequence using rheapbase as a temporary, and then restore the heap base (poorly tested).

On AArch64 and AIX we have a loop in Metaspace::allocate_metaspace_compressed_klass_ptrs that searches for a 4G aligned location to map the compressed class space and enable optimisations (2) and (3) above. It's possible to fail and fall through to (4) after mmap-ing at an arbitrary address, but I think in practice this never happens.

MetaspaceShared allocates the compressed class space in a different way, but the default base address is 0x80000000 which allows method (2) above. After the changes in 8231610 the CDS archive can be relocated to an arbitrary location decided by mmap, which makes case (4) much more likely.

The test case in the title passes -XX:HeapBaseMinAddress=33g which triggers the CDS relocation. The combination of (4) and the extra shift/add from the non-NULL narrow OOP base overflows the itable stub code buffer.

I want to fix this by ensuring that the compressed klass space is always at least 4G aligned on AArch64 so we can get rid of (4). This will reduce the variability in code size from memory layout, reduce the number of configurations we need to test, and makes repurposing rheapbase easier (see the review thread for 8233743).

This patch moves the AArch64/AIX address space search loop into a new function Metaspace::reserve_preferred_space. MetaspaceShared::reserve_shared_space now calls this rather than directly constructing the ReservedSpace. If use_requested_addr is false it will search for a suitably aligned location (replacement for ReservedSpace() with requested_addr=NULL), otherwise it checks the fixed address meets the alignment requirements. Should be no functional change on platforms other than AArch64 and AIX.

Additionally, to support method (3) when the compressed klass shift is non-zero (which it always is when CDS is on), the compressed klass base will be aligned to a (4 << LogKlassAlignmentInBytes)*G boundary when the base is above 32GB (limit for zero-based address). This allows us to shift the base address right LogKlassAlignmentInBytes bits and use MOVK to add it to the offset (discarding the lower 32 bits). I added an extra case to SharedBaseAddress.java to cover this during jtreg testing.

Finally there's a small bug in the existing code to determine if we can use the EOR optimisation (2).

use_XOR_for_compressed_class_base = operand_valid_for_logical_immediate (/*is32*/false, (uint64_t)CompressedKlassPointers::base()) && ((uint64_t)CompressedKlassPointers::base() > (1UL << log2_intptr(CompressedKlassPointers::range())));

This doesn't work if e.g. the base is 0x180000000 and the rounded-up range is 0x100000000 because the high significant bits of the compressed pointer overlap with the low significant bits of the base, even though the base is strictly greater than the range. So the following will SEGV with the current VM:

java -Xmx128m -XX:SharedBaseAddress=6g -Xshare:dump

Fixed this and moved all the logic for deciding which scheme to use into MacroAssembler::klass_decode_mode.

Tested jtreg hotspot_all_no_apps, jdk_core on AArch64 and x86. Also did some manual testing with -XX:SharedBaseAddress and -XX:ArchiveRelocationMode=1 (thanks Ioi).

I'm not able to test on AIX which shares the same metaspace code (CC'd ppc-aix-port-dev).

Thanks, Nick