8145940: TempNewSymbol should have correct copy and assignment functions

Add clear() to the assignment operator and add copy constructor.

Reviewed-by: mgronlun, lfoltan, kbarrett, jrose

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, 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
@@ -667,3 +667,53 @@ int SymboltableDCmd::num_arguments() {
     return 0;
   }
 }
+
+#ifndef PRODUCT
+// Internal test of TempNewSymbol
+void Test_TempNewSymbol() {
+  // Assert messages assume these symbols are unique, and the refcounts start at
+  // one, but code does not rely on this.
+  Thread* THREAD = Thread::current();
+  Symbol* abc = SymbolTable::new_symbol("abc", CATCH);
+  int abccount = abc->refcount();
+  TempNewSymbol ss = abc;
+  assert(ss->refcount() == abccount, "only one abc");
+  assert(ss->refcount() == abc->refcount(), "should match TempNewSymbol");
+
+  Symbol* efg = SymbolTable::new_symbol("efg", CATCH);
+  Symbol* hij = SymbolTable::new_symbol("hij", CATCH);
+  int efgcount = efg->refcount();
+  int hijcount = hij->refcount();
+
+  TempNewSymbol s1 = efg;
+  TempNewSymbol s2 = hij;
+  assert(s1->refcount() == efgcount, "one efg");
+  assert(s2->refcount() == hijcount, "one hij");
+
+  // Assignment operator
+  s1 = s2;
+  assert(hij->refcount() == hijcount + 1, "should be two hij");
+  assert(efg->refcount() == efgcount - 1, "should be no efg");
+
+  s1 = ss;  // s1 is abc
+  assert(s1->refcount() == abccount + 1, "should be two abc (s1 and ss)");
+  assert(hij->refcount() == hijcount, "should only have one hij now (s2)");
+
+  s1 = s1; // self assignment
+  assert(s1->refcount() == abccount + 1, "should still be two abc (s1 and ss)");
+
+  TempNewSymbol s3;
+  Symbol* klm = SymbolTable::new_symbol("klm", CATCH);
+  int klmcount = klm->refcount();
+  s3 = klm;   // assignment
+  assert(s3->refcount() == klmcount, "only one klm now");
+
+  Symbol* xyz = SymbolTable::new_symbol("xyz", CATCH);
+  int xyzcount = xyz->refcount();
+  { // inner scope
+     TempNewSymbol s_inner = xyz;
+  }
+  assert(xyz->refcount() == (xyzcount - 1),
+         "Should have been decremented by dtor in inner scope");
+}
+#endif // PRODUCT

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, 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
@@ -42,8 +42,17 @@
 class BoolObjectClosure;
 class outputStream;
 
-// Class to hold a newly created or referenced Symbol* temporarily in scope.
-// new_symbol() and lookup() will create a Symbol* if not already in the
+// TempNewSymbol acts as a handle class in a handle/body idiom and is
+// responsible for proper resource management of the body (which is a Symbol*).
+// The body is resource managed by a reference counting scheme.
+// TempNewSymbol can therefore be used to properly hold a newly created or referenced
+// Symbol* temporarily in scope.
+//
+// Routines in SymbolTable will initialize the reference count of a Symbol* before
+// it becomes "managed" by TempNewSymbol instances. As a handle class, TempNewSymbol
+// needs to maintain proper reference counting in context of copy semantics.
+//
+// In SymbolTable, new_symbol() and lookup() will create a Symbol* if not already in the
 // symbol table and add to the symbol's reference count.
 // probe() and lookup_only() will increment the refcount if symbol is found.
 class TempNewSymbol : public StackObj {
@@ -51,25 +60,38 @@ class TempNewSymbol : public StackObj {
 
  public:
   TempNewSymbol() : _temp(NULL) {}
-  // Creating or looking up a symbol increments the symbol's reference count
+
+  // Conversion from a Symbol* to a TempNewSymbol.
+  // Does not increment the current reference count.
   TempNewSymbol(Symbol *s) : _temp(s) {}
 
-  // Operator= increments reference count.
-  void operator=(const TempNewSymbol &s) {
-    //clear();  //FIXME
-    _temp = s._temp;
-    if (_temp !=NULL) _temp->increment_refcount();
+  // Copy constructor increments reference count.
+  TempNewSymbol(const TempNewSymbol& rhs) : _temp(rhs._temp) {
+    if (_temp != NULL) {
+      _temp->increment_refcount();
+    }
   }
 
-  // Decrement reference counter so it can go away if it's unique
-  void clear() { if (_temp != NULL)  _temp->decrement_refcount();  _temp = NULL; }
+  // Assignment operator uses a c++ trick called copy and swap idiom.
+  // rhs is passed by value so within the scope of this method it is a copy.
+  // At method exit it contains the former value of _temp, triggering the correct refcount
+  // decrement upon destruction.
+  void operator=(TempNewSymbol rhs) {
+    Symbol* tmp = rhs._temp;
+    rhs._temp = _temp;
+    _temp = tmp;
+  }
 
-  ~TempNewSymbol() { clear(); }
+  // Decrement reference counter so it can go away if it's unused
+  ~TempNewSymbol() {
+    if (_temp != NULL) {
+      _temp->decrement_refcount();
+    }
+  }
 
-  // Operators so they can be used like Symbols
+  // Symbol* conversion operators
   Symbol* operator -> () const                   { return _temp; }
   bool    operator == (Symbol* o) const          { return _temp == o; }
-  // Sneaky conversion function
   operator Symbol*()                             { return _temp; }
 };
 

hotspot/src/share/vm/prims/jni.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2012 Red Hat, Inc.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
@@ -3889,6 +3889,7 @@ void Test_linked_list();
 void TestResourcehash_test();
 void TestChunkedList_test();
 void Test_log_length();
+void Test_TempNewSymbol();
 #if INCLUDE_ALL_GCS
 void TestOldFreeSpaceCalculation_test();
 void TestG1BiasedArray_test();
@@ -3932,6 +3933,7 @@ void execute_internal_vm_tests() {
     run_unit_test(JSONTest::test());
     run_unit_test(Test_log_length());
     run_unit_test(DirectivesParser::test());
+    run_unit_test(Test_TempNewSymbol());
 #if INCLUDE_VM_STRUCTS
     run_unit_test(VMStructs::test());
 #endif