RFR: 8323273: AArch64: Strengthen CompressedClassPointers initialization check for base

[Yude Lin]

On Mon, 29 Jan 2024 02:25:34 GMT, Yude Lin <duke at openjdk.org> wrote:

>> Summary:
>> Add a platform-dependent check for CompressedClassSpaceBaseAddress;
>> Remove the "reserve anywhere" attempt after the initial mapping attempt failed---this is rarely used and will likely fail anyway, because the accepted mapping is very restricted on aarch64;
>> Additional assertions after initialization.
>> 
>> Passed hotspot/jtreg/:tier1 on fastdebug
>
> Ping. Can I get a review for this change?

> Hi @linade,
> 
> as part of Lilliput, we are planning larger changes to this code in mainline - see [openjdk/lilliput#128](https://github.com/openjdk/lilliput/pull/128), which will also go in a bit different form into mainline. It would be nice if you could hold off your PR until then.

Thanks for pointing out. If so, I will check again after the lilliput changes are in and see if the issue still persists. (I'll also address the review feedbacks then.) But I still think the changes are needed for the following reasons.

> But I am also not convinced the changes are needed. I see some improvements to code clarity, but those I would rather see after Lilliput hits, in a separate RFE. For the two functional main points this PR addresses:
> 
> * `CompressedClassSpaceBaseAddress` is a development debug-only tool that should do exactly what's on the label: dumbly enforce an address. Its only purpose is to observe the JVM's handling of this address. If that handling includes asserting, I'd like to see that. There is no need for more complexity - it is a simple tool with sharp edges and it only exists for tests.

The point at which the error handling happens is (init_globals->generate_stubs->..->load_klass->decode_klass_not_null), that is, trying to generate stubs. It doesn't look like a very good place to handle the errors from class space reservation (to me it looks like a double check). I would put asserts right after parsing the command-line value of CompressedClassSpaceBaseAddress, so that the reason of failing is clear.

However, since CompressedClassSpaceBaseAddress is develop, instead of adding dedicated asserts, we could also let illegal values hit the newly added assert in CompressedKlassPointers::initialize() which is the rendezvous of all reservation code.

> * I don't see what we gain by removing the "reserve anywhere" path. "It may fail with a high probability" is not enough reason - it also may succeed. At the cost of one additional mmap call (which we probably called before that point a hundred times) it still can prevent an early VM death.

The problem with the "reserve anywhere" path is that it allows illegal base address to pass (which subsequently crashes vm during generate_stubs). We could check the resulting address after we "reserve anywhere". But I just think the reservation itself is futile anyway since aarch64 imposes a strong constraint which is not easy to satisfy. So I would skip that path and then hit the added assert in CompressedKlassPointers::initialize()

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

PR Comment: https://git.openjdk.org/jdk/pull/17437#issuecomment-1950892388