8323273: AArch64: Strengthen CompressedClassPointers initialization check for base #17437

linade · 2024-01-16T02:41:40Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8323273: AArch64: Strengthen CompressedClassPointers initialization check for base (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17437/head:pull/17437
$ git checkout pull/17437

Update a local copy of the PR:
$ git checkout pull/17437
$ git pull https://git.openjdk.org/jdk.git pull/17437/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17437

View PR using the GUI difftool:
$ git pr show -t 17437

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17437.diff

Webrev

Link to Webrev Comment

…heck for base

bridgekeeper · 2024-01-16T02:42:49Z

👋 Welcome back linade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-01-16T02:44:19Z

@linade The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-01-16T02:47:43Z

Webrevs

00: Full (0e6e1284)

linade · 2024-01-18T11:49:12Z

Can I get a review on this patch please : )

linade · 2024-01-29T02:25:34Z

Ping. Can I get a review for this change?

dean-long · 2024-02-09T00:53:16Z

Don't you want to add a constraint function to CompressedClassSpaceBaseAddress in globals.hpp so invalid values are rejected during command-line parsing?

tstuefe

Hi @linade,

as part of Lilliput, we are planning larger changes to this code in mainline - see openjdk/lilliput#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.

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

Cheers, Thomas

tstuefe · 2024-02-09T15:08:17Z

src/hotspot/share/memory/metaspace.cpp

@@ -586,11 +586,14 @@ ReservedSpace Metaspace::reserve_address_space_for_compressed_classes(size_t siz
      (char*) CompressedKlassPointers::reserve_address_space_for_compressed_classes(size, RandomizeClassSpaceLocation,
                                                                                    optimize_for_zero_base));

+#if !defined(AARCH64)
+  // On aarch64, this likely wouldn't satisfy the encoding constraints.


Please don't add platform ifdefs here! We put much work into getting rid of platform-dependent sections. Also, as stated above, I think this change is not needed.

tstuefe · 2024-02-09T15:12:32Z

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp

@@ -130,4 +130,14 @@ void CompressedKlassPointers::initialize(address addr, size_t len) {
  _base = (end <= (address)unscaled_max) ? nullptr : addr;

  _range = end - _base;
+
+  DEBUG_ONLY(assert_is_valid_encoding(addr, len, _base, _shift);)


asserts are debug only, DEBUG_ONLY is not needed

tstuefe · 2024-02-09T15:22:20Z

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp

@@ -67,7 +67,7 @@ static char* reserve_at_eor_compatible_address(size_t size, bool aslr) {
    const int alt_index = (ntry & 1) ? 0 : num_immediates / 2;
    const int index = (start_index + ntry + alt_index) % num_immediates;
    const uint64_t immediate = ((uint64_t)immediates[index]) << 32;
-    assert(immediate > 0 && Assembler::operand_valid_for_logical_immediate(/*is32*/false, immediate),
+    assert(immediate > 0 && MacroAssembler::eor_compatible_klass_encoding(immediate) >= 32,


The format of this helper is rather awkward. It returns 0 if the address is not compatible? Otherwise, the number of trailing zeros?

I think there is no strong need for any of this, since the original assert is already over-cautious (after all, we only test a range of hard-coded values). But if you like to add a test for the lower 32-bit, I'd just add that here, no need for these functions.

tstuefe · 2024-02-09T15:23:22Z

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp

+  return (base == nullptr /* Zero-based encoding */ ||
+          (1ULL << MacroAssembler::eor_compatible_klass_encoding((uint64_t)base) >= range) ||
+          MacroAssembler::movk_compatible_klass_encoding((uint64_t)base)) &&
+         (shift == -1 /* Not specified */ || shift == 0);


This duplicates the assert we have in MacroAssembler; why do this twice, in two places?

tstuefe · 2024-02-09T15:24:47Z

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

      return (_klass_decode_mode = KlassDecodeXor);
    }
  }

  const uint64_t shifted_base =
    (uint64_t)CompressedKlassPointers::base() >> CompressedKlassPointers::shift();
-  guarantee((shifted_base & 0xffff0000ffffffff) == 0,
+  guarantee(movk_compatible_klass_encoding(shifted_base),
            "compressed class base bad alignment");


I also think this code can benefit from a bit cleanup some time in the future, but for now I ask you to hold off: there is no pressing need at the moment, and once Lilliput things have settled, this code should be easier to work with it.

Also, there are a few more things we could improve, and I rather see those in a combined single RFE.

linade · 2024-02-18T03:25:12Z

Hi @linade,

as part of Lilliput, we are planning larger changes to this code in mainline - see openjdk/lilliput#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()

linade · 2024-02-18T03:44:43Z

Don't you want to add a constraint function to CompressedClassSpaceBaseAddress in globals.hpp so invalid values are rejected during command-line parsing?

Thank you. That might also work. We might not need to deliberately check for this develop value though but instead let it hit asserts in more common code (see a comment above).

theRealAph · 2024-02-18T09:45:48Z

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.

Exactly this. Not a bug.

tstuefe · 2024-02-19T07:54:18Z

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 (

jdk/src/hotspot/cpu/ppc/assembler_ppc.cpp

Line 467 in 3742bc6

    
           int Assembler::add_const_optimized(Register d, Register s, long x, Register tmp, bool return_simm16_rest) {

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

theRealAph · 2024-02-19T09:01:25Z

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 (

I think we should leave this stuff alone. Compressed klass pointer handling has been around the block a few times, and there's little possibility of providing much more benefit to our end users. We've got something that is highly reliable and almost maximally performant.

tstuefe · 2024-02-19T09:18:34Z

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 (

I think we should leave this stuff alone. Compressed klass pointer handling has been around the block a few times, and there's little possibility of providing much more benefit to our end users. We've got something that is highly reliable and almost maximally performant.

Okay. Seeing how rare the failing of reservation is, I came to the same conclusion.

linade · 2024-02-19T10:38:31Z

I think the strangest thing to me is if the reservation fails (admittedly very rare if ever), the code choose to fire the error at stub generation. It's rather random---Maybe in another day, when initialization code changes and something that also decodes klass moves ahead of stub generation, the error could happen there. We would need time to associate all the appearances of the error with the root cause of the error. So why not add assertions right after reservation?

You might argue that this nearly never happens in product and not worth the complication. I currently cannot argue with that because I haven't observe one myself, and I will drop the patch. But note that a single assertion would help. Perhaps consider adding one in incoming class-space refactoring?

.. strange return code contract of eor_compatible_klass_encoding.

It returns the number of bits allowed to encode a klass. So 0 means there is no space left for klass encoding. Thanks for the feedbacks though. I understand the frustration of seeing those platform dependent code in shared code. I'll try not to write anything like that again.

tstuefe · 2024-02-19T12:38:16Z

I think the strangest thing to me is if the reservation fails (admittedly very rare if ever), the code choose to fire the error at stub generation. It's rather random---Maybe in another day, when initialization code changes and something that also decodes klass moves ahead of stub generation, the error could happen there. We would need time to associate all the appearances of the error with the root cause of the error.

Okay, you convinced me. I'm still against changes to CompressedClassSpaceBaseAddress and the removal of reserve-anywhere patch, though.

So why not add assertions right after reservation?

Yes, without code duplication.

The most straightforward and least invasive possibility would be to improve the assertion messages and/or add comments in MacroAssembler::klass_decode_mode().
The next simple possibility would be to reuse MacroAssembler::klass_decode_mode() as part of verification after initialization.

You might argue that this nearly never happens in product and not worth the complication. I currently cannot argue with that because I haven't observe one myself, and I will drop the patch. But note that a single assertion would help. Perhaps consider adding one in incoming class-space refactoring?

A single assertion means I still need a way to know when it fires, so its condition needs to guess at the decoding logic that is hidden in MacroAssembler. I don't like that duplication.

.. strange return code contract of eor_compatible_klass_encoding.

It returns the number of bits allowed to encode a klass. So 0 means there is no space left for klass encoding. Thanks for the feedbacks though. I understand the frustration of seeing those platform dependent code in shared code. I'll try not to write anything like that again.

Sorry for your frustration.

You get strong pushback here because this code, in particular, has been seeing platform ifdefs blooming like mushrooms over the years - it had often been easier to add one extra block than to look at the base structure. Which made the code complex and brittle over the years. We restructured this coding recently with https://bugs.openjdk.org/browse/JDK-8312018, and I would like to keep it somewhat clean and readable.

This coding is also rather tricky because it lives at the interjunction of three VM subsystems: metaspace/class space, JIT compiler (with its various CPU-specific idiosyncrasies) and CDS.

openjdk · 2024-03-13T20:15:41Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

bridgekeeper · 2024-04-11T00:56:42Z

@linade This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-05-09T04:01:20Z

@linade This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8323273: AArch64: Strengthen CompressedClassPointers initialization c…

0e6e128

…heck for base

openjdk bot added the rfr Pull request is ready for review label Jan 16, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 16, 2024

tstuefe reviewed Feb 9, 2024

View reviewed changes

bridgekeeper bot closed this May 9, 2024

8323273: AArch64: Strengthen CompressedClassPointers initialization check for base #17437

8323273: AArch64: Strengthen CompressedClassPointers initialization check for base #17437

Uh oh!

Conversation

linade commented Jan 16, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jan 16, 2024

Uh oh!

openjdk bot commented Jan 16, 2024

Uh oh!

mlbridge bot commented Jan 16, 2024

Webrevs

Uh oh!

linade commented Jan 18, 2024

Uh oh!

linade commented Jan 29, 2024

Uh oh!

dean-long commented Feb 9, 2024

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

tstuefe Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

tstuefe Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

tstuefe Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linade commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linade commented Feb 18, 2024

Uh oh!

theRealAph commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tstuefe commented Feb 19, 2024

Uh oh!

theRealAph commented Feb 19, 2024

Uh oh!

tstuefe commented Feb 19, 2024

Uh oh!

linade commented Feb 19, 2024

Uh oh!

tstuefe commented Feb 19, 2024

Uh oh!

openjdk bot commented Mar 13, 2024

Uh oh!

bridgekeeper bot commented Apr 11, 2024

Uh oh!

bridgekeeper bot commented May 9, 2024

Uh oh!

Uh oh!

linade commented Jan 16, 2024 •

edited by openjdk bot

Loading

tstuefe Feb 9, 2024 •

edited

Loading

tstuefe Feb 9, 2024 •

edited

Loading

linade commented Feb 18, 2024 •

edited

Loading

theRealAph commented Feb 18, 2024 •

edited

Loading