8255208: CodeStrings passed to Disassembler::decode are ignored

Reviewed-by: kvn, iklam

Author: cl4es

src/hotspot/share/asm/codeBuffer.cpp

@@ -140,7 +140,7 @@ CodeBuffer::~CodeBuffer() {
 
   // 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();
+  NOT_PRODUCT(free_strings();)
 
 #ifdef ASSERT
   // Save allocation type to execute assert in ~ResourceObj()
@@ -267,13 +267,14 @@ bool CodeBuffer::is_backward_branch(Label& L) {
   return L.is_bound() && insts_end() <= locator_address(L.loc());
 }
 
+#ifndef PRODUCT
 address CodeBuffer::decode_begin() {
   address begin = _insts.start();
   if (_decode_begin != NULL && _decode_begin > begin)
     begin = _decode_begin;
   return begin;
 }
-
+#endif // !PRODUCT
 
 GrowableArray<int>* CodeBuffer::create_patch_overflow() {
   if (_overflow_arena == NULL) {
@@ -752,7 +753,7 @@ void CodeBuffer::copy_code_to(CodeBlob* dest_blob) {
   relocate_code_to(&dest);
 
   // transfer strings and comments from buffer to blob
-  dest_blob->set_strings(_code_strings);
+  NOT_PRODUCT(dest_blob->set_strings(_code_strings);)
 
   // Done moving code bytes; were they the right size?
   assert((int)align_up(dest.total_content_size(), oopSize) == dest_blob->content_size(), "sanity");
@@ -957,12 +958,11 @@ void CodeBuffer::expand(CodeSection* which_cs, csize_t amount) {
   debug_only(Copy::fill_to_bytes(bxp->_total_start, bxp->_total_size,
                                  badCodeHeapFreeVal));
 
-  _decode_begin = NULL;  // sanity
-
   // Make certain that the new sections are all snugly inside the new blob.
   verify_section_allocation();
 
 #ifndef PRODUCT
+  _decode_begin = NULL;  // sanity
   if (PrintNMethods && (WizardMode || Verbose)) {
     tty->print("expanded CodeBuffer:");
     this->print();
@@ -1032,10 +1032,6 @@ void CodeBuffer::log_section_sizes(const char* name) {
 
 #ifndef PRODUCT
 
-void CodeSection::decode() {
-  Disassembler::decode(start(), end());
-}
-
 void CodeBuffer::block_comment(intptr_t offset, const char * comment) {
   if (_collect_comments) {
     _code_strings.add_comment(offset, comment);
@@ -1054,8 +1050,12 @@ class CodeString: public CHeapObj<mtCode> {
   CodeString*  _prev;
   intptr_t     _offset;
 
+  static long allocated_code_strings;
+
   ~CodeString() {
     assert(_next == NULL && _prev == NULL, "wrong interface for freeing list");
+    allocated_code_strings--;
+    log_trace(codestrings)("Freeing CodeString [%s] (%p)", _string, (void*)_string);
     os::free((void*)_string);
   }
 
@@ -1064,12 +1064,14 @@ class CodeString: public CHeapObj<mtCode> {
  public:
   CodeString(const char * string, intptr_t offset = -1)
     : _next(NULL), _prev(NULL), _offset(offset) {
+    allocated_code_strings++;
     _string = os::strdup(string, mtCode);
+    log_trace(codestrings)("Created CodeString [%s] (%p)", _string, (void*)_string);
   }
 
   const char * string() const { return _string; }
   intptr_t     offset() const { assert(_offset >= 0, "offset for non comment?"); return _offset;  }
-  CodeString* next()    const { return _next; }
+  CodeString*  next()   const { return _next; }
 
   void set_next(CodeString* next) {
     _next = next;
@@ -1094,6 +1096,10 @@ class CodeString: public CHeapObj<mtCode> {
   }
 };
 
+// For tracing statistics. Will use raw increment/decrement, so it might not be
+// exact
+long CodeString::allocated_code_strings = 0;
+
 CodeString* CodeStrings::find(intptr_t offset) const {
   CodeString* a = _strings->first_comment();
   while (a != NULL && a->offset() != offset) {
@@ -1116,7 +1122,7 @@ void CodeStrings::add_comment(intptr_t offset, const char * comment) {
   CodeString* c      = new CodeString(comment, offset);
   CodeString* inspos = (_strings == NULL) ? NULL : find_last(offset);
 
-  if (inspos) {
+  if (inspos != NULL) {
     // insert after already existing comments with same offset
     c->set_next(inspos->next());
     inspos->set_next(c);
@@ -1130,29 +1136,22 @@ void CodeStrings::add_comment(intptr_t offset, const char * comment) {
   }
 }
 
-void CodeStrings::assign(CodeStrings& other) {
-  other.check_valid();
-  assert(is_null(), "Cannot assign onto non-empty CodeStrings");
-  _strings = other._strings;
-  _strings_last = other._strings_last;
-#ifdef ASSERT
-  _defunct = false;
-#endif
-  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) {
+  log_debug(codestrings)("Copying %d Codestring(s)", other.count());
+
   other.check_valid();
   check_valid();
   assert(is_null(), "Cannot copy onto non-empty CodeStrings");
   CodeString* n = other._strings;
   CodeString** ps = &_strings;
   CodeString* prev = NULL;
   while (n != NULL) {
-    *ps = new CodeString(n->string(),n->offset());
+    if (n->is_comment()) {
+      *ps = new CodeString(n->string(), n->offset());
+    } else {
+      *ps = new CodeString(n->string());
+    }
     (*ps)->_prev = prev;
     prev = *ps;
     ps = &((*ps)->_next);
@@ -1162,13 +1161,6 @@ void CodeStrings::copy(CodeStrings& other) {
 
 const char* CodeStrings::_prefix = " ;; ";  // default: can be changed via set_prefix
 
-// Check if any block comments are pending for the given offset.
-bool CodeStrings::has_block_comment(intptr_t offset) const {
-  if (_strings == NULL) return false;
-  CodeString* c = find(offset);
-  return c != NULL;
-}
-
 void CodeStrings::print_block_comment(outputStream* stream, intptr_t offset) const {
   check_valid();
   if (_strings != NULL) {
@@ -1184,8 +1176,19 @@ void CodeStrings::print_block_comment(outputStream* stream, intptr_t offset) con
   }
 }
 
-// Also sets isNull()
+int CodeStrings::count() const {
+  int i = 0;
+  CodeString* s = _strings;
+  while (s != NULL) {
+    i++;
+    s = s->_next;
+  }
+  return i;
+}
+
+// Also sets is_null()
 void CodeStrings::free() {
+  log_debug(codestrings)("Freeing %d out of approx. %ld CodeString(s), ", count(), CodeString::allocated_code_strings);
   CodeString* n = _strings;
   while (n) {
     // unlink the node from the list saving a pointer to the next
@@ -1215,7 +1218,7 @@ const char* CodeStrings::add_string(const char * string) {
 
 void CodeBuffer::decode() {
   ttyLocker ttyl;
-  Disassembler::decode(decode_begin(), insts_end(), tty);
+  Disassembler::decode(decode_begin(), insts_end(), tty NOT_PRODUCT(COMMA &strings()));
   _decode_begin = insts_end();
 }
 
@@ -1246,10 +1249,4 @@ void CodeBuffer::print() {
   }
 }
 
-// Directly disassemble code buffer.
-void CodeBuffer::decode(address start, address end) {
-  ttyLocker ttyl;
-  Disassembler::decode(this, start, end, tty);
-}
-
 #endif // PRODUCT

src/hotspot/share/asm/codeBuffer.hpp

@@ -284,20 +284,18 @@ class CodeStrings {
   bool _defunct; // Zero bit pattern is "valid", see memset call in decode_env::decode_env
 #endif
   static const char* _prefix; // defaults to " ;; "
-#endif
 
   CodeString* find(intptr_t offset) const;
   CodeString* find_last(intptr_t offset) const;
 
   void set_null_and_invalidate() {
-#ifndef PRODUCT
     _strings = NULL;
     _strings_last = NULL;
 #ifdef ASSERT
     _defunct = true;
-#endif
 #endif
   }
+#endif
 
 public:
   CodeStrings() {
@@ -310,6 +308,7 @@ class CodeStrings {
 #endif
   }
 
+#ifndef PRODUCT
   bool is_null() {
 #ifdef ASSERT
     return _strings == NULL;
@@ -318,30 +317,25 @@ class CodeStrings {
 #endif
   }
 
-  const char* add_string(const char * string) PRODUCT_RETURN_(return NULL;);
+  const char* add_string(const char * string);
 
-  void add_comment(intptr_t offset, const char * comment) PRODUCT_RETURN;
-  bool has_block_comment(intptr_t offset) const;
-  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;
+  void add_comment(intptr_t offset, const char * comment);
+  void print_block_comment(outputStream* stream, intptr_t offset) const;
+  int  count() const;
   // COPY strings from other to this; leave other valid.
-  void copy(CodeStrings& other)  PRODUCT_RETURN;
+  void copy(CodeStrings& other);
   // FREE strings; invalidate this.
-  void free() PRODUCT_RETURN;
+  void free();
 
   // Guarantee that _strings are used at most once; assign and free invalidate a buffer.
   inline void check_valid() const {
-#ifdef ASSERT
     assert(!_defunct, "Use of invalid CodeStrings");
-#endif
   }
 
   static void set_prefix(const char *prefix) {
-#ifndef PRODUCT
     _prefix = prefix;
-#endif
   }
+#endif // !PRODUCT
 };
 
 // A CodeBuffer describes a memory space into which assembly
@@ -410,8 +404,7 @@ class CodeBuffer: public StackObj {
   csize_t      _total_size;     // size in bytes of combined memory buffer
 
   OopRecorder* _oop_recorder;
-  CodeStrings  _code_strings;
-  bool         _collect_comments;      // Indicate if we need to collect block comments at all.
+
   OopRecorder  _default_oop_recorder;  // override with initialize_oop_recorder
   Arena*       _overflow_arena;
 
@@ -421,8 +414,12 @@ class CodeBuffer: public StackObj {
   bool         _immutable_PIC;
 #endif
 
-  address      _decode_begin;   // start address for decode
+#ifndef PRODUCT
+  CodeStrings  _code_strings;
+  bool         _collect_comments; // Indicate if we need to collect block comments at all.
+  address      _decode_begin;     // start address for decode
   address      decode_begin();
+#endif
 
   void initialize_misc(const char * name) {
     // all pointers other than code_start/end and those inside the sections
@@ -431,14 +428,15 @@ class CodeBuffer: public StackObj {
     _before_expand   = NULL;
     _blob            = NULL;
     _oop_recorder    = NULL;
-    _decode_begin    = NULL;
     _overflow_arena  = NULL;
-    _code_strings    = CodeStrings();
     _last_insn       = NULL;
 #if INCLUDE_AOT
     _immutable_PIC   = false;
 #endif
 
+#ifndef PRODUCT
+    _decode_begin    = NULL;
+    _code_strings    = CodeStrings();
     // Collect block comments, but restrict collection to cases where a disassembly is output.
     _collect_comments = ( PrintAssembly
                        || PrintStubCode
@@ -447,6 +445,7 @@ class CodeBuffer: public StackObj {
                        || PrintSignatureHandlers
                        || UnlockDiagnosticVMOptions
                         );
+#endif
   }
 
   void initialize(address code_start, csize_t code_size) {
@@ -635,35 +634,27 @@ class CodeBuffer: public StackObj {
   // Override default oop recorder.
   void initialize_oop_recorder(OopRecorder* r);
 
-  OopRecorder* oop_recorder() const   { return _oop_recorder; }
-  CodeStrings& strings()              { return _code_strings; }
+  OopRecorder* oop_recorder() const { return _oop_recorder; }
 
   address last_insn() const { return _last_insn; }
   void set_last_insn(address a) { _last_insn = a; }
   void clear_last_insn() { set_last_insn(NULL); }
 
+#ifndef PRODUCT
+  CodeStrings& strings() { return _code_strings; }
+
   void free_strings() {
     if (!_code_strings.is_null()) {
       _code_strings.free(); // sets _strings Null as a side-effect.
     }
   }
 
-  // Directly disassemble code buffer.
   // Print the comment associated with offset on stream, if there is one.
   virtual void print_block_comment(outputStream* stream, address block_begin) {
-#ifndef PRODUCT
     intptr_t offset = (intptr_t)(block_begin - _total_start);  // I assume total_start is not correct for all code sections.
     _code_strings.print_block_comment(stream, offset);
-#endif
   }
-  bool has_block_comment(address block_begin) {
-#ifndef PRODUCT
-    intptr_t offset = (intptr_t)(block_begin - _total_start);  // I assume total_start is not correct for all code sections.
-    return _code_strings.has_block_comment(offset);
-#else
-    return false;
 #endif
-  }
 
   // Code generation
   void relocate(address at, RelocationHolder const& rspec, int format = 0) {

src/hotspot/share/code/codeBlob.cpp

@@ -87,8 +87,8 @@ CodeBlob::CodeBlob(const char* name, CompilerType type, const CodeBlobLayout& la
   _relocation_end(layout.relocation_end()),
   _oop_maps(oop_maps),
   _caller_must_gc_arguments(caller_must_gc_arguments),
-  _strings(CodeStrings()),
   _name(name)
+  NOT_PRODUCT(COMMA _strings(CodeStrings()))
 {
   assert(is_aligned(layout.size(),            oopSize), "unaligned size");
   assert(is_aligned(layout.header_size(),     oopSize), "unaligned size");
@@ -115,8 +115,8 @@ CodeBlob::CodeBlob(const char* name, CompilerType type, const CodeBlobLayout& la
   _relocation_begin(layout.relocation_begin()),
   _relocation_end(layout.relocation_end()),
   _caller_must_gc_arguments(caller_must_gc_arguments),
-  _strings(CodeStrings()),
   _name(name)
+  NOT_PRODUCT(COMMA _strings(CodeStrings()))
 {
   assert(is_aligned(_size,        oopSize), "unaligned size");
   assert(is_aligned(_header_size, oopSize), "unaligned size");
@@ -158,7 +158,7 @@ RuntimeBlob::RuntimeBlob(
 void CodeBlob::flush() {
   FREE_C_HEAP_ARRAY(unsigned char, _oop_maps);
   _oop_maps = NULL;
-  _strings.free();
+  NOT_PRODUCT(_strings.free();)
 }
 
 void CodeBlob::set_oop_maps(OopMapSet* p) {