RFR: 8340184: Bug in CompressedKlassPointers::is_in_encodable_range
Thomas Stuefe
stuefe at openjdk.org
Mon Sep 16 12:17:26 UTC 2024
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. 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 range end* and *encoding range start* ) and was only used in a single place in the VM to compute the max. value of a *preshifted* narrowKlass (see macroAssembler_aarch64.cpp).
- I renamed `is_in_encoding_range` to `is_encodable` since we don't check for the "encoding range", we check for the "klass range".
New Tests:
- regression test
Additional testing:
* [x] Linux x64 fastdebug, `runtime/cds`, `runtime/CompressedOops` and `gtest`
* [x] MaxOS aarch64 fastdebug, `runtime/CompressedOops` and `gtest`
-------------
Commit messages:
- fix macos
- JDK-8340184-Bug-in-CompressedKlassPointers-is_in_encodable_range
Changes: https://git.openjdk.org/jdk/pull/21015/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21015&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8340184
Stats: 240 lines in 10 files changed: 213 ins; 10 del; 17 mod
Patch: https://git.openjdk.org/jdk/pull/21015.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/21015/head:pull/21015
PR: https://git.openjdk.org/jdk/pull/21015
More information about the hotspot-dev
mailing list