8256725: Metaspace: better blocktree and binlist asserts

Reviewed-by: shade, rrich, lkorinth

Author: tstuefe

src/hotspot/share/memory/metaspace/binList.hpp

@@ -85,6 +85,9 @@ class BinListImpl {
     {}
   };
 
+#define BLOCK_FORMAT          "Block @" PTR_FORMAT ": size: " SIZE_FORMAT ", next: " PTR_FORMAT
+#define BLOCK_FORMAT_ARGS(b)  p2i(b), (b)->_word_size, p2i((b)->_next)
+
   // Smallest block size must be large enough to hold a Block structure.
   STATIC_ASSERT(smallest_word_size * sizeof(MetaWord) >= sizeof(Block));
   STATIC_ASSERT(num_lists > 0);
@@ -150,19 +153,16 @@ class BinListImpl {
     int index = index_for_word_size(word_size);
     index = index_for_next_non_empty_list(index);
     if (index != -1) {
-      assert(_blocks[index] != NULL &&
-             _blocks[index]->_word_size >= word_size, "sanity");
-
-      MetaWord* const p = (MetaWord*)_blocks[index];
+      Block* b = _blocks[index];
       const size_t real_word_size = word_size_for_index(index);
-
-      _blocks[index] = _blocks[index]->_next;
-
+      assert(b != NULL, "Sanity");
+      assert(b->_word_size >= word_size &&
+             b->_word_size == real_word_size,
+             "bad block size in list[%d] (" BLOCK_FORMAT ")", index, BLOCK_FORMAT_ARGS(b));
+      _blocks[index] = b->_next;
       _counter.sub(real_word_size);
       *p_real_word_size = real_word_size;
-
-      return p;
-
+      return (MetaWord*)b;
     } else {
       *p_real_word_size = 0;
       return NULL;
@@ -182,8 +182,11 @@ class BinListImpl {
     MemRangeCounter local_counter;
     for (int i = 0; i < num_lists; i++) {
       const size_t s = MinWordSize + i;
-      for (Block* b = _blocks[i]; b != NULL; b = b->_next) {
-        assert(b->_word_size == s, "bad block size");
+      int pos = 0;
+      for (Block* b = _blocks[i]; b != NULL; b = b->_next, pos++) {
+        assert(b->_word_size == s,
+               "bad block size in list[%d] at pos %d (" BLOCK_FORMAT ")",
+               i, pos, BLOCK_FORMAT_ARGS(b));
         local_counter.add(s);
       }
     }

src/hotspot/share/memory/metaspace/blockTree.cpp

@@ -24,6 +24,7 @@
  */
 
 #include "precompiled.hpp"
+#include "memory/metaspace/chunklevel.hpp"
 #include "memory/metaspace/blockTree.hpp"
 #include "memory/resourceArea.hpp"
 #include "utilities/debug.hpp"
@@ -36,27 +37,43 @@ namespace metaspace {
 // Needed to prevent linker errors on MacOS and AIX
 const size_t BlockTree::MinWordSize;
 
+#define NODE_FORMAT \
+  "@" PTR_FORMAT \
+  ": canary " INTPTR_FORMAT \
+  ", parent " PTR_FORMAT \
+  ", left " PTR_FORMAT \
+  ", right " PTR_FORMAT \
+  ", next " PTR_FORMAT \
+  ", size " SIZE_FORMAT
+
+#define NODE_FORMAT_ARGS(n) \
+  p2i(n), \
+  (n)->_canary, \
+  p2i((n)->_parent), \
+  p2i((n)->_left), \
+  p2i((n)->_right), \
+  p2i((n)->_next), \
+  (n)->_word_size
+
 #ifdef ASSERT
 
 // Tree verification
 
-// These asserts prints the tree, then asserts
-#define assrt(cond, format, ...) \
+// This assert prints the tree too
+#define tree_assert(cond, format, ...) \
   do { \
     if (!(cond)) { \
+      tty->print("Error in tree @" PTR_FORMAT ": ", p2i(this)); \
+      tty->print_cr(format, __VA_ARGS__); \
+      tty->print_cr("Tree:"); \
       print_tree(tty); \
       assert(cond, format, __VA_ARGS__); \
     } \
   } while (0)
 
-  // This assert prints the tree, then stops (generic message)
-#define assrt0(cond) \
-  do { \
-    if (!(cond)) { \
-      print_tree(tty); \
-      assert(cond, "sanity"); \
-    } \
-  } while (0)
+// Assert, prints tree and specific given node
+#define tree_assert_invalid_node(cond, failure_node) \
+  tree_assert(cond, "Invalid node: " NODE_FORMAT, NODE_FORMAT_ARGS(failure_node))
 
 // walkinfo keeps a node plus the size corridor it and its children
 //  are supposed to be in.
@@ -67,12 +84,24 @@ struct BlockTree::walkinfo {
   size_t lim2; // )
 };
 
+// Helper for verify()
+void BlockTree::verify_node_pointer(const Node* n) const {
+  tree_assert(os::is_readable_pointer(n),
+              "Invalid node: @" PTR_FORMAT " is unreadable.", p2i(n));
+  // If the canary is broken, this is either an invalid node pointer or
+  // the node has been overwritten. Either way, print a hex dump, then
+  // assert away.
+  if (n->_canary != Node::_canary_value) {
+    os::print_hex_dump(tty, (address)n, (address)n + sizeof(Node), 1);
+    tree_assert(false, "Invalid node: @" PTR_FORMAT " canary broken or pointer invalid", p2i(n));
+  }
+}
+
 void BlockTree::verify() const {
   // Traverse the tree and test that all nodes are in the correct order.
 
   MemRangeCounter counter;
   int longest_edge = 0;
-
   if (_root != NULL) {
 
     ResourceMark rm;
@@ -90,27 +119,29 @@ void BlockTree::verify() const {
       info = stack.pop();
       const Node* n = info.n;
 
+      verify_node_pointer(n);
+
       // Assume a (ridiculously large) edge limit to catch cases
       //  of badly degenerated or circular trees.
-      assrt0(info.depth < 10000);
+      tree_assert(info.depth < 10000, "too deep (%d)", info.depth);
       counter.add(n->_word_size);
 
-      // Verify node.
       if (n == _root) {
-        assrt0(n->_parent == NULL);
+        tree_assert_invalid_node(n->_parent == NULL, n);
       } else {
-        assrt0(n->_parent != NULL);
+        tree_assert_invalid_node(n->_parent != NULL, n);
       }
 
       // check size and ordering
-      assrt(n->_word_size >= MinWordSize, "bad node size " SIZE_FORMAT, n->_word_size);
-      assrt0(n->_word_size > info.lim1);
-      assrt0(n->_word_size < info.lim2);
+      tree_assert_invalid_node(n->_word_size >= MinWordSize, n);
+      tree_assert_invalid_node(n->_word_size <= chunklevel::MAX_CHUNK_WORD_SIZE, n);
+      tree_assert_invalid_node(n->_word_size > info.lim1, n);
+      tree_assert_invalid_node(n->_word_size < info.lim2, n);
 
       // Check children
       if (n->_left != NULL) {
-        assrt0(n->_left != n);
-        assrt0(n->_left->_parent == n);
+        tree_assert_invalid_node(n->_left != n, n);
+        tree_assert_invalid_node(n->_left->_parent == n, n);
 
         walkinfo info2;
         info2.n = n->_left;
@@ -121,8 +152,8 @@ void BlockTree::verify() const {
       }
 
       if (n->_right != NULL) {
-        assrt0(n->_right != n);
-        assrt0(n->_right->_parent == n);
+        tree_assert_invalid_node(n->_right != n, n);
+        tree_assert_invalid_node(n->_right->_parent == n, n);
 
         walkinfo info2;
         info2.n = n->_right;
@@ -135,26 +166,33 @@ void BlockTree::verify() const {
       // If node has same-sized siblings check those too.
       const Node* n2 = n->_next;
       while (n2 != NULL) {
-        assrt0(n2 != n);
-        assrt0(n2->_word_size == n->_word_size);
+        verify_node_pointer(n2);
+        tree_assert_invalid_node(n2 != n, n2); // catch simple circles
+        tree_assert_invalid_node(n2->_word_size == n->_word_size, n2);
         counter.add(n2->_word_size);
         n2 = n2->_next;
       }
     }
   }
 
   // At the end, check that counters match
+  // (which also verifies that we visited every node, or at least
+  //  as many nodes as are in this tree)
   _counter.check(counter);
+
+  #undef assrt0n
 }
 
 void BlockTree::zap_range(MetaWord* p, size_t word_size) {
   memset(p, 0xF3, word_size * sizeof(MetaWord));
 }
 
-#undef assrt
-#undef assrt0
-
 void BlockTree::print_tree(outputStream* st) const {
+
+  // Note: we do not print the tree indented, since I found that printing it
+  //  as a quasi list is much clearer to the eye.
+  // We print the tree depth-first, with stacked nodes below normal ones
+  //  (normal "real" nodes are marked with a leading '+')
   if (_root != NULL) {
 
     ResourceMark rm;
@@ -168,11 +206,27 @@ void BlockTree::print_tree(outputStream* st) const {
     while (stack.length() > 0) {
       info = stack.pop();
       const Node* n = info.n;
+
       // Print node.
-      for (int i = 0; i < info.depth; i++) {
-         st->print("---");
+      st->print("%4d + ", info.depth);
+      if (os::is_readable_pointer(n)) {
+        st->print_cr(NODE_FORMAT, NODE_FORMAT_ARGS(n));
+      } else {
+        st->print_cr("@" PTR_FORMAT ": unreadable (skipping subtree)", p2i(n));
+        continue; // don't print this subtree
+      }
+
+      // Print same-sized-nodes stacked under this node
+      for (Node* n2 = n->_next; n2 != NULL; n2 = n2->_next) {
+        st->print_raw("       ");
+        if (os::is_readable_pointer(n2)) {
+          st->print_cr(NODE_FORMAT, NODE_FORMAT_ARGS(n2));
+        } else {
+          st->print_cr("@" PTR_FORMAT ": unreadable (skipping rest of chain).", p2i(n2));
+          break; // stop printing this chain.
+        }
       }
-      st->print_cr("<" PTR_FORMAT " (size " SIZE_FORMAT ")", p2i(n), n->_word_size);
+
       // Handle children.
       if (n->_right != NULL) {
         walkinfo info2;

src/hotspot/share/memory/metaspace/blockTree.hpp

@@ -77,7 +77,18 @@ class BlockTree: public CHeapObj<mtMetaspace> {
 
   struct Node {
 
+    static const intptr_t _canary_value =
+        NOT_LP64(0x4e4f4445) LP64_ONLY(0x4e4f44454e4f4445ULL); // "NODE" resp "NODENODE"
+
+    // Note: we afford us the luxury of an always-there canary value.
+    //  The space for that is there (these nodes are only used to manage larger blocks,
+    //  see FreeBlocks::MaxSmallBlocksWordSize).
+    //  It is initialized in debug and release, but only automatically tested
+    //  in debug.
+    const intptr_t _canary;
+
     // Normal tree node stuff...
+    //  (Note: all null if this is a stacked node)
     Node* _parent;
     Node* _left;
     Node* _right;
@@ -91,18 +102,31 @@ class BlockTree: public CHeapObj<mtMetaspace> {
     const size_t _word_size;
 
     Node(size_t word_size) :
+      _canary(_canary_value),
       _parent(NULL),
       _left(NULL),
       _right(NULL),
       _next(NULL),
       _word_size(word_size)
     {}
 
+#ifdef ASSERT
+    bool valid() const {
+      return _canary == _canary_value &&
+        _word_size >= sizeof(Node) &&
+        _word_size < chunklevel::MAX_CHUNK_WORD_SIZE;
+    }
+#endif
   };
 
   // Needed for verify() and print_tree()
   struct walkinfo;
 
+#ifdef ASSERT
+  // Run a quick check on a node; upon suspicion dive into a full tree check.
+  void check_node(const Node* n) const { if (!n->valid()) verify(); }
+#endif
+
 public:
 
   // Minimum word size a block has to be to be added to this structure (note ceil division).
@@ -196,9 +220,10 @@ class BlockTree: public CHeapObj<mtMetaspace> {
   }
 
   // Given a node n and an insertion point, insert n under insertion point.
-  static void insert(Node* insertion_point, Node* n) {
+  void insert(Node* insertion_point, Node* n) {
     assert(n->_parent == NULL, "Sanity");
     for (;;) {
+      DEBUG_ONLY(check_node(insertion_point);)
       if (n->_word_size == insertion_point->_word_size) {
         add_to_list(n, insertion_point); // parent stays NULL in this case.
         break;
@@ -222,9 +247,10 @@ class BlockTree: public CHeapObj<mtMetaspace> {
 
   // Given a node and a wish size, search this node and all children for
   // the node closest (equal or larger sized) to the size s.
-  static Node* find_closest_fit(Node* n, size_t s) {
+  Node* find_closest_fit(Node* n, size_t s) {
     Node* best_match = NULL;
     while (n != NULL) {
+      DEBUG_ONLY(check_node(n);)
       if (n->_word_size >= s) {
         best_match = n;
         if (n->_word_size == s) {
@@ -311,6 +337,8 @@ class BlockTree: public CHeapObj<mtMetaspace> {
 
 #ifdef ASSERT
   void zap_range(MetaWord* p, size_t word_size);
+  // Helper for verify()
+  void verify_node_pointer(const Node* n) const;
 #endif // ASSERT
 
 public:
@@ -339,6 +367,7 @@ class BlockTree: public CHeapObj<mtMetaspace> {
     Node* n = find_closest_fit(word_size);
 
     if (n != NULL) {
+      DEBUG_ONLY(check_node(n);)
       assert(n->_word_size >= word_size, "sanity");
 
       if (n->_next != NULL) {

src/hotspot/share/memory/metaspace/freeBlocks.hpp

@@ -69,6 +69,10 @@ class FreeBlocks : public CHeapObj<mtMetaspace> {
   // to fit into _smallblocks.
   BlockTree _tree;
 
+  // This verifies that blocks too large to go into the binlist can be
+  // kept in the blocktree.
+  STATIC_ASSERT(BinList32::MaxWordSize >= BlockTree::MinWordSize);
+
   // Cutoff point: blocks larger than this size are kept in the
   // tree, blocks smaller than or equal to this size in the bin list.
   const size_t MaxSmallBlocksWordSize = BinList32::MaxWordSize;

test/hotspot/gtest/metaspace/test_blocktree.cpp

@@ -215,7 +215,33 @@ TEST_VM(metaspace, BlockTree_print_test) {
   bt.print_tree(&ss);
 
   LOG("%s", ss.as_string());
+}
+
+// Test that an overwritten node would result in an assert and a printed tree
+TEST_VM_ASSERT_MSG(metaspace, BlockTree_overwriter_test, "Invalid node") {
+  static const size_t sizes1[] = { 30, 17, 0 };
+  static const size_t sizes2[] = { 12, 12, 0 };
+
+  BlockTree bt;
+  FeederBuffer fb(4 * K);
+
+  // some nodes...
+  create_nodes(sizes1, fb, bt);
+
+  // a node we will break...
+  MetaWord* p_broken = fb.get(12);
+  bt.add_block(p_broken, 12);
+
+  // some more nodes...
+  create_nodes(sizes2, fb, bt);
 
+  // overwrite node memory (only the very first byte), then verify tree.
+  // Verification should catch the broken canary, print the tree,
+  // then assert.
+  LOG("Will break node at " PTR_FORMAT ".", p2i(p_broken));
+  tty->print_cr("Death test, please ignore the following \"Invalid node\" printout.");
+  *((char*)p_broken) = '\0';
+  bt.verify();
 }
 #endif