RFR: 8340184: Bug in CompressedKlassPointers::is_in_encodable_range
Coleen Phillimore
coleenp at openjdk.org
Mon Sep 16 13:41:10 UTC 2024
On Mon, 16 Sep 2024 10:37:55 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 range end* and *encoding range start* ) and was only used in a sing...
I have some minor comments.
src/hotspot/share/oops/compressedKlass.hpp line 39:
> 37: // a contiguous memory range into which we place Klass that should be encodable. Not every Klass
> 38: // needs to be encodable. There is only one such memory range.
> 39: // If CDS is disabled, this Klass Range is the same as the class space. If CDS is enabled, the
Can you say metaspace class space, or class space in metaspace?
test/hotspot/gtest/oops/test_compressedKlass.cpp line 31:
> 29: #include "unittest.hpp"
> 30:
> 31: TEST_VM(compressedKlass, basics) {
We typically capitalize the test name. Does that conflict with the class name in the JVM? If so it could be CompressedKlassTest.
test/hotspot/jtreg/gtest/CompressedKlassGtest.java line 2:
> 1: /*
> 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
I don't know why you need this. I thought the gtests just ran with TEST_VM.
Also Copyright SAP is probably wrong for you now.
-------------
Changes requested by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21015#pullrequestreview-2306664929
PR Review Comment: https://git.openjdk.org/jdk/pull/21015#discussion_r1761156751
PR Review Comment: https://git.openjdk.org/jdk/pull/21015#discussion_r1761166596
PR Review Comment: https://git.openjdk.org/jdk/pull/21015#discussion_r1761176362
More information about the hotspot-dev
mailing list