6599425: 4/3 OopMapCache::lookup() can cause later crash or assert() failure

Summary: Add should_not_be_cached() to markOop and methodOop and query that status inOopMapCache::lookup()
Reviewed-by: coleenp, sspitsyn, jmasa

src/share/vm/includeDB_core

@@ -3068,6 +3068,7 @@ oopMap.hpp                              vmreg.hpp
 
 oopMapCache.cpp                         allocation.inline.hpp
 oopMapCache.cpp                         handles.inline.hpp
+oopMapCache.cpp                         jvmtiRedefineClassesTrace.hpp
 oopMapCache.cpp                         oop.inline.hpp
 oopMapCache.cpp                         oopMapCache.hpp
 oopMapCache.cpp                         resourceArea.hpp

src/share/vm/interpreter/oopMapCache.cpp

@@ -532,6 +532,10 @@ void OopMapCache::flush_obsolete_entries() {
     if (!_array[i].is_empty() && _array[i].method()->is_old()) {
       // Cache entry is occupied by an old redefined method and we don't want
       // to pin it down so flush the entry.
+      RC_TRACE(0x08000000, ("flush: %s(%s): cached entry @%d",
+        _array[i].method()->name()->as_C_string(),
+        _array[i].method()->signature()->as_C_string(), i));
+
       _array[i].flush();
     }
 }
@@ -577,6 +581,15 @@ void OopMapCache::lookup(methodHandle method,
   // Entry is not in hashtable.
   // Compute entry and return it
 
+  if (method->should_not_be_cached()) {
+    // It is either not safe or not a good idea to cache this methodOop
+    // at this time. We give the caller of lookup() a copy of the
+    // interesting info via parameter entry_for, but we don't add it to
+    // the cache. See the gory details in methodOop.cpp.
+    compute_one_oop_map(method, bci, entry_for);
+    return;
+  }
+
   // First search for an empty slot
   for(i = 0; i < _probe_depth; i++) {
     entry  = entry_at(probe + i);
@@ -584,12 +597,6 @@ void OopMapCache::lookup(methodHandle method,
       entry->fill(method, bci);
       entry_for->resource_copy(entry);
       assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
-      if (method->is_old()) {
-        // The caller of lookup() will receive a copy of the interesting
-        // info via entry_for, but we don't keep an old redefined method in
-        // the cache to avoid pinning down the method.
-        entry->flush();
-      }
       return;
     }
   }
@@ -623,13 +630,6 @@ void OopMapCache::lookup(methodHandle method,
   }
   assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
 
-  if (method->is_old()) {
-    // The caller of lookup() will receive a copy of the interesting
-    // info via entry_for, but we don't keep an old redefined method in
-    // the cache to avoid pinning down the method.
-    entry->flush();
-  }
-
   return;
 }
 

src/share/vm/oops/markOop.cpp

@@ -37,3 +37,32 @@ void markOopDesc::print_on(outputStream* st) const {
     st->print("age %d)", age());
   }
 }
+
+
+// Give advice about whether the oop that contains this markOop
+// should be cached or not.
+bool markOopDesc::should_not_be_cached() const {
+  // the cast is because decode_pointer() isn't marked const
+  if (is_marked() && ((markOopDesc *)this)->decode_pointer() != NULL) {
+    // If the oop containing this markOop is being forwarded, then
+    // we are in the middle of GC and we do not want the containing
+    // oop to be added to a cache. We have no way of knowing whether
+    // the cache has already been visited by the current GC phase so
+    // we don't know whether the forwarded oop will be properly
+    // processed in this phase. If the forwarded oop is not properly
+    // processed, then we'll see strange crashes or asserts during
+    // the next GC run because the markOop will contain an unexpected
+    // value.
+    //
+    // This situation has been seen when we are GC'ing a methodOop
+    // because we use the methodOop while we're GC'ing it. Scary
+    // stuff. Some of the uses the methodOop cause the methodOop to
+    // be added to the OopMapCache in the instanceKlass as a side
+    // effect. This check lets the cache maintainer know when a
+    // cache addition would not be safe.
+    return true;
+  }
+
+  // caching the containing oop should be just fine
+  return false;
+}

src/share/vm/oops/markOop.hpp

@@ -357,4 +357,7 @@ class markOopDesc: public oopDesc {
 
   // Recover address of oop from encoded form used in mark
   inline void* decode_pointer() { if (UseBiasedLocking && has_bias_pattern()) return NULL; return clear_lock_bits(); }
+
+  // see the definition in markOop.cpp for the gory details
+  bool should_not_be_cached() const;
 };

src/share/vm/oops/methodOop.cpp

@@ -765,6 +765,28 @@ bool methodOopDesc::is_overridden_in(klassOop k) const {
 }
 
 
+// give advice about whether this methodOop should be cached or not
+bool methodOopDesc::should_not_be_cached() const {
+  if (is_old()) {
+    // This method has been redefined. It is either EMCP or obsolete
+    // and we don't want to cache it because that would pin the method
+    // down and prevent it from being collectible if and when it
+    // finishes executing.
+    return true;
+  }
+
+  if (mark()->should_not_be_cached()) {
+    // It is either not safe or not a good idea to cache this
+    // method at this time because of the state of the embedded
+    // markOop. See markOop.cpp for the gory details.
+    return true;
+  }
+
+  // caching this method should be just fine
+  return false;
+}
+
+
 methodHandle methodOopDesc:: clone_with_new_data(methodHandle m, u_char* new_code, int new_code_length,
                                                 u_char* new_compressed_linenumber_table, int new_compressed_linenumber_size, TRAPS) {
   // Code below does not work for native methods - they should never get rewritten anyway

src/share/vm/oops/methodOop.hpp

@@ -524,6 +524,8 @@ class methodOopDesc : public oopDesc {
   void set_is_old()                                 { _access_flags.set_is_old(); }
   bool is_obsolete() const                          { return access_flags().is_obsolete(); }
   void set_is_obsolete()                            { _access_flags.set_is_obsolete(); }
+  // see the definition in methodOop.cpp for the gory details
+  bool should_not_be_cached() const;
 
   // JVMTI Native method prefixing support:
   bool is_prefixed_native() const                   { return access_flags().is_prefixed_native(); }

src/share/vm/prims/jvmtiRedefineClassesTrace.hpp

@@ -64,7 +64,7 @@
 //    0x01000000 |   16777216 - impl details: nmethod evolution info
 //    0x02000000 |   33554432 - impl details: annotation updates
 //    0x04000000 |   67108864 - impl details: StackMapTable updates
-//    0x08000000 |  134217728 - unused
+//    0x08000000 |  134217728 - impl details: OopMapCache updates
 //    0x10000000 |  268435456 - unused
 //    0x20000000 |  536870912 - unused
 //    0x40000000 | 1073741824 - unused