7178670: runtime/7158800/BadUtf8.java fails in SymbolTable::rehash_table

Cannot delete _buckets and HashtableEntries in shared space (CDS)

Reviewed-by: acorn, kvn, dlong, dcubed, kamg

hotspot/src/share/vm/classfile/symbolTable.cpp

@@ -46,11 +46,8 @@ bool SymbolTable::_needs_rehashing = false;
 jint SymbolTable::_seed = 0;
 
 Symbol* SymbolTable::allocate_symbol(const u1* name, int len, bool c_heap, TRAPS) {
-  // Don't allow symbols to be created which cannot fit in a Symbol*.
+  assert (len <= Symbol::max_length(), "should be checked by caller");
-  if (len > Symbol::max_length()) {
+
-    THROW_MSG_0(vmSymbols::java_lang_InternalError(),
-                "name is too long to represent");
-  }
   Symbol* sym;
   // Allocate symbols in the C heap when dumping shared spaces in case there
   // are temporary symbols we can remove.
@@ -95,9 +92,14 @@ void SymbolTable::unlink() {
   int total = 0;
   size_t memory_total = 0;
   for (int i = 0; i < the_table()->table_size(); ++i) {
-    for (HashtableEntry<Symbol*>** p = the_table()->bucket_addr(i); *p != NULL; ) {
+    HashtableEntry<Symbol*>** p = the_table()->bucket_addr(i);
-      HashtableEntry<Symbol*>* entry = *p;
+    HashtableEntry<Symbol*>* entry = the_table()->bucket(i);
-      if (entry->is_shared()) {
+    while (entry != NULL) {
+      // Shared entries are normally at the end of the bucket and if we run into
+      // a shared entry, then there is nothing more to remove. However, if we
+      // have rehashed the table, then the shared entries are no longer at the
+      // end of the bucket.
+      if (entry->is_shared() && !use_alternate_hashcode()) {
         break;
       }
       Symbol* s = entry->literal();
@@ -106,6 +108,7 @@ void SymbolTable::unlink() {
       assert(s != NULL, "just checking");
       // If reference count is zero, remove.
       if (s->refcount() == 0) {
+        assert(!entry->is_shared(), "shared entries should be kept live");
         delete s;
         removed++;
         *p = entry->next();
@@ -113,6 +116,8 @@ void SymbolTable::unlink() {
       } else {
         p = entry->next_addr();
       }
+      // get next entry
+      entry = (HashtableEntry<Symbol*>*)HashtableEntry<Symbol*>::make_ptr(*p);
     }
   }
   symbols_removed += removed;
@@ -135,7 +140,8 @@ unsigned int SymbolTable::new_hash(Symbol* sym) {
 // with the existing strings.   Set flag to use the alternate hash code afterwards.
 void SymbolTable::rehash_table() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
-  assert(!DumpSharedSpaces, "this should never happen with -Xshare:dump");
+  // This should never happen with -Xshare:dump but it might in testing mode.
+  if (DumpSharedSpaces) return;
   // Create a new symbol table
   SymbolTable* new_table = new SymbolTable();
 
@@ -176,12 +182,11 @@ Symbol* SymbolTable::lookup(int index, const char* name,
   return NULL;
 }
 
-// Pick hashing algorithm, but return value already given if not using a new
+// Pick hashing algorithm.
-// hash algorithm.
+unsigned int SymbolTable::hash_symbol(const char* s, int len) {
-unsigned int SymbolTable::hash_symbol(const char* s, int len, unsigned int hashValue) {
   return use_alternate_hashcode() ?
            AltHashing::murmur3_32(seed(), (const jbyte*)s, len) :
-           (hashValue != 0 ? hashValue : java_lang_String::to_hash(s, len));
+           java_lang_String::to_hash(s, len);
 }
 
 
@@ -201,6 +206,9 @@ Symbol* SymbolTable::lookup(const char* name, int len, TRAPS) {
   // Found
   if (s != NULL) return s;
 
+  // Grab SymbolTable_lock first.
+  MutexLocker ml(SymbolTable_lock, THREAD);
+
   // Otherwise, add to symbol to table
   return the_table()->basic_add(index, (u1*)name, len, hashValue, true, CHECK_NULL);
 }
@@ -238,6 +246,9 @@ Symbol* SymbolTable::lookup(const Symbol* sym, int begin, int end, TRAPS) {
   // We can't include the code in No_Safepoint_Verifier because of the
   // ResourceMark.
 
+  // Grab SymbolTable_lock first.
+  MutexLocker ml(SymbolTable_lock, THREAD);
+
   return the_table()->basic_add(index, (u1*)buffer, len, hashValue, true, CHECK_NULL);
 }
 
@@ -306,6 +317,9 @@ void SymbolTable::add(Handle class_loader, constantPoolHandle cp,
                       int names_count,
                       const char** names, int* lengths, int* cp_indices,
                       unsigned int* hashValues, TRAPS) {
+  // Grab SymbolTable_lock first.
+  MutexLocker ml(SymbolTable_lock, THREAD);
+
   SymbolTable* table = the_table();
   bool added = table->basic_add(class_loader, cp, names_count, names, lengths,
                                 cp_indices, hashValues, CHECK);
@@ -326,22 +340,39 @@ Symbol* SymbolTable::new_permanent_symbol(const char* name, TRAPS) {
   if (result != NULL) {
     return result;
   }
+  // Grab SymbolTable_lock first.
+  MutexLocker ml(SymbolTable_lock, THREAD);
+
   SymbolTable* table = the_table();
   int index = table->hash_to_index(hash);
   return table->basic_add(index, (u1*)name, (int)strlen(name), hash, false, THREAD);
 }
 
-Symbol* SymbolTable::basic_add(int index, u1 *name, int len,
+Symbol* SymbolTable::basic_add(int index_arg, u1 *name, int len,
                                unsigned int hashValue_arg, bool c_heap, TRAPS) {
   assert(!Universe::heap()->is_in_reserved(name) || GC_locker::is_active(),
          "proposed name of symbol must be stable");
 
-  // Grab SymbolTable_lock first.
+  // Don't allow symbols to be created which cannot fit in a Symbol*.
-  MutexLocker ml(SymbolTable_lock, THREAD);
+  if (len > Symbol::max_length()) {
+    THROW_MSG_0(vmSymbols::java_lang_InternalError(),
+                "name is too long to represent");
+  }
+
+  // Cannot hit a safepoint in this function because the "this" pointer can move.
+  No_Safepoint_Verifier nsv;
 
   // Check if the symbol table has been rehashed, if so, need to recalculate
-  // the hash value.
+  // the hash value and index.
-  unsigned int hashValue = hash_symbol((const char*)name, len, hashValue_arg);
+  unsigned int hashValue;
+  int index;
+  if (use_alternate_hashcode()) {
+    hashValue = hash_symbol((const char*)name, len);
+    index = hash_to_index(hashValue);
+  } else {
+    hashValue = hashValue_arg;
+    index = index_arg;
+  }
 
   // Since look-up was done lock-free, we need to check if another
   // thread beat us in the race to insert the symbol.
@@ -377,13 +408,18 @@ bool SymbolTable::basic_add(Handle class_loader, constantPoolHandle cp,
     }
   }
 
-  // Hold SymbolTable_lock through the symbol creation
+  // Cannot hit a safepoint in this function because the "this" pointer can move.
-  MutexLocker ml(SymbolTable_lock, THREAD);
+  No_Safepoint_Verifier nsv;
 
   for (int i=0; i<names_count; i++) {
     // Check if the symbol table has been rehashed, if so, need to recalculate
     // the hash value.
-    unsigned int hashValue = hash_symbol(names[i], lengths[i], hashValues[i]);
+    unsigned int hashValue;
+    if (use_alternate_hashcode()) {
+      hashValue = hash_symbol(names[i], lengths[i]);
+    } else {
+      hashValue = hashValues[i];
+    }
     // Since look-up was done lock-free, we need to check if another
     // thread beat us in the race to insert the symbol.
     int index = hash_to_index(hashValue);
@@ -587,9 +623,9 @@ bool StringTable::_needs_rehashing = false;
 jint StringTable::_seed = 0;
 
 // Pick hashing algorithm
-unsigned int StringTable::hash_string(const jchar* s, int len, unsigned int hashValue) {
+unsigned int StringTable::hash_string(const jchar* s, int len) {
   return use_alternate_hashcode() ? AltHashing::murmur3_32(seed(), s, len) :
-            (hashValue != 0 ? hashValue : java_lang_String::to_hash(s, len));
+                                    java_lang_String::to_hash(s, len);
 }
 
 oop StringTable::lookup(int index, jchar* name,
@@ -611,29 +647,25 @@ oop StringTable::lookup(int index, jchar* name,
 }
 
 
-oop StringTable::basic_add(int index, Handle string_or_null, jchar* name,
+oop StringTable::basic_add(int index_arg, Handle string, jchar* name,
                            int len, unsigned int hashValue_arg, TRAPS) {
-  debug_only(StableMemoryChecker smc(name, len * sizeof(name[0])));
-  assert(!Universe::heap()->is_in_reserved(name) || GC_locker::is_active(),
-         "proposed name of symbol must be stable");
-
-  Handle string;
-  // try to reuse the string if possible
-  if (!string_or_null.is_null() && (!JavaObjectsInPerm || string_or_null()->is_perm())) {
-    string = string_or_null;
-  } else {
-    string = java_lang_String::create_tenured_from_unicode(name, len, CHECK_NULL);
-  }
-
-  // Allocation must be done before grapping the SymbolTable_lock lock
-  MutexLocker ml(StringTable_lock, THREAD);
 
   assert(java_lang_String::equals(string(), name, len),
          "string must be properly initialized");
+  // Cannot hit a safepoint in this function because the "this" pointer can move.
+  No_Safepoint_Verifier nsv;
 
   // Check if the symbol table has been rehashed, if so, need to recalculate
-  // the hash value before second lookup.
+  // the hash value and index before second lookup.
-  unsigned int hashValue = hash_string(name, len, hashValue_arg);
+  unsigned int hashValue;
+  int index;
+  if (use_alternate_hashcode()) {
+    hashValue = hash_string(name, len);
+    index = hash_to_index(hashValue);
+  } else {
+    hashValue = hashValue_arg;
+    index = index_arg;
+  }
 
   // Since look-up was done lock-free, we need to check if another
   // thread beat us in the race to insert the symbol.
@@ -664,13 +696,29 @@ oop StringTable::intern(Handle string_or_null, jchar* name,
                         int len, TRAPS) {
   unsigned int hashValue = hash_string(name, len);
   int index = the_table()->hash_to_index(hashValue);
-  oop string = the_table()->lookup(index, name, len, hashValue);
+  oop found_string = the_table()->lookup(index, name, len, hashValue);
 
   // Found
-  if (string != NULL) return string;
+  if (found_string != NULL) return found_string;
+
+  debug_only(StableMemoryChecker smc(name, len * sizeof(name[0])));
+  assert(!Universe::heap()->is_in_reserved(name) || GC_locker::is_active(),
+         "proposed name of symbol must be stable");
+
+  Handle string;
+  // try to reuse the string if possible
+  if (!string_or_null.is_null() && (!JavaObjectsInPerm || string_or_null()->is_perm())) {
+    string = string_or_null;
+  } else {
+    string = java_lang_String::create_tenured_from_unicode(name, len, CHECK_NULL);
+  }
+
+  // Grab the StringTable_lock before getting the_table() because it could
+  // change at safepoint.
+  MutexLocker ml(StringTable_lock, THREAD);
 
   // Otherwise, add to symbol to table
-  return the_table()->basic_add(index, string_or_null, name, len,
+  return the_table()->basic_add(index, string, name, len,
                                 hashValue, CHECK_NULL);
 }
 
@@ -713,18 +761,24 @@ void StringTable::unlink(BoolObjectClosure* is_alive) {
   // entries at a safepoint.
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
   for (int i = 0; i < the_table()->table_size(); ++i) {
-    for (HashtableEntry<oop>** p = the_table()->bucket_addr(i); *p != NULL; ) {
+    HashtableEntry<oop>** p = the_table()->bucket_addr(i);
-      HashtableEntry<oop>* entry = *p;
+    HashtableEntry<oop>* entry = the_table()->bucket(i);
-      if (entry->is_shared()) {
+    while (entry != NULL) {
+      // Shared entries are normally at the end of the bucket and if we run into
+      // a shared entry, then there is nothing more to remove. However, if we
+      // have rehashed the table, then the shared entries are no longer at the
+      // end of the bucket.
+      if (entry->is_shared() && !use_alternate_hashcode()) {
         break;
       }
       assert(entry->literal() != NULL, "just checking");
-      if (is_alive->do_object_b(entry->literal())) {
+      if (entry->is_shared() || is_alive->do_object_b(entry->literal())) {
         p = entry->next_addr();
       } else {
         *p = entry->next();
         the_table()->free_entry(entry);
       }
+      entry = (HashtableEntry<oop>*)HashtableEntry<oop>::make_ptr(*p);
     }
   }
 }
@@ -795,7 +849,8 @@ unsigned int StringTable::new_hash(oop string) {
 // with the existing strings.   Set flag to use the alternate hash code afterwards.
 void StringTable::rehash_table() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
-  assert(!DumpSharedSpaces, "this should never happen with -Xshare:dump");
+  // This should never happen with -Xshare:dump but it might in testing mode.
+  if (DumpSharedSpaces) return;
   StringTable* new_table = new StringTable();
 
   // Initialize new global seed for hashing.

hotspot/src/share/vm/classfile/symbolTable.hpp

@@ -156,7 +156,7 @@ public:
     initialize_symbols();
   }
 
-  static unsigned int hash_symbol(const char* s, int len, unsigned int hashValue = 0);
+  static unsigned int hash_symbol(const char* s, int len);
 
   static Symbol* lookup(const char* name, int len, TRAPS);
   // lookup only, won't add. Also calculate hash.
@@ -294,7 +294,7 @@ public:
   // Hashing algorithm, used as the hash value used by the
   //     StringTable for bucket selection and comparison (stored in the
   //     HashtableEntry structures).  This is used in the String.intern() method.
-  static unsigned int hash_string(const jchar* s, int len, unsigned int hashValue = 0);
+  static unsigned int hash_string(const jchar* s, int len);
 
   // Internal test.
   static void test_alt_hash() PRODUCT_RETURN;

hotspot/src/share/vm/utilities/hashtable.cpp

@@ -24,6 +24,7 @@
 
 #include "precompiled.hpp"
 #include "memory/allocation.inline.hpp"
+#include "memory/filemap.hpp"
 #include "memory/resourceArea.hpp"
 #include "oops/oop.inline.hpp"
 #include "runtime/safepoint.hpp"
@@ -119,8 +120,16 @@ template <class T> void Hashtable<T>::move_to(Hashtable<T>* new_table) {
       // Get a new index relative to the new table (can also change size)
       int index = new_table->hash_to_index(hashValue);
       p->set_hash(hashValue);
+      // Keep the shared bit in the Hashtable entry to indicate that this entry
+      // can't be deleted.   The shared bit is the LSB in the _next field so
+      // walking the hashtable past these entries requires
+      // BasicHashtableEntry::make_ptr() call.
+      bool keep_shared = p->is_shared();
       unlink_entry(p);
       new_table->add_entry(index, p);
+      if (keep_shared) {
+        p->set_shared();
+      }
       p = next;
     }
   }
@@ -135,6 +144,19 @@ template <class T> void Hashtable<T>::move_to(Hashtable<T>* new_table) {
   free_buckets();
 }
 
+void BasicHashtable::free_buckets() {
+  if (NULL != _buckets) {
+    // Don't delete the buckets in the shared space.  They aren't
+    // allocated by os::malloc
+    if (!UseSharedSpaces ||
+        !FileMapInfo::current_info()->is_in_shared_space(_buckets)) {
+       FREE_C_HEAP_ARRAY(HashtableBucket, _buckets);
+    }
+    _buckets = NULL;
+  }
+}
+
+
 // Reverse the order of elements in the hash buckets.
 
 void BasicHashtable::reverse() {

hotspot/src/share/vm/utilities/hashtable.hpp

@@ -217,12 +217,7 @@ protected:
   }
 
   // Free the buckets in this hashtable
-  void free_buckets() {
+  void free_buckets();
-    if (NULL != _buckets) {
-      FREE_C_HEAP_ARRAY(HashtableBucket, _buckets);
-      _buckets = NULL;
-    }
-  }
 
 public:
   int table_size() { return _table_size; }