RFR: 8340184: Bug in CompressedKlassPointers::is_in_encodable_range [v2]

[Coleen Phillimore]

On Mon, 16 Sep 2024 14:29:38 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Since JDK-8338526, we keep only those Klass in the encoding range that need a narrowKlass id.
>> 
>> To check whether a Klass has a narrowKlass id, we call `CompressedKlassPointers::is_in_encodable_range()`. There is a small bug that results from the confusion around "encoding range" vs "klass range".
>> 
>> The **"Encoding Range"** is the range that can be encoded with the current encoding base, encoding shift, and (implicitly) the bit size of the narrowKlass, which is 32. The encoding range reaches from `[ <encoding base> ... <encoding base> + 1 << (32 + <encoding shift>) ).`. Its size is either 4G (shift=0) or 32G (shift=3).
>> 
>> The **"Klass Range"** is the range that actually holds Klass structures. It is part of the Encoding Range, but usually much smaller:
>> - with zero-based encoding, the encoding base is zero, so it precedes the start of the Klass range
>> - The encoding range can reach far beyond the end of the Klass range.
>> 
>> For a more detailed explanation, including pleasing ASCII art, please refer to `compressedKlass.hpp` in this patch.
>> 
>> ----
>> 
>> The error in this case, introduced with 8338526, was that we use the range `[<encoding base> ... <klass range end>)` for `is_in_encodable_range()`. That can lead to false positives since `<encoding base>` can be zero. In a highly contrived theoretical case, we could mis-classify a Klass as being encodable if it lives in metaspace outside class space, but its metaspace region happens to be located below the class space start.
>> 
>> The error is extremely unlikely because:
>> - non-class Metaspace regions - freely allocated by mmap - typically live in high-address ranges
>> - we only allow zero-based encoding with Xshare:off, itself a very unusual setting nowadays
>> 
>> But the error highlights misuse of the term "encoding range", so it should be fixed.
>> 
>> ----
>> 
>> Note that most of the patch has been selectively copied from Lilliput. Lilliput, with its non-22-bit narrowKlass, had needed to straighten out this code a while ago, and it is better for it.
>> 
>> The fix:
>> - we now use the actual *Klass Range* for the "is encodable" check. We don't use the encoding base. That is the real fix.
>> - To do that, we need to keep track of the Klass Range inside `CompressedKlassPointers`. That is simple, since we are given this range during initialization.
>> 
>> Minor cleanups:
>> - I also removed the confusingly named `CompressedKlassPointers::range()`. This was a strange animal ( the distance between *klass ran...
>
> Thomas Stuefe has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - comment fix
>  - comment fix missing close paranthesys
>  - Feedback Johan
>  - Feedback Coleen

Looks really good.

src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp line 134:

> 132:   // Remember the Klass range:
> 133:   _klass_range_start = addr;
> 134:   _klass_range_end = addr + len;

In the Lilliput review, I think you were moving this to shared code with an ifdef so that these variables are all set in one place.  This is ok here for now.

test/hotspot/jtreg/gtest/CompressedKlassGtest.java line 31:

> 29:  * mode, we start with CDS disabled, a small class space and a large (albeit uncommitted, to save memory) heap. The
> 30:  * JVM will likely place the class space in low-address territory.
> 31:  * (If it does not manage to do this, the test will note that and print "skipped"

Ok, I see why you have this.  thanks for the comment.  One nit, the ) is missing in this sentence.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21015#pullrequestreview-2306833750
PR Review Comment: https://git.openjdk.org/jdk/pull/21015#discussion_r1761265199
PR Review Comment: https://git.openjdk.org/jdk/pull/21015#discussion_r1761263495