RFR: 8234794: AArch64: runtime/memory/ReadFromNoaccessArea.java crashes

[Jiangli Zhou]

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 at 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
>
>