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

[Thomas Stuefe]

On Sun, 18 Feb 2024 03:25:12 GMT, Yude Lin <duke at openjdk.org> wrote:

> > 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()

I think most of your changes are unnecessary, sorry, for the reasons stated in my last answer. In your patch, I dislike the duplication of the assert logic, increasing the MacroAssembler API surface, and the strange return code contract of `eor_compatible_klass_encoding`. 

But lets step back a bit. We worked quite hard to make the class space reservation on Arm64 to work with an extremely high probability. Do you see the "reserve_anywhere" path hit somewhere for real? If so, I'd be interested in the context - which machine (an sbc?), which linux, which kernel, which settings, and so on.

Also, it would be a lot nicer to not have to worry about this at all. I took a stab last summer in adding a decoding mode that would just work with any address, similar to how PPC does it (https://github.com/openjdk/jdk/blob/3742bc626e80f597373913f02e79c5231e1b7dbc/src/hotspot/cpu/ppc/assembler_ppc.cpp#L467) but I found that I couldn't do it for the case where I only have one register to work with in `decode_klass_not_null()`, and it was not clear to me which registers were scratchable (I also did not try very hard, admittedly).

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

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