RFR: 8340212: -Xshare:off -XX:CompressedClassSpaceBaseAddress=0x40001000000 crashes on macos-aarch64 [v10]

[Coleen Phillimore]

On Mon, 9 Dec 2024 17:21:59 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
>> 
>>  - Merge branch 'master' into decode-mode
>>  - Recalculate decode mode if OS picks a different encoding.
>>  - Only calculate klass-decode-mode in one place.
>>  - Missed one. Another good reason to not copy code.
>>  - Rename ceil_log2 to log2i_ceil
>>  - Merge branch 'master' into decode-mode
>>  - Merge with incoming change for PR https://github.com/openjdk/jdk/pull/21932
>>  - Merge branch 'master' into decode-mode
>>  - include formatBuffer.hpp
>>  - 8340212: -Xshare:off -XX:CompressedClassSpaceBaseAddress=0x40001000000 crashes on macos-aarch64
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5328:
> 
>> 5326: 
>> 5327: // Check if one of the above decoding modes will work for given base, shift and range.
>> 5328: bool MacroAssembler::check_klass_decode_mode(address base, int shift, const size_t range) {
> 
> Can we refactor the code so that `check_klass_decode_mode()` only does the checking and have no side effects?
> 
> This code is called from CDS, which sometimes will unmap the archive and retry at an os-assigned random address, so updating the actual base/shift here will be problematic.

My first patch did that, but it had to copy the code to see if the encoding could use Movk.  I think I do like that better to not have side effects since the code has to be called again to have the correct decode mode after allocation succeeds.

> src/hotspot/share/cds/metaspaceShared.cpp line 1496:
> 
>> 1494:                   p2i(base_address), precomputed_narrow_klass_shift);
>> 1495:     use_archive_base_addr = false;
>> 1496:   }
> 
> It seems there's no easy way of testing this code, as the `base_address` need to come from a CDS archive, but the CDS archive cannot contain invalid base addresses.
> 
> Maybe we should change this to `log_error()`, or even `fatal()`

If this is a log-error or fatal, then maybe we should give an error for an invalid SharedBaseAddress, as with Thomas's suggested patch.  We generally try to get SharedBaseAddress and that fails, let the OS code pick one.  If we want to give an error for invalid SharedBaseAddress, I could fix the test that fails to not try for an invalid address.  But the validity of the address is only for aarch64 so I don't know if we want to do this.

> test/hotspot/jtreg/runtime/CompressedOops/CompressedClassPointersEncodingScheme.java line 87:
> 
>> 85: 
>> 86:         // We ignore cases where we were not able to map at the force address
>> 87:         if (!output.contains("Successfully forced class space address to " + forceAddressString)) {
> 
> "Successfully forced class space address" seems misleading. Can we change the code in metaspace.cpp to say something like "reserved class space at ...."?

Ok.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21695#discussion_r1876410676
PR Review Comment: https://git.openjdk.org/jdk/pull/21695#discussion_r1876414312
PR Review Comment: https://git.openjdk.org/jdk/pull/21695#discussion_r1876415087