8003635: NPG: AsynchGetCallTrace broken by Method* virtual call

Wed, 28 Nov 2012 17:50:21 -0500

author
coleenp
date
Wed, 28 Nov 2012 17:50:21 -0500
changeset 4295
59c790074993
parent 4294
b51dc8df86e5
child 4301
c24f778e9401

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

src/cpu/sparc/vm/frame_sparc.cpp file | annotate | diff | comparison | revisions
src/cpu/x86/vm/frame_x86.cpp file | annotate | diff | comparison | revisions
src/share/vm/gc_interface/collectedHeap.hpp file | annotate | diff | comparison | revisions
src/share/vm/gc_interface/collectedHeap.inline.hpp file | annotate | diff | comparison | revisions
src/share/vm/memory/allocation.cpp file | annotate | diff | comparison | revisions
src/share/vm/memory/allocation.hpp file | annotate | diff | comparison | revisions
src/share/vm/memory/metaspace.cpp file | annotate | diff | comparison | revisions
src/share/vm/memory/metaspace.hpp file | annotate | diff | comparison | revisions
src/share/vm/memory/universe.cpp file | annotate | diff | comparison | revisions
src/share/vm/oops/compiledICHolder.cpp file | annotate | diff | comparison | revisions
src/share/vm/oops/method.cpp file | annotate | diff | comparison | revisions
src/share/vm/oops/method.hpp file | annotate | diff | comparison | revisions
src/share/vm/prims/forte.cpp file | annotate | diff | comparison | revisions
src/share/vm/utilities/globalDefinitions.hpp file | annotate | diff | comparison | revisions
     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

mercurial