8140685: Fix backtrace building to not rely on constant pool merging
authorcoleenp
Wed, 01 Feb 2017 17:56:22 -0500
changeset 46257 3e95288ce4ca
parent 43605 07baf498d588
child 46258 d26ebd7e2f10
8140685: Fix backtrace building to not rely on constant pool merging Summary: Store Symbol* for the name in the backtrace Reviewed-by: gtriantafill, dholmes, kbarrett, lfoltan
hotspot/src/share/vm/classfile/javaClasses.cpp
hotspot/src/share/vm/classfile/javaClasses.hpp
hotspot/src/share/vm/memory/oopFactory.cpp
hotspot/src/share/vm/memory/oopFactory.hpp
hotspot/src/share/vm/oops/typeArrayOop.hpp
--- a/hotspot/src/share/vm/classfile/javaClasses.cpp	Tue Jan 31 19:26:50 2017 -0500
+++ b/hotspot/src/share/vm/classfile/javaClasses.cpp	Wed Feb 01 17:56:22 2017 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -1578,7 +1578,7 @@
   typeArrayOop    _methods;
   typeArrayOop    _bcis;
   objArrayOop     _mirrors;
-  typeArrayOop    _cprefs; // needed to insulate method name against redefinition
+  typeArrayOop    _names; // needed to insulate method name against redefinition
   int             _index;
   NoSafepointVerifier _nsv;
 
@@ -1586,7 +1586,7 @@
     trace_methods_offset = java_lang_Throwable::trace_methods_offset,
     trace_bcis_offset    = java_lang_Throwable::trace_bcis_offset,
     trace_mirrors_offset = java_lang_Throwable::trace_mirrors_offset,
-    trace_cprefs_offset  = java_lang_Throwable::trace_cprefs_offset,
+    trace_names_offset   = java_lang_Throwable::trace_names_offset,
     trace_next_offset    = java_lang_Throwable::trace_next_offset,
     trace_size           = java_lang_Throwable::trace_size,
     trace_chunk_size     = java_lang_Throwable::trace_chunk_size
@@ -1608,16 +1608,16 @@
     assert(mirrors != NULL, "mirror array should be initialized in backtrace");
     return mirrors;
   }
-  static typeArrayOop get_cprefs(objArrayHandle chunk) {
-    typeArrayOop cprefs = typeArrayOop(chunk->obj_at(trace_cprefs_offset));
-    assert(cprefs != NULL, "cprefs array should be initialized in backtrace");
-    return cprefs;
+  static typeArrayOop get_names(objArrayHandle chunk) {
+    typeArrayOop names = typeArrayOop(chunk->obj_at(trace_names_offset));
+    assert(names != NULL, "names array should be initialized in backtrace");
+    return names;
   }
 
  public:
 
   // constructor for new backtrace
-  BacktraceBuilder(TRAPS): _methods(NULL), _bcis(NULL), _head(NULL), _mirrors(NULL), _cprefs(NULL) {
+  BacktraceBuilder(TRAPS): _methods(NULL), _bcis(NULL), _head(NULL), _mirrors(NULL), _names(NULL) {
     expand(CHECK);
     _backtrace = _head;
     _index = 0;
@@ -1627,9 +1627,10 @@
     _methods = get_methods(backtrace);
     _bcis = get_bcis(backtrace);
     _mirrors = get_mirrors(backtrace);
-    _cprefs = get_cprefs(backtrace);
+    _names = get_names(backtrace);
     assert(_methods->length() == _bcis->length() &&
-           _methods->length() == _mirrors->length(),
+           _methods->length() == _mirrors->length() &&
+           _mirrors->length() == _names->length(),
            "method and source information arrays should match");
 
     // head is the preallocated backtrace
@@ -1653,8 +1654,8 @@
     objArrayOop mirrors = oopFactory::new_objectArray(trace_chunk_size, CHECK);
     objArrayHandle new_mirrors(THREAD, mirrors);
 
-    typeArrayOop cprefs = oopFactory::new_shortArray(trace_chunk_size, CHECK);
-    typeArrayHandle new_cprefs(THREAD, cprefs);
+    typeArrayOop names = oopFactory::new_symbolArray(trace_chunk_size, CHECK);
+    typeArrayHandle new_names(THREAD, names);
 
     if (!old_head.is_null()) {
       old_head->obj_at_put(trace_next_offset, new_head());
@@ -1662,13 +1663,13 @@
     new_head->obj_at_put(trace_methods_offset, new_methods());
     new_head->obj_at_put(trace_bcis_offset, new_bcis());
     new_head->obj_at_put(trace_mirrors_offset, new_mirrors());
-    new_head->obj_at_put(trace_cprefs_offset, new_cprefs());
+    new_head->obj_at_put(trace_names_offset, new_names());
 
     _head    = new_head();
     _methods = new_methods();
     _bcis = new_bcis();
     _mirrors = new_mirrors();
-    _cprefs  = new_cprefs();
+    _names  = new_names();
     _index = 0;
   }
 
@@ -1690,7 +1691,11 @@
 
     _methods->short_at_put(_index, method->orig_method_idnum());
     _bcis->int_at_put(_index, Backtrace::merge_bci_and_version(bci, method->constants()->version()));
-    _cprefs->short_at_put(_index, method->name_index());
+
+    // Note:this doesn't leak symbols because the mirror in the backtrace keeps the
+    // klass owning the symbols alive so their refcounts aren't decremented.
+    Symbol* name = method->name();
+    _names->symbol_at_put(_index, name);
 
     // We need to save the mirrors in the backtrace to keep the class
     // from being unloaded while we still have this stack trace.
@@ -1705,10 +1710,10 @@
   int _method_id;
   int _bci;
   int _version;
-  int _cpref;
+  Symbol* _name;
   Handle _mirror;
-  BacktraceElement(Handle mirror, int mid, int version, int bci, int cpref) :
-                   _mirror(mirror), _method_id(mid), _version(version), _bci(bci), _cpref(cpref) {}
+  BacktraceElement(Handle mirror, int mid, int version, int bci, Symbol* name) :
+                   _mirror(mirror), _method_id(mid), _version(version), _bci(bci), _name(name) {}
 };
 
 class BacktraceIterator : public StackObj {
@@ -1717,7 +1722,7 @@
   objArrayHandle  _mirrors;
   typeArrayHandle _methods;
   typeArrayHandle _bcis;
-  typeArrayHandle _cprefs;
+  typeArrayHandle _names;
 
   void init(objArrayHandle result, Thread* thread) {
     // Get method id, bci, version and mirror from chunk
@@ -1726,7 +1731,7 @@
       _methods = typeArrayHandle(thread, BacktraceBuilder::get_methods(_result));
       _bcis = typeArrayHandle(thread, BacktraceBuilder::get_bcis(_result));
       _mirrors = objArrayHandle(thread, BacktraceBuilder::get_mirrors(_result));
-      _cprefs = typeArrayHandle(thread, BacktraceBuilder::get_cprefs(_result));
+      _names = typeArrayHandle(thread, BacktraceBuilder::get_names(_result));
       _index = 0;
     }
   }
@@ -1741,7 +1746,7 @@
                         _methods->short_at(_index),
                         Backtrace::version_at(_bcis->int_at(_index)),
                         Backtrace::bci_at(_bcis->int_at(_index)),
-                        _cprefs->short_at(_index));
+                        _names->symbol_at(_index));
     _index++;
 
     if (_index >= java_lang_Throwable::trace_chunk_size) {
@@ -1761,7 +1766,7 @@
 
 // Print stack trace element to resource allocated buffer
 static void print_stack_element_to_stream(outputStream* st, Handle mirror, int method_id,
-                                          int version, int bci, int cpref) {
+                                          int version, int bci, Symbol* name) {
   ResourceMark rm;
 
   // Get strings and string lengths
@@ -1769,11 +1774,7 @@
   const char* klass_name  = holder->external_name();
   int buf_len = (int)strlen(klass_name);
 
-  Method* method = holder->method_with_orig_idnum(method_id, version);
-
-  // The method can be NULL if the requested class version is gone
-  Symbol* sym = (method != NULL) ? method->name() : holder->constants()->symbol_at(cpref);
-  char* method_name = sym->as_C_string();
+  char* method_name = name->as_C_string();
   buf_len += (int)strlen(method_name);
 
   char* source_file_name = NULL;
@@ -1809,6 +1810,8 @@
     }
   }
 
+  // The method can be NULL if the requested class version is gone
+  Method* method = holder->method_with_orig_idnum(method_id, version);
   if (!version_matches(method, version)) {
     strcat(buf, "Redefined)");
   } else {
@@ -1840,8 +1843,7 @@
   Handle mirror = method->method_holder()->java_mirror();
   int method_id = method->orig_method_idnum();
   int version = method->constants()->version();
-  int cpref = method->name_index();
-  print_stack_element_to_stream(st, mirror, method_id, version, bci, cpref);
+  print_stack_element_to_stream(st, mirror, method_id, version, bci, method->name());
 }
 
 /**
@@ -1865,7 +1867,7 @@
 
     while (iter.repeat()) {
       BacktraceElement bte = iter.next(THREAD);
-      print_stack_element_to_stream(st, bte._mirror, bte._method_id, bte._version, bte._bci, bte._cpref);
+      print_stack_element_to_stream(st, bte._mirror, bte._method_id, bte._version, bte._bci, bte._name);
     }
     {
       // Call getCause() which doesn't necessarily return the _cause field.
@@ -2144,7 +2146,7 @@
                                          method,
                                          bte._version,
                                          bte._bci,
-                                         bte._cpref, CHECK);
+                                         bte._name, CHECK);
   }
 }
 
@@ -2159,15 +2161,14 @@
 
   Handle element = ik->allocate_instance_handle(CHECK_0);
 
-  int cpref = method->name_index();
   int version = method->constants()->version();
-  fill_in(element, method->method_holder(), method, version, bci, cpref, CHECK_0);
+  fill_in(element, method->method_holder(), method, version, bci, method->name(), CHECK_0);
   return element();
 }
 
 void java_lang_StackTraceElement::fill_in(Handle element,
                                           InstanceKlass* holder, const methodHandle& method,
-                                          int version, int bci, int cpref, TRAPS) {
+                                          int version, int bci, Symbol* name, TRAPS) {
   assert(element->is_a(SystemDictionary::StackTraceElement_klass()), "sanity check");
 
   // Fill in class name
@@ -2184,11 +2185,8 @@
       java_lang_StackTraceElement::set_classLoaderName(element(), loader_name);
   }
 
-  // The method can be NULL if the requested class version is gone
-  Symbol* sym = !method.is_null() ? method->name() : holder->constants()->symbol_at(cpref);
-
   // Fill in method name
-  oop methodname = StringTable::intern(sym, CHECK);
+  oop methodname = StringTable::intern(name, CHECK);
   java_lang_StackTraceElement::set_methodName(element(), methodname);
 
   // Fill in module name and version
@@ -2205,7 +2203,7 @@
     java_lang_StackTraceElement::set_moduleVersion(element(), module_version);
   }
 
-  if (!version_matches(method(), version)) {
+  if (method() == NULL || !version_matches(method(), version)) {
     // The method was redefined, accurate line number information isn't available
     java_lang_StackTraceElement::set_fileName(element(), NULL);
     java_lang_StackTraceElement::set_lineNumber(element(), -1);
@@ -2252,8 +2250,8 @@
 
   short version = stackFrame->short_field(_version_offset);
   short bci = stackFrame->short_field(_bci_offset);
-  int cpref = method->name_index();
-  java_lang_StackTraceElement::fill_in(stack_trace_element, holder, method, version, bci, cpref, CHECK);
+  Symbol* name = method->name();
+  java_lang_StackTraceElement::fill_in(stack_trace_element, holder, method, version, bci, name, CHECK);
 }
 
 void java_lang_StackFrameInfo::compute_offsets() {
--- a/hotspot/src/share/vm/classfile/javaClasses.hpp	Tue Jan 31 19:26:50 2017 -0500
+++ b/hotspot/src/share/vm/classfile/javaClasses.hpp	Wed Feb 01 17:56:22 2017 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -466,7 +466,7 @@
     trace_methods_offset = 0,
     trace_bcis_offset    = 1,
     trace_mirrors_offset = 2,
-    trace_cprefs_offset  = 3,
+    trace_names_offset   = 3,
     trace_next_offset    = 4,
     trace_size           = 5,
     trace_chunk_size     = 32
@@ -1331,7 +1331,7 @@
   static oop create(const methodHandle& method, int bci, TRAPS);
 
   static void fill_in(Handle element, InstanceKlass* holder, const methodHandle& method,
-                      int version, int bci, int cpref, TRAPS);
+                      int version, int bci, Symbol* name, TRAPS);
 
   // Debugging
   friend class JavaClasses;
--- a/hotspot/src/share/vm/memory/oopFactory.cpp	Tue Jan 31 19:26:50 2017 -0500
+++ b/hotspot/src/share/vm/memory/oopFactory.cpp	Wed Feb 01 17:56:22 2017 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -57,17 +57,15 @@
   return result;
 }
 
-// Create a Java array that points to metadata.
-// As far as Java code is concerned, a metaData array is either an array of
-// int or long depending on pointer size.  Only a few things use this, like
-// stack trace elements in Throwable.  They cast Method* into this type.
-// Note:can't point to symbols because there's no way to unreference count
-// them when this object goes away.
-typeArrayOop oopFactory::new_metaDataArray(int length, TRAPS) {
+// Create a Java array that points to Symbol.
+// As far as Java code is concerned, a Symbol array is either an array of
+// int or long depending on pointer size.  Only stack trace elements in Throwable use
+// this.  They cast Symbol* into this type.
+typeArrayOop oopFactory::new_symbolArray(int length, TRAPS) {
   BasicType type = LP64_ONLY(T_LONG) NOT_LP64(T_INT);
   Klass* type_asKlassOop = Universe::typeArrayKlassObj(type);
   TypeArrayKlass* type_asArrayKlass = TypeArrayKlass::cast(type_asKlassOop);
-  typeArrayOop result = type_asArrayKlass->allocate_common(length, true, THREAD);
+  typeArrayOop result = type_asArrayKlass->allocate(length, THREAD);
   return result;
 }
 
--- a/hotspot/src/share/vm/memory/oopFactory.hpp	Tue Jan 31 19:26:50 2017 -0500
+++ b/hotspot/src/share/vm/memory/oopFactory.hpp	Wed Feb 01 17:56:22 2017 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -62,7 +62,7 @@
 
   static typeArrayOop    new_typeArray(BasicType type, int length, TRAPS);
   static typeArrayOop    new_typeArray_nozero(BasicType type, int length, TRAPS);
-  static typeArrayOop    new_metaDataArray(int length, TRAPS);
+  static typeArrayOop    new_symbolArray(int length, TRAPS);
 
   // Regular object arrays
   static objArrayOop     new_objArray(Klass* klass, int length, TRAPS);
--- a/hotspot/src/share/vm/oops/typeArrayOop.hpp	Tue Jan 31 19:26:50 2017 -0500
+++ b/hotspot/src/share/vm/oops/typeArrayOop.hpp	Wed Feb 01 17:56:22 2017 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, 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
@@ -122,19 +122,19 @@
   jbyte byte_at_acquire(int which) const              { return OrderAccess::load_acquire(byte_at_addr(which)); }
   void release_byte_at_put(int which, jbyte contents) { OrderAccess::release_store(byte_at_addr(which), contents); }
 
-  // Java thinks metadata arrays are just arrays of either long or int, since
+  // Java thinks Symbol arrays are just arrays of either long or int, since
   // there doesn't seem to be T_ADDRESS, so this is a bit of unfortunate
   // casting
 #ifdef _LP64
-  Metadata* metadata_at(int which) const {
-    return (Metadata*)*long_at_addr(which); }
-  void metadata_at_put(int which, Metadata* contents) {
+  Symbol* symbol_at(int which) const {
+    return (Symbol*)*long_at_addr(which); }
+  void symbol_at_put(int which, Symbol* contents) {
     *long_at_addr(which) = (jlong)contents;
   }
 #else
-  Metadata* metadata_at(int which) const {
-    return (Metadata*)*int_at_addr(which); }
-  void metadata_at_put(int which, Metadata* contents) {
+  Symbol* symbol_at(int which) const {
+    return (Symbol*)*int_at_addr(which); }
+  void symbol_at_put(int which, Symbol* contents) {
     *int_at_addr(which) = (int)contents;
   }
 #endif // _LP64