Add missing card mark verification to CC
Easier than adapting the code in heap.cc to do this. The verification
ensures that objects on clean cards never reference objects in newly
allocated regions.
Revert some changes from aog/341344 that caused the verification to
fail.
Bug: 12687968
Test: test-art-host with CC
Change-Id: Iad583644bb76633ccea0dba87cb383f30adaa80b
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 46f1644..57fc909 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -3798,14 +3798,10 @@
}
void ClassLinker::WriteBarrierForBootOatFileBssRoots(const OatFile* oat_file) {
- if (!kUseReadBarrier) {
- WriterMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
- DCHECK(!oat_file->GetBssGcRoots().empty()) << oat_file->GetLocation();
- if (log_new_roots_ && !ContainsElement(new_bss_roots_boot_oat_files_, oat_file)) {
- new_bss_roots_boot_oat_files_.push_back(oat_file);
- }
- } else {
- LOG(FATAL) << "UNREACHABLE";
+ WriterMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
+ DCHECK(!oat_file->GetBssGcRoots().empty()) << oat_file->GetLocation();
+ if (log_new_roots_ && !ContainsElement(new_bss_roots_boot_oat_files_, oat_file)) {
+ new_bss_roots_boot_oat_files_.push_back(oat_file);
}
}
diff --git a/runtime/entrypoints/quick/quick_dexcache_entrypoints.cc b/runtime/entrypoints/quick/quick_dexcache_entrypoints.cc
index 47c6b51..355d7b3 100644
--- a/runtime/entrypoints/quick/quick_dexcache_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_dexcache_entrypoints.cc
@@ -32,12 +32,9 @@
namespace art {
static inline void BssWriteBarrier(ArtMethod* outer_method) REQUIRES_SHARED(Locks::mutator_lock_) {
- // For non-CC AOT code, we need a write barrier for the class loader that holds the
- // GC roots in the .bss. For CC, we do not need to do anything because the roots
- // we're storing are all referencing to-space and do not need to be re-visited.
- // However, we do the DCHECK() for the registration of oat files with .bss sections.
- const DexFile* dex_file =
- (kUseReadBarrier && !kIsDebugBuild) ? nullptr : outer_method->GetDexFile();
+ // For AOT code, we need a write barrier for the class loader that holds the
+ // GC roots in the .bss.
+ const DexFile* dex_file = outer_method->GetDexFile();
if (dex_file != nullptr &&
dex_file->GetOatDexFile() != nullptr &&
!dex_file->GetOatDexFile()->GetOatFile()->GetBssGcRoots().empty()) {
@@ -50,15 +47,13 @@
<< "Oat file with .bss GC roots was not registered in class table: "
<< dex_file->GetOatDexFile()->GetOatFile()->GetLocation();
}
- if (!kUseReadBarrier) {
- if (class_loader != nullptr) {
- // Note that we emit the barrier before the compiled code stores the String or Class
- // as a GC root. This is OK as there is no suspend point point in between.
- Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(class_loader);
- } else {
- Runtime::Current()->GetClassLinker()->WriteBarrierForBootOatFileBssRoots(
- dex_file->GetOatDexFile()->GetOatFile());
- }
+ if (class_loader != nullptr) {
+ // Note that we emit the barrier before the compiled code stores the String or Class
+ // as a GC root. This is OK as there is no suspend point point in between.
+ Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(class_loader);
+ } else {
+ Runtime::Current()->GetClassLinker()->WriteBarrierForBootOatFileBssRoots(
+ dex_file->GetOatDexFile()->GetOatFile());
}
}
}
diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index f18ffb4..b939cbe 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -53,6 +53,8 @@
// Slow path mark stack size, increase this if the stack is getting full and it is causing
// performance problems.
static constexpr size_t kReadBarrierMarkStackSize = 512 * KB;
+// Verify that there are no missing card marks.
+static constexpr bool kVerifyNoMissingCardMarks = kIsDebugBuild;
ConcurrentCopying::ConcurrentCopying(Heap* heap,
const std::string& name_prefix,
@@ -318,6 +320,9 @@
TimingLogger::ScopedTiming split("(Paused)FlipCallback", cc->GetTimings());
// Note: self is not necessarily equal to thread since thread may be suspended.
Thread* self = Thread::Current();
+ if (kVerifyNoMissingCardMarks) {
+ cc->VerifyNoMissingCardMarks();
+ }
CHECK(thread == self);
Locks::mutator_lock_->AssertExclusiveHeld(self);
cc->region_space_->SetFromSpace(cc->rb_table_, cc->force_evacuate_all_);
@@ -428,6 +433,72 @@
}
}
+class ConcurrentCopying::VerifyNoMissingCardMarkVisitor {
+ public:
+ VerifyNoMissingCardMarkVisitor(ConcurrentCopying* cc, ObjPtr<mirror::Object> holder)
+ : cc_(cc),
+ holder_(holder) {}
+
+ void operator()(ObjPtr<mirror::Object> obj,
+ MemberOffset offset,
+ bool is_static ATTRIBUTE_UNUSED) const
+ REQUIRES_SHARED(Locks::mutator_lock_) ALWAYS_INLINE {
+ if (offset.Uint32Value() != mirror::Object::ClassOffset().Uint32Value()) {
+ CheckReference(obj->GetFieldObject<mirror::Object, kDefaultVerifyFlags, kWithoutReadBarrier>(
+ offset), offset.Uint32Value());
+ }
+ }
+ void operator()(ObjPtr<mirror::Class> klass,
+ ObjPtr<mirror::Reference> ref) const
+ REQUIRES_SHARED(Locks::mutator_lock_) ALWAYS_INLINE {
+ CHECK(klass->IsTypeOfReferenceClass());
+ this->operator()(ref, mirror::Reference::ReferentOffset(), false);
+ }
+
+ void VisitRootIfNonNull(mirror::CompressedReference<mirror::Object>* root) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ if (!root->IsNull()) {
+ VisitRoot(root);
+ }
+ }
+
+ void VisitRoot(mirror::CompressedReference<mirror::Object>* root) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ CheckReference(root->AsMirrorPtr());
+ }
+
+ void CheckReference(mirror::Object* ref, uint32_t offset = 0) const
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ CHECK(ref == nullptr || !cc_->region_space_->IsInNewlyAllocatedRegion(ref))
+ << holder_->PrettyTypeOf() << "(" << holder_.Ptr() << ") references object "
+ << ref->PrettyTypeOf() << "(" << ref << ") in newly allocated region at offset=" << offset;
+ }
+
+ private:
+ ConcurrentCopying* const cc_;
+ ObjPtr<mirror::Object> const holder_;
+};
+
+void ConcurrentCopying::VerifyNoMissingCardMarkCallback(mirror::Object* obj, void* arg) {
+ auto* collector = reinterpret_cast<ConcurrentCopying*>(arg);
+ // Objects not on dirty cards should never have references to newly allocated regions.
+ if (!collector->heap_->GetCardTable()->IsDirty(obj)) {
+ VerifyNoMissingCardMarkVisitor visitor(collector, /*holder*/ obj);
+ obj->VisitReferences</*kVisitNativeRoots*/true, kVerifyNone, kWithoutReadBarrier>(
+ visitor,
+ visitor);
+ }
+}
+
+void ConcurrentCopying::VerifyNoMissingCardMarks() {
+ TimingLogger::ScopedTiming split(__FUNCTION__, GetTimings());
+ region_space_->Walk(&VerifyNoMissingCardMarkCallback, this);
+ {
+ ReaderMutexLock rmu(Thread::Current(), *Locks::heap_bitmap_lock_);
+ heap_->GetLiveBitmap()->Walk(&VerifyNoMissingCardMarkCallback, this);
+ }
+}
+
// Switch threads that from from-space to to-space refs. Forward/mark the thread roots.
void ConcurrentCopying::FlipThreadRoots() {
TimingLogger::ScopedTiming split("FlipThreadRoots", GetTimings());
@@ -2330,7 +2401,9 @@
MutexLock mu(self, mark_stack_lock_);
CHECK_EQ(pooled_mark_stacks_.size(), kMarkStackPoolSize);
}
- {
+ // kVerifyNoMissingCardMarks relies on the region space cards not being cleared to avoid false
+ // positives.
+ if (!kVerifyNoMissingCardMarks) {
TimingLogger::ScopedTiming split("ClearRegionSpaceCards", GetTimings());
// We do not currently use the region space cards at all, madvise them away to save ram.
heap_->GetCardTable()->ClearCardRange(region_space_->Begin(), region_space_->Limit());
diff --git a/runtime/gc/collector/concurrent_copying.h b/runtime/gc/collector/concurrent_copying.h
index 844bb45..11060a8 100644
--- a/runtime/gc/collector/concurrent_copying.h
+++ b/runtime/gc/collector/concurrent_copying.h
@@ -162,6 +162,12 @@
void VerifyGrayImmuneObjects()
REQUIRES(Locks::mutator_lock_)
REQUIRES(!mark_stack_lock_);
+ static void VerifyNoMissingCardMarkCallback(mirror::Object* obj, void* arg)
+ REQUIRES(Locks::mutator_lock_)
+ REQUIRES(!mark_stack_lock_);
+ void VerifyNoMissingCardMarks()
+ REQUIRES(Locks::mutator_lock_)
+ REQUIRES(!mark_stack_lock_);
size_t ProcessThreadLocalMarkStacks(bool disable_weak_ref_access, Closure* checkpoint_callback)
REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(!mark_stack_lock_);
void RevokeThreadLocalMarkStacks(bool disable_weak_ref_access, Closure* checkpoint_callback)
@@ -329,6 +335,7 @@
class VerifyNoFromSpaceRefsFieldVisitor;
class VerifyNoFromSpaceRefsObjectVisitor;
class VerifyNoFromSpaceRefsVisitor;
+ class VerifyNoMissingCardMarkVisitor;
DISALLOW_IMPLICIT_CONSTRUCTORS(ConcurrentCopying);
};
diff --git a/runtime/gc/space/region_space.h b/runtime/gc/space/region_space.h
index f3b9595..feab9b0 100644
--- a/runtime/gc/space/region_space.h
+++ b/runtime/gc/space/region_space.h
@@ -176,6 +176,14 @@
return false;
}
+ bool IsInNewlyAllocatedRegion(mirror::Object* ref) {
+ if (HasAddress(ref)) {
+ Region* r = RefToRegionUnlocked(ref);
+ return r->IsNewlyAllocated();
+ }
+ return false;
+ }
+
bool IsInUnevacFromSpace(mirror::Object* ref) {
if (HasAddress(ref)) {
Region* r = RefToRegionUnlocked(ref);
@@ -351,6 +359,10 @@
return idx_;
}
+ bool IsNewlyAllocated() const {
+ return is_newly_allocated_;
+ }
+
bool IsInFromSpace() const {
return type_ == RegionType::kRegionTypeFromSpace;
}