8340184: Bug in CompressedKlassPointers::is_in_encodable_range

Reviewed-by: coleenp, rkennke, jsjolen

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;
 }

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 =
+    const size_t range = CompressedKlassPointers::klass_range_end() - CompressedKlassPointers::base();
-      (1ULL << log2i(CompressedKlassPointers::range())) - 1;
+    const uint64_t range_mask = (1ULL << log2i(range)) - 1;
     if (((uint64_t)CompressedKlassPointers::base() & range_mask) == 0) {
       return (_klass_decode_mode = KlassDecodeXor);
     }

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;
   }

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;
 }
 

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, "
+  if (UseCompressedClassPointers) {
-               "Narrow klass range: " SIZE_FORMAT_X, p2i(base()), shift(),
+    st->print_cr("Narrow klass base: " PTR_FORMAT ", Narrow klass shift: %d",
-               range());
+                  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

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:
+//   [ <encoding base> ... <encoding base> + (1 << (<narrowKlass-bitsize> + <shift>) )
+//
+// 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
+  // Start and end of the Klass Range.
-  //  structures will be located: [base, base+range). While the maximal
+  // Note: guaranteed to be aligned to KlassAlignmentInBytes
-  //  possible encoding range is 4|32G for shift 0|3, if we know beforehand
+  static address _klass_range_start;
-  //  the expected range of Klass* pointers will be smaller, a platform
+  static address _klass_range_end;
-  //  could use this info to optimize encoding.
-  static size_t _range;
 
   // 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.
-
+  static inline bool is_encodable(const void* p) {
-  // encoding                                                               encoding
+    return (address) p >= _klass_range_start &&
-  // base                                                                   end (base+range)
+           (address) p < _klass_range_end;
-  // |-----------------------------------------------------------------------|
-  // |----CDS---| |--------------------class space---------------------------|
-
-  static inline bool is_in_encoding_range(const void* p) {
-    return p >= _base && p < (_base + _range);
   }
 };
 

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

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

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));
+}

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
+ */