Wed, 28 Nov 2012 17:50:21 -0500
8003635: NPG: AsynchGetCallTrace broken by Method* virtual call
Summary: Make metaspace::contains be lock free and used to see if something is in metaspace, also compare Method* with vtbl pointer.
Reviewed-by: dholmes, sspitsyn, dcubed, jmasa
1.1 --- a/src/cpu/sparc/vm/frame_sparc.cpp Wed Nov 28 08:43:26 2012 -0800 1.2 +++ b/src/cpu/sparc/vm/frame_sparc.cpp Wed Nov 28 17:50:21 2012 -0500 1.3 @@ -648,7 +648,7 @@ 1.4 Method* m = *interpreter_frame_method_addr(); 1.5 1.6 // validate the method we'd find in this potential sender 1.7 - if (!Universe::heap()->is_valid_method(m)) return false; 1.8 + if (!m->is_valid_method()) return false; 1.9 1.10 // stack frames shouldn't be much larger than max_stack elements 1.11
2.1 --- a/src/cpu/x86/vm/frame_x86.cpp Wed Nov 28 08:43:26 2012 -0800 2.2 +++ b/src/cpu/x86/vm/frame_x86.cpp Wed Nov 28 17:50:21 2012 -0500 2.3 @@ -534,7 +534,7 @@ 2.4 Method* m = *interpreter_frame_method_addr(); 2.5 2.6 // validate the method we'd find in this potential sender 2.7 - if (!Universe::heap()->is_valid_method(m)) return false; 2.8 + if (!m->is_valid_method()) return false; 2.9 2.10 // stack frames shouldn't be much larger than max_stack elements 2.11
3.1 --- a/src/share/vm/gc_interface/collectedHeap.hpp Wed Nov 28 08:43:26 2012 -0800 3.2 +++ b/src/share/vm/gc_interface/collectedHeap.hpp Wed Nov 28 17:50:21 2012 -0500 3.3 @@ -289,11 +289,6 @@ 3.4 // (A scavenge is a GC which is not a full GC.) 3.5 virtual bool is_scavengable(const void *p) = 0; 3.6 3.7 - // Returns "TRUE" if "p" is a method oop in the 3.8 - // current heap, with high probability. This predicate 3.9 - // is not stable, in general. 3.10 - bool is_valid_method(Method* p) const; 3.11 - 3.12 void set_gc_cause(GCCause::Cause v) { 3.13 if (UsePerfData) { 3.14 _gc_lastcause = _gc_cause;
4.1 --- a/src/share/vm/gc_interface/collectedHeap.inline.hpp Wed Nov 28 08:43:26 2012 -0800 4.2 +++ b/src/share/vm/gc_interface/collectedHeap.inline.hpp Wed Nov 28 17:50:21 2012 -0500 4.3 @@ -242,36 +242,6 @@ 4.4 return (oop)obj; 4.5 } 4.6 4.7 -// Returns "TRUE" if "p" is a method oop in the 4.8 -// current heap with high probability. NOTE: The main 4.9 -// current consumers of this interface are Forte:: 4.10 -// and ThreadProfiler::. In these cases, the 4.11 -// interpreter frame from which "p" came, may be 4.12 -// under construction when sampled asynchronously, so 4.13 -// the clients want to check that it represents a 4.14 -// valid method before using it. Nonetheless since 4.15 -// the clients do not typically lock out GC, the 4.16 -// predicate is_valid_method() is not stable, so 4.17 -// it is possible that by the time "p" is used, it 4.18 -// is no longer valid. 4.19 -inline bool CollectedHeap::is_valid_method(Method* p) const { 4.20 - return 4.21 - p != NULL && 4.22 - 4.23 - // Check whether "method" is metadata 4.24 - p->is_metadata() && 4.25 - 4.26 - // See if GC is active; however, there is still an 4.27 - // apparently unavoidable window after this call 4.28 - // and before the client of this interface uses "p". 4.29 - // If the client chooses not to lock out GC, then 4.30 - // it's a risk the client must accept. 4.31 - !is_gc_active() && 4.32 - 4.33 - // Check that p is a Method*. 4.34 - p->is_method(); 4.35 -} 4.36 - 4.37 inline void CollectedHeap::oop_iterate_no_header(OopClosure* cl) { 4.38 NoHeaderExtendedOopClosure no_header_cl(cl); 4.39 oop_iterate(&no_header_cl);
5.1 --- a/src/share/vm/memory/allocation.cpp Wed Nov 28 08:43:26 2012 -0800 5.2 +++ b/src/share/vm/memory/allocation.cpp Wed Nov 28 17:50:21 2012 -0500 5.3 @@ -66,10 +66,17 @@ 5.4 } 5.5 5.6 bool MetaspaceObj::is_metadata() const { 5.7 - // ClassLoaderDataGraph::contains((address)this); has lock inversion problems 5.8 + // GC Verify checks use this in guarantees. 5.9 + // TODO: either replace them with is_metaspace_object() or remove them. 5.10 + // is_metaspace_object() is slower than this test. This test doesn't 5.11 + // seem very useful for metaspace objects anymore though. 5.12 return !Universe::heap()->is_in_reserved(this); 5.13 } 5.14 5.15 +bool MetaspaceObj::is_metaspace_object() const { 5.16 + return Metaspace::contains((void*)this); 5.17 +} 5.18 + 5.19 void MetaspaceObj::print_address_on(outputStream* st) const { 5.20 st->print(" {"INTPTR_FORMAT"}", this); 5.21 }
6.1 --- a/src/share/vm/memory/allocation.hpp Wed Nov 28 08:43:26 2012 -0800 6.2 +++ b/src/share/vm/memory/allocation.hpp Wed Nov 28 17:50:21 2012 -0500 6.3 @@ -245,6 +245,7 @@ 6.4 class MetaspaceObj { 6.5 public: 6.6 bool is_metadata() const; 6.7 + bool is_metaspace_object() const; // more specific test but slower 6.8 bool is_shared() const; 6.9 void print_address_on(outputStream* st) const; // nonvirtual address printing 6.10
7.1 --- a/src/share/vm/memory/metaspace.cpp Wed Nov 28 08:43:26 2012 -0800 7.2 +++ b/src/share/vm/memory/metaspace.cpp Wed Nov 28 17:50:21 2012 -0500 7.3 @@ -36,6 +36,7 @@ 7.4 #include "memory/universe.hpp" 7.5 #include "runtime/globals.hpp" 7.6 #include "runtime/mutex.hpp" 7.7 +#include "runtime/orderAccess.hpp" 7.8 #include "services/memTracker.hpp" 7.9 #include "utilities/copy.hpp" 7.10 #include "utilities/debug.hpp" 7.11 @@ -1007,6 +1008,8 @@ 7.12 delete new_entry; 7.13 return false; 7.14 } else { 7.15 + // ensure lock-free iteration sees fully initialized node 7.16 + OrderAccess::storestore(); 7.17 link_vs(new_entry, vs_word_size); 7.18 return true; 7.19 } 7.20 @@ -1096,7 +1099,6 @@ 7.21 } 7.22 } 7.23 7.24 -#ifndef PRODUCT 7.25 bool VirtualSpaceList::contains(const void *ptr) { 7.26 VirtualSpaceNode* list = virtual_space_list(); 7.27 VirtualSpaceListIterator iter(list); 7.28 @@ -1108,7 +1110,6 @@ 7.29 } 7.30 return false; 7.31 } 7.32 -#endif // PRODUCT 7.33 7.34 7.35 // MetaspaceGC methods 7.36 @@ -2739,15 +2740,17 @@ 7.37 } 7.38 } 7.39 7.40 -#ifndef PRODUCT 7.41 -bool Metaspace::contains(const void * ptr) const { 7.42 +bool Metaspace::contains(const void * ptr) { 7.43 if (MetaspaceShared::is_in_shared_space(ptr)) { 7.44 return true; 7.45 } 7.46 - MutexLockerEx cl(SpaceManager::expand_lock(), Mutex::_no_safepoint_check_flag); 7.47 + // This is checked while unlocked. As long as the virtualspaces are added 7.48 + // at the end, the pointer will be in one of them. The virtual spaces 7.49 + // aren't deleted presently. When they are, some sort of locking might 7.50 + // be needed. Note, locking this can cause inversion problems with the 7.51 + // caller in MetaspaceObj::is_metadata() function. 7.52 return space_list()->contains(ptr) || class_space_list()->contains(ptr); 7.53 } 7.54 -#endif 7.55 7.56 void Metaspace::verify() { 7.57 vsm()->verify();
8.1 --- a/src/share/vm/memory/metaspace.hpp Wed Nov 28 08:43:26 2012 -0800 8.2 +++ b/src/share/vm/memory/metaspace.hpp Wed Nov 28 17:50:21 2012 -0500 8.3 @@ -135,11 +135,7 @@ 8.4 8.5 static bool is_initialized() { return _class_space_list != NULL; } 8.6 8.7 -#ifndef PRODUCT 8.8 - bool contains(const void *ptr) const; 8.9 - bool contains_class(const void *ptr) const; 8.10 -#endif 8.11 - 8.12 + static bool contains(const void *ptr); 8.13 void dump(outputStream* const out) const; 8.14 8.15 void print_on(outputStream* st) const;
9.1 --- a/src/share/vm/memory/universe.cpp Wed Nov 28 08:43:26 2012 -0800 9.2 +++ b/src/share/vm/memory/universe.cpp Wed Nov 28 17:50:21 2012 -0500 9.3 @@ -425,14 +425,10 @@ 9.4 // from MetaspaceObj, because the latter does not have virtual functions. 9.5 // If the metadata type has a vtable, it cannot be shared in the read-only 9.6 // section of the CDS archive, because the vtable pointer is patched. 9.7 -static inline void* dereference(void* addr) { 9.8 - return *(void**)addr; 9.9 -} 9.10 - 9.11 static inline void add_vtable(void** list, int* n, void* o, int count) { 9.12 guarantee((*n) < count, "vtable list too small"); 9.13 - void* vtable = dereference(o); 9.14 - assert(dereference(vtable) != NULL, "invalid vtable"); 9.15 + void* vtable = dereference_vptr(o); 9.16 + assert(*(void**)(vtable) != NULL, "invalid vtable"); 9.17 list[(*n)++] = vtable; 9.18 } 9.19
10.1 --- a/src/share/vm/oops/compiledICHolder.cpp Wed Nov 28 08:43:26 2012 -0800 10.2 +++ b/src/share/vm/oops/compiledICHolder.cpp Wed Nov 28 17:50:21 2012 -0500 10.3 @@ -48,8 +48,8 @@ 10.4 // Verification 10.5 10.6 void CompiledICHolder::verify_on(outputStream* st) { 10.7 - guarantee(holder_method()->is_metadata(), "should be in permspace"); 10.8 + guarantee(holder_method()->is_metadata(), "should be in metaspace"); 10.9 guarantee(holder_method()->is_method(), "should be method"); 10.10 - guarantee(holder_klass()->is_metadata(), "should be in permspace"); 10.11 + guarantee(holder_klass()->is_metadata(), "should be in metaspace"); 10.12 guarantee(holder_klass()->is_klass(), "should be klass"); 10.13 }
11.1 --- a/src/share/vm/oops/method.cpp Wed Nov 28 08:43:26 2012 -0800 11.2 +++ b/src/share/vm/oops/method.cpp Wed Nov 28 17:50:21 2012 -0500 11.3 @@ -1814,6 +1814,23 @@ 11.4 loader_data->jmethod_ids()->clear_all_methods(); 11.5 } 11.6 11.7 + 11.8 +// Check that this pointer is valid by checking that the vtbl pointer matches 11.9 +bool Method::is_valid_method() const { 11.10 + if (this == NULL) { 11.11 + return false; 11.12 + } else if (!is_metaspace_object()) { 11.13 + return false; 11.14 + } else { 11.15 + Method m; 11.16 + // This assumes that the vtbl pointer is the first word of a C++ object. 11.17 + // This assumption is also in universe.cpp patch_klass_vtble 11.18 + void* vtbl2 = dereference_vptr((void*)&m); 11.19 + void* this_vtbl = dereference_vptr((void*)this); 11.20 + return vtbl2 == this_vtbl; 11.21 + } 11.22 +} 11.23 + 11.24 #ifndef PRODUCT 11.25 void Method::print_jmethod_ids(ClassLoaderData* loader_data, outputStream* out) { 11.26 out->print_cr("jni_method_id count = %d", loader_data->jmethod_ids()->count_methods()); 11.27 @@ -1935,7 +1952,7 @@ 11.28 guarantee(constMethod()->is_metadata(), "should be metadata"); 11.29 MethodData* md = method_data(); 11.30 guarantee(md == NULL || 11.31 - md->is_metadata(), "should be in permspace"); 11.32 + md->is_metadata(), "should be metadata"); 11.33 guarantee(md == NULL || 11.34 md->is_methodData(), "should be method data"); 11.35 }
12.1 --- a/src/share/vm/oops/method.hpp Wed Nov 28 08:43:26 2012 -0800 12.2 +++ b/src/share/vm/oops/method.hpp Wed Nov 28 17:50:21 2012 -0500 12.3 @@ -169,7 +169,8 @@ 12.4 ConstMethod::MethodType method_type, 12.5 TRAPS); 12.6 12.7 - Method() { assert(DumpSharedSpaces || UseSharedSpaces, "only for CDS"); } 12.8 + // CDS and vtbl checking can create an empty Method to get vtbl pointer. 12.9 + Method(){} 12.10 12.11 // The Method vtable is restored by this call when the Method is in the 12.12 // shared archive. See patch_klass_vtables() in metaspaceShared.cpp for 12.13 @@ -812,6 +813,9 @@ 12.14 12.15 const char* internal_name() const { return "{method}"; } 12.16 12.17 + // Check for valid method pointer 12.18 + bool is_valid_method() const; 12.19 + 12.20 // Verify 12.21 void verify() { verify_on(tty); } 12.22 void verify_on(outputStream* st);
13.1 --- a/src/share/vm/prims/forte.cpp Wed Nov 28 08:43:26 2012 -0800 13.2 +++ b/src/share/vm/prims/forte.cpp Wed Nov 28 17:50:21 2012 -0500 13.3 @@ -216,10 +216,7 @@ 13.4 // not yet valid. 13.5 13.6 *method_p = method; 13.7 - 13.8 - // See if gc may have invalidated method since we validated frame 13.9 - 13.10 - if (!Universe::heap()->is_valid_method(method)) return false; 13.11 + if (!method->is_valid_method()) return false; 13.12 13.13 intptr_t bcx = fr->interpreter_frame_bcx(); 13.14 13.15 @@ -394,19 +391,11 @@ 13.16 bool fully_decipherable = find_initial_Java_frame(thd, &top_frame, &initial_Java_frame, &method, &bci); 13.17 13.18 // The frame might not be walkable but still recovered a method 13.19 - // (e.g. an nmethod with no scope info for the pc 13.20 + // (e.g. an nmethod with no scope info for the pc) 13.21 13.22 if (method == NULL) return; 13.23 13.24 - CollectedHeap* ch = Universe::heap(); 13.25 - 13.26 - // The method is not stored GC safe so see if GC became active 13.27 - // after we entered AsyncGetCallTrace() and before we try to 13.28 - // use the Method*. 13.29 - // Yes, there is still a window after this check and before 13.30 - // we use Method* below, but we can't lock out GC so that 13.31 - // has to be an acceptable risk. 13.32 - if (!ch->is_valid_method(method)) { 13.33 + if (!method->is_valid_method()) { 13.34 trace->num_frames = ticks_GC_active; // -2 13.35 return; 13.36 } 13.37 @@ -440,13 +429,7 @@ 13.38 bci = st.bci(); 13.39 method = st.method(); 13.40 13.41 - // The method is not stored GC safe so see if GC became active 13.42 - // after we entered AsyncGetCallTrace() and before we try to 13.43 - // use the Method*. 13.44 - // Yes, there is still a window after this check and before 13.45 - // we use Method* below, but we can't lock out GC so that 13.46 - // has to be an acceptable risk. 13.47 - if (!ch->is_valid_method(method)) { 13.48 + if (!method->is_valid_method()) { 13.49 // we throw away everything we've gathered in this sample since 13.50 // none of it is safe 13.51 trace->num_frames = ticks_GC_active; // -2
14.1 --- a/src/share/vm/utilities/globalDefinitions.hpp Wed Nov 28 08:43:26 2012 -0800 14.2 +++ b/src/share/vm/utilities/globalDefinitions.hpp Wed Nov 28 17:50:21 2012 -0500 14.3 @@ -1280,4 +1280,12 @@ 14.4 14.5 #define ARRAY_SIZE(array) (sizeof(array)/sizeof((array)[0])) 14.6 14.7 +// Dereference vptr 14.8 +// All C++ compilers that we know of have the vtbl pointer in the first 14.9 +// word. If there are exceptions, this function needs to be made compiler 14.10 +// specific. 14.11 +static inline void* dereference_vptr(void* addr) { 14.12 + return *(void**)addr; 14.13 +} 14.14 + 14.15 #endif // SHARE_VM_UTILITIES_GLOBALDEFINITIONS_HPP