From 7849f252937dc774a1935cc4c68f2a46649f180b Mon Sep 17 00:00:00 2001 From: Thomas Stuefe Date: Tue, 17 Sep 2024 05:22:59 +0000 Subject: [PATCH] 8340184: Bug in CompressedKlassPointers::is_in_encodable_range Reviewed-by: coleenp, rkennke, jsjolen --- .../cpu/aarch64/compressedKlass_aarch64.cpp | 4 +- .../cpu/aarch64/macroAssembler_aarch64.cpp | 4 +- src/hotspot/share/ci/ciKlass.hpp | 2 +- .../types/traceid/jfrTraceIdKlassQueue.cpp | 2 +- src/hotspot/share/oops/compressedKlass.cpp | 27 ++++-- src/hotspot/share/oops/compressedKlass.hpp | 90 +++++++++++++++---- .../share/oops/compressedKlass.inline.hpp | 5 ++ .../share/utilities/globalDefinitions.hpp | 5 ++ .../gtest/oops/test_compressedKlass.cpp | 79 ++++++++++++++++ .../jtreg/gtest/CompressedKlassGtest.java | 39 ++++++++ 10 files changed, 230 insertions(+), 27 deletions(-) create mode 100644 test/hotspot/gtest/oops/test_compressedKlass.cpp create mode 100644 test/hotspot/jtreg/gtest/CompressedKlassGtest.java diff --git a/src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp b/src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp index 54af69ffaba..fc78813c161 100644 --- a/src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp @@ -129,5 +129,7 @@ void CompressedKlassPointers::initialize(address addr, size_t len) { address const end = addr + len; _base = (end <= (address)unscaled_max) ? nullptr : addr; - _range = end - _base; + // Remember the Klass range: + _klass_range_start = addr; + _klass_range_end = addr + len; } diff --git a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp index d09ef26cef9..08b69b34a94 100644 --- a/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp @@ -5082,8 +5082,8 @@ MacroAssembler::KlassDecodeMode MacroAssembler::klass_decode_mode() { if (operand_valid_for_logical_immediate( /*is32*/false, (uint64_t)CompressedKlassPointers::base())) { - const uint64_t range_mask = - (1ULL << log2i(CompressedKlassPointers::range())) - 1; + const size_t range = CompressedKlassPointers::klass_range_end() - CompressedKlassPointers::base(); + const uint64_t range_mask = (1ULL << log2i(range)) - 1; if (((uint64_t)CompressedKlassPointers::base() & range_mask) == 0) { return (_klass_decode_mode = KlassDecodeXor); } diff --git a/src/hotspot/share/ci/ciKlass.hpp b/src/hotspot/share/ci/ciKlass.hpp index de2d3701f3e..10d8395ed7f 100644 --- a/src/hotspot/share/ci/ciKlass.hpp +++ b/src/hotspot/share/ci/ciKlass.hpp @@ -109,7 +109,7 @@ public: bool is_in_encoding_range() { Klass* k = get_Klass(); - bool is_in_encoding_range = CompressedKlassPointers::is_in_encoding_range(k); + bool is_in_encoding_range = CompressedKlassPointers::is_encodable(k); assert(is_in_encoding_range || k->is_interface() || k->is_abstract(), "sanity"); return is_in_encoding_range; } diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdKlassQueue.cpp b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdKlassQueue.cpp index f7873b50058..30f2afd880c 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdKlassQueue.cpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdKlassQueue.cpp @@ -75,7 +75,7 @@ static size_t element_size(bool compressed) { } static bool can_compress_element(const Klass* klass) { - return CompressedKlassPointers::is_in_encoding_range(klass) && + return CompressedKlassPointers::is_encodable(klass) && JfrTraceId::load_raw(klass) < uncompressed_threshold; } diff --git a/src/hotspot/share/oops/compressedKlass.cpp b/src/hotspot/share/oops/compressedKlass.cpp index 4a94eb9bde0..02483d25a9a 100644 --- a/src/hotspot/share/oops/compressedKlass.cpp +++ b/src/hotspot/share/oops/compressedKlass.cpp @@ -25,7 +25,7 @@ #include "precompiled.hpp" #include "logging/log.hpp" #include "memory/metaspace.hpp" -#include "oops/compressedKlass.hpp" +#include "oops/compressedKlass.inline.hpp" #include "runtime/globals.hpp" #include "runtime/os.hpp" #include "utilities/debug.hpp" @@ -34,7 +34,8 @@ address CompressedKlassPointers::_base = nullptr; int CompressedKlassPointers::_shift = 0; -size_t CompressedKlassPointers::_range = 0; +address CompressedKlassPointers::_klass_range_start = nullptr; +address CompressedKlassPointers::_klass_range_end = nullptr; #ifdef _LP64 @@ -60,9 +61,12 @@ void CompressedKlassPointers::initialize_for_given_encoding(address addr, size_t assert(requested_base == addr, "Invalid requested base"); assert(encoding_range_end >= end, "Encoding does not cover the full Klass range"); + // Remember Klass range: + _klass_range_start = addr; + _klass_range_end = addr + len; + _base = requested_base; _shift = requested_shift; - _range = encoding_range_size; DEBUG_ONLY(assert_is_valid_encoding(addr, len, _base, _shift);) } @@ -87,6 +91,11 @@ char* CompressedKlassPointers::reserve_address_space_for_16bit_move(size_t size, #if !defined(AARCH64) || defined(ZERO) // On aarch64 we have an own version; all other platforms use the default version void CompressedKlassPointers::initialize(address addr, size_t len) { + + // Remember the Klass range: + _klass_range_start = addr; + _klass_range_end = addr + len; + // The default version of this code tries, in order of preference: // -unscaled (base=0 shift=0) // -zero-based (base=0 shift>0) @@ -111,16 +120,20 @@ void CompressedKlassPointers::initialize(address addr, size_t len) { _shift = 0; } } - _range = end - _base; DEBUG_ONLY(assert_is_valid_encoding(addr, len, _base, _shift);) } #endif // !AARCH64 || ZERO void CompressedKlassPointers::print_mode(outputStream* st) { - st->print_cr("Narrow klass base: " PTR_FORMAT ", Narrow klass shift: %d, " - "Narrow klass range: " SIZE_FORMAT_X, p2i(base()), shift(), - range()); + if (UseCompressedClassPointers) { + st->print_cr("Narrow klass base: " PTR_FORMAT ", Narrow klass shift: %d", + p2i(base()), shift()); + st->print_cr("Encoding Range: " RANGE2FMT, RANGE2FMTARGS(_base, encoding_range_end())); + st->print_cr("Klass Range: " RANGE2FMT, RANGE2FMTARGS(_klass_range_start, _klass_range_end)); + } else { + st->print_cr("UseCompressedClassPointers off"); + } } #endif // _LP64 diff --git a/src/hotspot/share/oops/compressedKlass.hpp b/src/hotspot/share/oops/compressedKlass.hpp index 26e0a5f6256..8f89b0550ff 100644 --- a/src/hotspot/share/oops/compressedKlass.hpp +++ b/src/hotspot/share/oops/compressedKlass.hpp @@ -31,6 +31,69 @@ class outputStream; class Klass; +// Narrow Klass Encoding +// +// Klass Range: +// a contiguous memory range into which we place Klass that should be encodable. Not every Klass +// needs to be encodable. There is only one such memory range. +// If CDS is disabled, this Klass Range is the same as the metaspace class space. If CDS is enabled, the +// Klass Range contains both CDS and class space adjacent to each other (with a potential small +// unused alignment gap between them). +// +// Encoding Range: +// This is the range covered by the current encoding scheme. The encoding scheme is defined by +// the encoding base, encoding shift and (implicitly) the bit size of the narrowKlass. The +// Encoding Range is: +// [ ... + (1 << ( + ) ) +// +// Note that while the Klass Range must be contained within the Encoding Range, the Encoding Range +// is typically a lot larger than the Klass Range: +// - the encoding base can start before the Klass Range start (specifically, it can start at 0 for +// zero-based encoding) +// - the end of the Encoding Range usually extends far beyond the end of the Klass Range. +// +// +// Examples: +// +// "unscaled" (zero-based zero-shift) encoding, CDS off, class space of 1G starts at 0x4B00_0000: +// - Encoding Range: [0 .. 0x1_0000_0000 ) (4 GB) +// - Klass Range: [0x4B00_0000 .. 0x 8B00_0000 ) (1 GB) +// +// +// _base _klass_range_start _klass_range_end encoding end +// | |//////////////////////////////| | +// | ... |///////1gb class space////////| ... | +// | |//////////////////////////////| | +// 0x0 0x4B00_0000 0x8B00_0000 0x1_0000_0000 +// +// +// +// "zero-based" (but scaled) encoding, shift=3, CDS off, 1G Class space at 0x7_C000_0000 (31GB): +// - Encoding Range: [0 .. 0x8_0000_0000 ) (32 GB) +// - Klass Range: [0x7_C000_0000 .. 0x8_0000_0000 ) (1 GB) +// +// encoding end +// _base _klass_range_start _klass_range_end +// | |//////////////////////////////| +// | ... |///////1gb class space////////| +// | |//////////////////////////////| +// 0x0 0x7_C000_0000 0x8_0000_0000 +// +// +// CDS enabled, 128MB CDS region starts 0x8_0000_0000, followed by a 1GB class space. Encoding +// base will point to CDS region start, shift=0: +// - Encoding Range: [0x8_0000_0000 .. 0x9_0000_0000 ) (4 GB) +// - Klass Range: [0x8_0000_0000 .. 0x8_4800_0000 ) (128 MB + 1 GB) +// +// _base +// _klass_range_start _klass_range_end encoding end +// |//////////|///////////////////////////| | +// |///CDS////|////1gb class space////////| ... ... | +// |//////////|///////////////////////////| | +// | | | +// 0x8_0000_0000 0x8_4800_0000 0x9_0000_0000 +// + // If compressed klass pointers then use narrowKlass. typedef juint narrowKlass; @@ -50,12 +113,10 @@ class CompressedKlassPointers : public AllStatic { static address _base; static int _shift; - // Together with base, this defines the address range within which Klass - // structures will be located: [base, base+range). While the maximal - // possible encoding range is 4|32G for shift 0|3, if we know beforehand - // the expected range of Klass* pointers will be smaller, a platform - // could use this info to optimize encoding. - static size_t _range; + // Start and end of the Klass Range. + // Note: guaranteed to be aligned to KlassAlignmentInBytes + static address _klass_range_start; + static address _klass_range_end; // Helper function for common cases. static char* reserve_address_space_X(uintptr_t from, uintptr_t to, size_t size, size_t alignment, bool aslr); @@ -92,9 +153,13 @@ public: static void print_mode(outputStream* st); static address base() { return _base; } - static size_t range() { return _range; } static int shift() { return _shift; } + static address klass_range_start() { return _klass_range_start; } + static address klass_range_end() { return _klass_range_end; } + + static inline address encoding_range_end(); + static bool is_null(Klass* v) { return v == nullptr; } static bool is_null(narrowKlass v) { return v == 0; } @@ -110,14 +175,9 @@ public: // Returns whether the pointer is in the memory region used for encoding compressed // class pointers. This includes CDS. - - // encoding encoding - // base end (base+range) - // |-----------------------------------------------------------------------| - // |----CDS---| |--------------------class space---------------------------| - - static inline bool is_in_encoding_range(const void* p) { - return p >= _base && p < (_base + _range); + static inline bool is_encodable(const void* p) { + return (address) p >= _klass_range_start && + (address) p < _klass_range_end; } }; diff --git a/src/hotspot/share/oops/compressedKlass.inline.hpp b/src/hotspot/share/oops/compressedKlass.inline.hpp index 9cfef964f18..c9c9af24ad8 100644 --- a/src/hotspot/share/oops/compressedKlass.inline.hpp +++ b/src/hotspot/share/oops/compressedKlass.inline.hpp @@ -82,4 +82,9 @@ inline narrowKlass CompressedKlassPointers::encode(Klass* v) { return is_null(v) ? (narrowKlass)0 : encode_not_null(v); } +inline address CompressedKlassPointers::encoding_range_end() { + const int max_bits = (sizeof(narrowKlass) * BitsPerByte) + _shift; // narrowKlass are 32 bit + return _base + nth_bit(max_bits); +} + #endif // SHARE_OOPS_COMPRESSEDKLASS_INLINE_HPP diff --git a/src/hotspot/share/utilities/globalDefinitions.hpp b/src/hotspot/share/utilities/globalDefinitions.hpp index b7be6dc04bf..6d385165b8b 100644 --- a/src/hotspot/share/utilities/globalDefinitions.hpp +++ b/src/hotspot/share/utilities/globalDefinitions.hpp @@ -384,9 +384,14 @@ inline T byte_size_in_proper_unit(T s) { #define PROPERFMT SIZE_FORMAT "%s" #define PROPERFMTARGS(s) byte_size_in_proper_unit(s), proper_unit_for_byte_size(s) +// Printing a range, with start and bytes given #define RANGEFMT "[" PTR_FORMAT " - " PTR_FORMAT "), (" SIZE_FORMAT " bytes)" #define RANGEFMTARGS(p1, size) p2i(p1), p2i(p1 + size), size +// Printing a range, with start and end given +#define RANGE2FMT "[" PTR_FORMAT " - " PTR_FORMAT "), (" SIZE_FORMAT " bytes)" +#define RANGE2FMTARGS(p1, p2) p2i(p1), p2i(p2), ((uintptr_t)p2 - (uintptr_t)p1) + inline const char* exact_unit_for_byte_size(size_t s) { #ifdef _LP64 if (s >= G && (s % G) == 0) { diff --git a/test/hotspot/gtest/oops/test_compressedKlass.cpp b/test/hotspot/gtest/oops/test_compressedKlass.cpp new file mode 100644 index 00000000000..b2fc5064581 --- /dev/null +++ b/test/hotspot/gtest/oops/test_compressedKlass.cpp @@ -0,0 +1,79 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. All rights reserved. + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +#include "precompiled.hpp" +#include "oops/compressedKlass.inline.hpp" +#include "utilities/globalDefinitions.hpp" + +#include "unittest.hpp" + +TEST_VM(CompressedKlass, basics) { + if (!UseCompressedClassPointers) { + return; + } + ASSERT_LE((address)0, CompressedKlassPointers::base()); + ASSERT_LE(CompressedKlassPointers::base(), CompressedKlassPointers::klass_range_start()); + ASSERT_LT(CompressedKlassPointers::klass_range_start(), CompressedKlassPointers::klass_range_end()); + ASSERT_LE(CompressedKlassPointers::klass_range_end(), CompressedKlassPointers::encoding_range_end()); + switch (CompressedKlassPointers::shift()) { + case 0: + ASSERT_EQ(CompressedKlassPointers::encoding_range_end() - CompressedKlassPointers::base(), (ptrdiff_t)(4 * G)); + break; + case 3: + ASSERT_EQ(CompressedKlassPointers::encoding_range_end() - CompressedKlassPointers::base(), (ptrdiff_t)(32 * G)); + break; + default: + ShouldNotReachHere(); + } +} + +TEST_VM(CompressedKlass, test_too_low_address) { + if (!UseCompressedClassPointers) { + return; + } + address really_low = (address) 32; + ASSERT_FALSE(CompressedKlassPointers::is_encodable(really_low)); + address low = CompressedKlassPointers::klass_range_start() - 1; + ASSERT_FALSE(CompressedKlassPointers::is_encodable(low)); +} + +TEST_VM(CompressedKlass, test_too_high_address) { + if (!UseCompressedClassPointers) { + return; + } + address really_high = (address) UINTPTR_MAX; + ASSERT_FALSE(CompressedKlassPointers::is_encodable(really_high)); + address high = CompressedKlassPointers::klass_range_end(); + ASSERT_FALSE(CompressedKlassPointers::is_encodable(high)); +} + +TEST_VM(CompressedKlass, test_good_address) { + if (!UseCompressedClassPointers) { + return; + } + address addr = CompressedKlassPointers::klass_range_start(); + ASSERT_TRUE(CompressedKlassPointers::is_encodable(addr)); + addr = CompressedKlassPointers::klass_range_end() - 1; + ASSERT_TRUE(CompressedKlassPointers::is_encodable(addr)); +} diff --git a/test/hotspot/jtreg/gtest/CompressedKlassGtest.java b/test/hotspot/jtreg/gtest/CompressedKlassGtest.java new file mode 100644 index 00000000000..e6377a8570d --- /dev/null +++ b/test/hotspot/jtreg/gtest/CompressedKlassGtest.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. All rights reserved. + * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +/* + * This runs the "compressedKlass" class of gtests. + * Note: we try to trigger bugs by enforcing the JVM to use zero-based mode. To increase the chance of zero-based + * mode, we start with CDS disabled, a small class space and a large (albeit uncommitted, to save memory) heap. The + * JVM will likely place the class space in low-address territory. + * (If it does not manage to do this, the test will still succeed, but it won't alert us on regressions) + */ + +/* @test id=use-zero-based-encoding + * @library /test/lib + * @modules java.base/jdk.internal.misc + * java.xml + * @run main/native GTestWrapper --gtest_filter=CompressedKlass* -Xlog:metaspace* -Xmx6g -Xms128m -Xshare:off -XX:CompressedClassSpaceSize=128m + */