Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8237363: Remove automatic is in heap verification in OopIterateClosure #797

Closed
wants to merge 4 commits into from
Closed

8237363: Remove automatic is in heap verification in OopIterateClosure #797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd33ae3
8237363: Remove automatic is in heap verification in OopIterateClosure
stefank Sep 29, 2020
b618149
Merge branch 'master' into 8237363_remove_oop_iterate_verification
stefank Oct 21, 2020
7ff6412
Merge branch 'master' into 8237363_remove_oop_iterate_verification
stefank Oct 22, 2020
cac794c
Remove assert macros
stefank Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/g1/heapRegion.cpp
View file
Open in desktop
Expand Up @@ -516,9 +516,6 @@ class G1VerificationClosure : public BasicOopIterateClosure {
obj->print_on(out);
#endif // PRODUCT
}

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
};

class VerifyLiveClosure : public G1VerificationClosure {
Expand Down Expand Up @@ -655,9 +652,6 @@ class G1Mux2Closure : public BasicOopIterateClosure {
}
virtual inline void do_oop(oop* p) { do_oop_work(p); }
virtual inline void do_oop(narrowOop* p) { do_oop_work(p); }

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
};

void HeapRegion::verify(VerifyOption vo,
Expand Down
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/parallel/psCompactionManager.inline.hpp
View file
Open in desktop
Expand Up @@ -47,9 +47,6 @@ class PCMarkAndPushClosure: public OopClosure {
template <typename T> void do_oop_nv(T* p) { _compaction_manager->mark_and_push(p); }
virtual void do_oop(oop* p) { do_oop_nv(p); }
virtual void do_oop(narrowOop* p) { do_oop_nv(p); }

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
};

class PCIterateMarkAndPushClosure: public MetadataVisitingOopIterateClosure {
Expand All @@ -64,9 +61,6 @@ class PCIterateMarkAndPushClosure: public MetadataVisitingOopIterateClosure {

void do_klass_nv(Klass* k) { _compaction_manager->follow_klass(k); }
void do_cld_nv(ClassLoaderData* cld) { _compaction_manager->follow_class_loader(cld); }

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
};

inline bool ParCompactionManager::steal(int queue_num, oop& t) {
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/gc/parallel/psParallelCompact.inline.hpp
View file
Open in desktop
Expand Up @@ -134,8 +134,6 @@ class PCAdjustPointerClosure: public BasicOopIterateClosure {
virtual void do_oop(oop* p) { do_oop_nv(p); }
virtual void do_oop(narrowOop* p) { do_oop_nv(p); }

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
virtual ReferenceIterationMode reference_iteration_mode() { return DO_FIELDS; }
private:
ParCompactionManager* _cm;
Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp
View file
Open in desktop
Expand Up @@ -97,9 +97,6 @@ class PSPushContentsClosure: public BasicOopIterateClosure {

virtual void do_oop(oop* p) { do_oop_nv(p); }
virtual void do_oop(narrowOop* p) { do_oop_nv(p); }

// Don't use the oop verification code in the oop_oop_iterate framework.
debug_only(virtual bool should_verify_oops() { return false; })
};

//
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/gc/serial/defNewGeneration.inline.hpp
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ inline void DefNewGeneration::KeepAliveClosure::do_oop_work(T* p) {
}
#endif // ASSERT

Devirtualizer::do_oop_no_verify(_cl, p);
Devirtualizer::do_oop(_cl, p);

// Card marking is trickier for weak refs.
// This oop is a 'next' field which was filled in while we
Expand Down Expand Up @@ -77,7 +77,7 @@ inline void DefNewGeneration::FastKeepAliveClosure::do_oop_work(T* p) {
}
#endif // ASSERT

Devirtualizer::do_oop_no_verify(_cl, p);
Devirtualizer::do_oop(_cl, p);

// Optimized for Defnew generation if it's the youngest generation:
// we set a younger_gen card if we have an older->youngest
Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/gc/serial/markSweep.hpp
View file
Open in desktop
Expand Up @@ -191,9 +191,6 @@ class AdjustPointerClosure: public BasicOopIterateClosure {
virtual void do_oop(oop* p);
virtual void do_oop(narrowOop* p);
virtual ReferenceIterationMode reference_iteration_mode() { return DO_FIELDS; }

// This closure provides its own oop verification code.
debug_only(virtual bool should_verify_oops() { return false; })
};

class PreservedMark {
Expand Down
6 changes: 0 additions & 6 deletions src/hotspot/share/gc/z/zHeapIterator.cpp
View file
Open in desktop
Expand Up @@ -160,12 +160,6 @@ class ZHeapIteratorOopClosure : public ClaimMetadataVisitingOopIterateClosure {
virtual void do_oop(narrowOop* p) {
ShouldNotReachHere();
}

#ifdef ASSERT
virtual bool should_verify_oops() {
return false;
}
#endif
};

ZHeapIterator::ZHeapIterator(uint nworkers, bool visit_weaks) :
Expand Down
12 changes: 0 additions & 12 deletions src/hotspot/share/gc/z/zOopClosures.hpp
View file
Open in desktop
Expand Up @@ -31,12 +31,6 @@ class ZLoadBarrierOopClosure : public BasicOopIterateClosure {
public:
virtual void do_oop(oop* p);
virtual void do_oop(narrowOop* p);

#ifdef ASSERT
virtual bool should_verify_oops() {
return false;
}
#endif
};

class ZNMethodOopClosure : public OopClosure {
Expand All @@ -52,12 +46,6 @@ class ZMarkBarrierOopClosure : public ClaimMetadataVisitingOopIterateClosure {

virtual void do_oop(oop* p);
virtual void do_oop(narrowOop* p);

#ifdef ASSERT
virtual bool should_verify_oops() {
return false;
}
#endif
};

class ZPhantomIsAliveObjectClosure : public BoolObjectClosure {
Expand Down
7 changes: 0 additions & 7 deletions src/hotspot/share/gc/z/zVerify.cpp
View file
Open in desktop
Expand Up @@ -214,13 +214,6 @@ class ZVerifyOopClosure : public ClaimMetadataVisitingOopIterateClosure, public
virtual ReferenceIterationMode reference_iteration_mode() {
return _verify_weaks ? DO_FIELDS : DO_FIELDS_EXCEPT_REFERENT;
}

#ifdef ASSERT
// Verification handled by the closure itself
virtual bool should_verify_oops() {
return false;
}
#endif
};

template <typename RootsIterator>
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/jfr/leakprofiler/chains/bfsClosure.hpp
View file
Open in desktop
Expand Up @@ -64,7 +64,6 @@ class BFSClosure : public BasicOopIterateClosure {

public:
virtual ReferenceIterationMode reference_iteration_mode() { return DO_FIELDS_EXCEPT_REFERENT; }
virtual bool should_verify_oops() { return false; }

BFSClosure(EdgeQueue* edge_queue, EdgeStore* edge_store, BitSet* mark_bits);
void process();
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/jfr/leakprofiler/chains/dfsClosure.hpp
View file
Open in desktop
Expand Up @@ -54,7 +54,6 @@ class DFSClosure : public BasicOopIterateClosure {

public:
virtual ReferenceIterationMode reference_iteration_mode() { return DO_FIELDS_EXCEPT_REFERENT; }
virtual bool should_verify_oops() { return false; }

static void find_leaks_from_edge(EdgeStore* edge_store, BitSet* mark_bits, const Edge* start_edge);
static void find_leaks_from_root_set(EdgeStore* edge_store, BitSet* mark_bits);
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/memory/filemap.cpp
View file
Open in desktop
Expand Up @@ -1737,7 +1737,7 @@ address FileMapInfo::decode_start_address(FileMapRegion* spc, bool with_current_
size_t offset = spc->mapping_offset();
narrowOop n = CompressedOops::narrow_oop_cast(offset);
if (with_current_oop_encoding_mode) {
return cast_from_oop<address>(CompressedOops::decode_not_null(n));
return cast_from_oop<address>(CompressedOops::decode_raw_not_null(n));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this line skip verification now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is used to setup the CDS archive. At that point in time, the heap isn't mapped yet, and there's no heap region at the suggested offset, so the new assert fails.

HeapRegion::is_in_reserved (this=0x0, p=0x7bfe00000)
HeapRegion::is_in (this=0x0, p=0x7bfe00000)
G1CollectedHeap::is_in (this=0x7ffff003a100, p=0x7bfe00000)
CompressedOops::decode_not_null (v=(unknown: -134479872))
FileMapInfo::decode_start_address (this=0x7ffff05d3e60, spc=0x7ffff05d4000, with_current_oop_encoding_mode=true)
FileMapInfo::start_address_as_decoded_with_current_oop_encoding_mode
FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode
FileMapInfo::map_heap_regions_impl
FileMapInfo::map_heap_regions
MetaspaceShared::map_archives
MetaspaceShared::initialize_runtime_shared_and_meta_spaces ()
Metaspace::global_initialize
universe_init ()
init_globals ()
Threads::create_vm

Note that the heap region is null (this=0x0). This also mean that the returned oop is not actually a valid oop at all, and this can sort of be seen by the immediate cast to address.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew. That seems to imply that coops decoding is now tied to heap initialization for these asserts to work. I am pleasantly surprised it only fails in one place! I guess it is fine.

} else {
return cast_from_oop<address>(HeapShared::decode_from_archive(n));
}
Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/memory/heapShared.cpp
View file
Open in desktop
Expand Up @@ -1150,9 +1150,6 @@ class FindEmbeddedNonNullPointers: public BasicOopIterateClosure {
FindEmbeddedNonNullPointers(narrowOop* start, BitMap* oopmap)
: _start(start), _oopmap(oopmap), _num_total_oops(0), _num_null_oops(0) {}

virtual bool should_verify_oops(void) {
return false;
}
virtual void do_oop(narrowOop* p) {
_num_total_oops ++;
narrowOop v = *p;
Expand Down
9 changes: 0 additions & 9 deletions src/hotspot/share/memory/iterator.hpp
View file
Open in desktop
Expand Up @@ -102,14 +102,6 @@ class OopIterateClosure : public OopClosure {
virtual bool do_metadata() = 0;
virtual void do_klass(Klass* k) = 0;
virtual void do_cld(ClassLoaderData* cld) = 0;

#ifdef ASSERT
// Default verification of each visited oop field.
template <typename T> void verify(T* p);

// Can be used by subclasses to turn off the default verification of oop fields.
virtual bool should_verify_oops() { return true; }
#endif
};

// An OopIterateClosure that can be used when there's no need to visit the Metadata.
Expand Down Expand Up @@ -357,7 +349,6 @@ class CompareClosure : public Closure {
// a concrete implementation, otherwise a virtual call is taken.
class Devirtualizer {
public:
template <typename OopClosureType, typename T> static void do_oop_no_verify(OopClosureType* closure, T* p);
template <typename OopClosureType, typename T> static void do_oop(OopClosureType* closure, T* p);
template <typename OopClosureType> static void do_klass(OopClosureType* closure, Klass* k);
template <typename OopClosureType> static void do_cld(OopClosureType* closure, ClassLoaderData* cld);
Expand Down
26 changes: 1 addition & 25 deletions src/hotspot/share/memory/iterator.inline.hpp
View file
Open in desktop
Expand Up @@ -27,7 +27,6 @@

#include "classfile/classLoaderData.hpp"
#include "memory/iterator.hpp"
#include "memory/universe.hpp"
#include "oops/access.inline.hpp"
#include "oops/compressedOops.inline.hpp"
#include "oops/klass.hpp"
Expand All @@ -52,22 +51,6 @@ inline void ClaimMetadataVisitingOopIterateClosure::do_klass(Klass* k) {
ClaimMetadataVisitingOopIterateClosure::do_cld(cld);
}

#ifdef ASSERT
// This verification is applied to all visited oops.
// The closures can turn is off by overriding should_verify_oops().
template <typename T>
void OopIterateClosure::verify(T* p) {
if (should_verify_oops()) {
T heap_oop = RawAccess<>::oop_load(p);
if (!CompressedOops::is_null(heap_oop)) {
oop o = CompressedOops::decode_not_null(heap_oop);
assert(Universe::heap()->is_in(o),
"should be in closed *p " PTR_FORMAT " " PTR_FORMAT, p2i(p), p2i(o));
}
}
}
#endif

// Implementation of the non-virtual do_oop dispatch.
//
// The same implementation is used for do_metadata, do_klass, and do_cld.
Expand Down Expand Up @@ -123,16 +106,9 @@ call_do_oop(void (Receiver::*)(T*), void (Base::*)(T*), OopClosureType* closure,
closure->OopClosureType::do_oop(p);
}

template <typename OopClosureType, typename T>
inline void Devirtualizer::do_oop_no_verify(OopClosureType* closure, T* p) {
call_do_oop<T>(&OopClosureType::do_oop, &OopClosure::do_oop, closure, p);
}

template <typename OopClosureType, typename T>
inline void Devirtualizer::do_oop(OopClosureType* closure, T* p) {
debug_only(closure->verify(p));

do_oop_no_verify(closure, p);
call_do_oop<T>(&OopClosureType::do_oop, &OopClosure::do_oop, closure, p);
}

// Implementation of the non-virtual do_metadata dispatch.
Expand Down
9 changes: 5 additions & 4 deletions src/hotspot/share/oops/compressedOops.hpp
View file
Open in desktop
Expand Up @@ -119,17 +119,18 @@ class CompressedOops : public AllStatic {
static bool is_null(oop v) { return v == NULL; }
static bool is_null(narrowOop v) { return v == narrowOop::null; }

static inline oop decode_raw_not_null(narrowOop v);
static inline oop decode_raw(narrowOop v);
static inline oop decode_not_null(narrowOop v);
static inline oop decode(narrowOop v);
static inline narrowOop encode_not_null(oop v);
static inline narrowOop encode(oop v);

// No conversions needed for these overloads
static oop decode_not_null(oop v) { return v; }
static oop decode(oop v) { return v; }
static narrowOop encode_not_null(narrowOop v) { return v; }
static narrowOop encode(narrowOop v) { return v; }
static inline oop decode_not_null(oop v);
static inline oop decode(oop v);
static inline narrowOop encode_not_null(narrowOop v);
static inline narrowOop encode(narrowOop v);

static inline uint32_t narrow_oop_value(oop o);
static inline uint32_t narrow_oop_value(narrowOop o);
Expand Down
26 changes: 25 additions & 1 deletion src/hotspot/share/oops/compressedOops.inline.hpp
View file
Open in desktop
Expand Up @@ -41,6 +41,11 @@
// offset from the heap base. Saving the check for null can save instructions
// in inner GC loops so these are separated.

inline oop CompressedOops::decode_raw_not_null(narrowOop v) {
assert(!is_null(v), "narrow oop value can never be zero");
return decode_raw(v);
}

inline oop CompressedOops::decode_raw(narrowOop v) {
return (oop)(void*)((uintptr_t)base() + ((uintptr_t)v << shift()));
}
Expand All @@ -49,6 +54,7 @@ inline oop CompressedOops::decode_not_null(narrowOop v) {
assert(!is_null(v), "narrow oop value can never be zero");
oop result = decode_raw(v);
assert(is_object_aligned(result), "address not aligned: " INTPTR_FORMAT, p2i((void*) result));
assert(Universe::heap()->is_in(result), "object not in heap " PTR_FORMAT, p2i((void*) result));
return result;
}

Expand All @@ -63,14 +69,32 @@ inline narrowOop CompressedOops::encode_not_null(oop v) {
uint64_t pd = (uint64_t)(pointer_delta((void*)v, (void*)base(), 1));
assert(OopEncodingHeapMax > pd, "change encoding max if new encoding");
narrowOop result = narrow_oop_cast(pd >> shift());
assert(decode(result) == v, "reversibility");
assert(decode_raw(result) == v, "reversibility");
return result;
}

inline narrowOop CompressedOops::encode(oop v) {
return is_null(v) ? narrowOop::null : encode_not_null(v);
}

inline oop CompressedOops::decode_not_null(oop v) {
assert(Universe::heap()->is_in(v), "object not in heap " PTR_FORMAT, p2i((void*) v));
return v;
}

inline oop CompressedOops::decode(oop v) {
assert(Universe::heap()->is_in_or_null(v), "object not in heap " PTR_FORMAT, p2i((void*) v));
return v;
}

inline narrowOop CompressedOops::encode_not_null(narrowOop v) {
return v;
}

inline narrowOop CompressedOops::encode(narrowOop v) {
return v;
}

inline uint32_t CompressedOops::narrow_oop_value(oop o) {
return narrow_oop_value(encode(o));
}
Expand Down