# HG changeset patch
# User drchase
# Date 1409355949 14400
# Node ID 094cbdffa87d5f8eae557c9f8cb97bda794a4ec4
# Parent  4d8781a35525e4834f119625c720d4097594a8ea
8054292: code comments leak in fastdebug builds
Summary: Added deallocation to destructor; hardened interface against misuse
Reviewed-by: kvn

diff -r 4d8781a35525 -r 094cbdffa87d src/share/vm/asm/codeBuffer.cpp
--- a/src/share/vm/asm/codeBuffer.cpp	Wed Sep 03 15:26:06 2014 +0400
+++ b/src/share/vm/asm/codeBuffer.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -133,6 +133,10 @@
   // free any overflow storage
   delete _overflow_arena;
 
+  // Claim is that stack allocation ensures resources are cleaned up.
+  // This is resource clean up, let's hope that all were properly copied out.
+  free_strings();
+
 #ifdef ASSERT
   // Save allocation type to execute assert in ~ResourceObj()
   // which is called after this destructor.
@@ -704,7 +708,7 @@
   relocate_code_to(&dest);
 
   // transfer strings and comments from buffer to blob
-  dest_blob->set_strings(_strings);
+  dest_blob->set_strings(_code_strings);
 
   // Done moving code bytes; were they the right size?
   assert(round_to(dest.total_content_size(), oopSize) == dest_blob->content_size(), "sanity");
@@ -1003,11 +1007,11 @@
 
 
 void CodeBuffer::block_comment(intptr_t offset, const char * comment) {
-  _strings.add_comment(offset, comment);
+  _code_strings.add_comment(offset, comment);
 }
 
 const char* CodeBuffer::code_string(const char* str) {
-  return _strings.add_string(str);
+  return _code_strings.add_string(str);
 }
 
 class CodeString: public CHeapObj<mtCode> {
@@ -1073,6 +1077,7 @@
 }
 
 void CodeStrings::add_comment(intptr_t offset, const char * comment) {
+  check_valid();
   CodeString* c      = new CodeString(comment, offset);
   CodeString* inspos = (_strings == NULL) ? NULL : find_last(offset);
 
@@ -1088,11 +1093,32 @@
 }
 
 void CodeStrings::assign(CodeStrings& other) {
+  other.check_valid();
+  // Cannot do following because CodeStrings constructor is not alway run!
+  assert(is_null(), "Cannot assign onto non-empty CodeStrings");
   _strings = other._strings;
+  other.set_null_and_invalidate();
+}
+
+// Deep copy of CodeStrings for consistent memory management.
+// Only used for actual disassembly so this is cheaper than reference counting
+// for the "normal" fastdebug case.
+void CodeStrings::copy(CodeStrings& other) {
+  other.check_valid();
+  check_valid();
+  assert(is_null(), "Cannot copy onto non-empty CodeStrings");
+  CodeString* n = other._strings;
+  CodeString** ps = &_strings;
+  while (n != NULL) {
+    *ps = new CodeString(n->string(),n->offset());
+    ps = &((*ps)->_next);
+    n = n->next();
+  }
 }
 
 void CodeStrings::print_block_comment(outputStream* stream, intptr_t offset) const {
-  if (_strings != NULL) {
+    check_valid();
+    if (_strings != NULL) {
     CodeString* c = find(offset);
     while (c && c->offset() == offset) {
       stream->bol();
@@ -1103,7 +1129,7 @@
   }
 }
 
-
+// Also sets isNull()
 void CodeStrings::free() {
   CodeString* n = _strings;
   while (n) {
@@ -1113,10 +1139,11 @@
     delete n;
     n = p;
   }
-  _strings = NULL;
+  set_null_and_invalidate();
 }
 
 const char* CodeStrings::add_string(const char * string) {
+  check_valid();
   CodeString* s = new CodeString(string);
   s->set_next(_strings);
   _strings = s;
diff -r 4d8781a35525 -r 094cbdffa87d src/share/vm/asm/codeBuffer.hpp
--- a/src/share/vm/asm/codeBuffer.hpp	Wed Sep 03 15:26:06 2014 +0400
+++ b/src/share/vm/asm/codeBuffer.hpp	Fri Aug 29 19:45:49 2014 -0400
@@ -27,6 +27,7 @@
 
 #include "code/oopRecorder.hpp"
 #include "code/relocInfo.hpp"
+#include "utilities/debug.hpp"
 
 class CodeStrings;
 class PhaseCFG;
@@ -245,15 +246,39 @@
 private:
 #ifndef PRODUCT
   CodeString* _strings;
+#ifdef ASSERT
+  // Becomes true after copy-out, forbids further use.
+  bool _defunct; // Zero bit pattern is "valid", see memset call in decode_env::decode_env
+#endif
 #endif
 
   CodeString* find(intptr_t offset) const;
   CodeString* find_last(intptr_t offset) const;
 
+  void set_null_and_invalidate() {
+#ifndef PRODUCT
+    _strings = NULL;
+#ifdef ASSERT
+    _defunct = true;
+#endif
+#endif
+  }
+
 public:
   CodeStrings() {
 #ifndef PRODUCT
     _strings = NULL;
+#ifdef ASSERT
+    _defunct = false;
+#endif
+#endif
+  }
+
+  bool is_null() {
+#ifdef ASSERT
+    return _strings == NULL;
+#else
+    return true;
 #endif
   }
 
@@ -261,8 +286,17 @@
 
   void add_comment(intptr_t offset, const char * comment) PRODUCT_RETURN;
   void print_block_comment(outputStream* stream, intptr_t offset) const PRODUCT_RETURN;
+  // MOVE strings from other to this; invalidate other.
   void assign(CodeStrings& other)  PRODUCT_RETURN;
+  // COPY strings from other to this; leave other valid.
+  void copy(CodeStrings& other)  PRODUCT_RETURN;
   void free() PRODUCT_RETURN;
+  // Guarantee that _strings are used at most once; assign invalidates a buffer.
+  inline void check_valid() const {
+#ifdef ASSERT
+    assert(!_defunct, "Use of invalid CodeStrings");
+#endif
+  }
 };
 
 // A CodeBuffer describes a memory space into which assembly
@@ -330,7 +364,7 @@
   csize_t      _total_size;     // size in bytes of combined memory buffer
 
   OopRecorder* _oop_recorder;
-  CodeStrings  _strings;
+  CodeStrings  _code_strings;
   OopRecorder  _default_oop_recorder;  // override with initialize_oop_recorder
   Arena*       _overflow_arena;
 
@@ -531,7 +565,13 @@
   void initialize_oop_recorder(OopRecorder* r);
 
   OopRecorder* oop_recorder() const   { return _oop_recorder; }
-  CodeStrings& strings()              { return _strings; }
+  CodeStrings& strings()              { return _code_strings; }
+
+  void free_strings() {
+    if (!_code_strings.is_null()) {
+      _code_strings.free(); // sets _strings Null as a side-effect.
+    }
+  }
 
   // Code generation
   void relocate(address at, RelocationHolder const& rspec, int format = 0) {
diff -r 4d8781a35525 -r 094cbdffa87d src/share/vm/code/codeBlob.cpp
--- a/src/share/vm/code/codeBlob.cpp	Wed Sep 03 15:26:06 2014 +0400
+++ b/src/share/vm/code/codeBlob.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -253,6 +253,7 @@
 
 void BufferBlob::free( BufferBlob *blob ) {
   ThreadInVMfromUnknown __tiv;  // get to VM state in case we block on CodeCache_lock
+  blob->flush();
   {
     MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
     CodeCache::free((CodeBlob*)blob);
diff -r 4d8781a35525 -r 094cbdffa87d src/share/vm/compiler/disassembler.cpp
--- a/src/share/vm/compiler/disassembler.cpp	Wed Sep 03 15:26:06 2014 +0400
+++ b/src/share/vm/compiler/disassembler.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -245,12 +245,12 @@
 };
 
 decode_env::decode_env(CodeBlob* code, outputStream* output, CodeStrings c) {
-  memset(this, 0, sizeof(*this));
+  memset(this, 0, sizeof(*this)); // Beware, this zeroes bits of fields.
   _output = output ? output : tty;
   _code = code;
   if (code != NULL && code->is_nmethod())
     _nm = (nmethod*) code;
-  _strings.assign(c);
+  _strings.copy(c);
 
   // by default, output pc but not bytes:
   _print_pc       = true;
diff -r 4d8781a35525 -r 094cbdffa87d src/share/vm/interpreter/interpreter.hpp
--- a/src/share/vm/interpreter/interpreter.hpp	Wed Sep 03 15:26:06 2014 +0400
+++ b/src/share/vm/interpreter/interpreter.hpp	Fri Aug 29 19:45:49 2014 -0400
@@ -53,7 +53,9 @@
  public:
   // Initialization/finalization
   void    initialize(int size,
-                     CodeStrings& strings)       { _size = size; DEBUG_ONLY(_strings.assign(strings);) }
+                     CodeStrings& strings)       { _size = size;
+                                                   DEBUG_ONLY(::new(&_strings) CodeStrings();)
+                                                   DEBUG_ONLY(_strings.assign(strings);) }
   void    finalize()                             { ShouldNotCallThis(); }
 
   // General info/converters