changeset 26432:9b974e2eae27

8054292: code comments leak in fastdebug builds Summary: Added deallocation to destructor; hardened interface against misuse Reviewed-by: kvn
author drchase
date Fri, 29 Aug 2014 19:45:49 -0400
parents 83ba55fba738
children 27020fde2dbf
files hotspot/src/share/vm/asm/codeBuffer.cpp hotspot/src/share/vm/asm/codeBuffer.hpp hotspot/src/share/vm/code/codeBlob.cpp hotspot/src/share/vm/compiler/disassembler.cpp hotspot/src/share/vm/interpreter/interpreter.hpp
diffstat 5 files changed, 81 insertions(+), 11 deletions(-) [+]
line wrap: on
line diff
--- a/hotspot/src/share/vm/asm/codeBuffer.cpp	Fri Aug 29 13:46:50 2014 +0000
+++ b/hotspot/src/share/vm/asm/codeBuffer.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -134,6 +134,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.
@@ -705,7 +709,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");
@@ -1005,11 +1009,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> {
@@ -1075,6 +1079,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);
 
@@ -1090,11 +1095,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();
@@ -1105,7 +1131,7 @@
   }
 }
 
-
+// Also sets isNull()
 void CodeStrings::free() {
   CodeString* n = _strings;
   while (n) {
@@ -1115,10 +1141,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;
--- a/hotspot/src/share/vm/asm/codeBuffer.hpp	Fri Aug 29 13:46:50 2014 +0000
+++ b/hotspot/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) {
--- a/hotspot/src/share/vm/code/codeBlob.cpp	Fri Aug 29 13:46:50 2014 +0000
+++ b/hotspot/src/share/vm/code/codeBlob.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -238,6 +238,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);
--- a/hotspot/src/share/vm/compiler/disassembler.cpp	Fri Aug 29 13:46:50 2014 +0000
+++ b/hotspot/src/share/vm/compiler/disassembler.cpp	Fri Aug 29 19:45:49 2014 -0400
@@ -246,12 +246,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;
--- a/hotspot/src/share/vm/interpreter/interpreter.hpp	Fri Aug 29 13:46:50 2014 +0000
+++ b/hotspot/src/share/vm/interpreter/interpreter.hpp	Fri Aug 29 19:45:49 2014 -0400
@@ -55,7 +55,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