Revert "Revert "Add missing card mark verification to CC""
Add missing write barrier for AddStrongRoot on the dex cache.
Test: test-art-host-run-test ART_TEST_INTERPRETER=true ART_TEST_OPTIMIZING=true ART_TEST_GC_STRESS=true
Bug: 12687968
This reverts commit 50805e747cbb7e9c8d30bd3b49e27ab0741f3cf8.
Change-Id: I72c6de2120d8e0ddc2512dd41010776aecfc9e2c
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 46f1644..9b0ffaf 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -3471,6 +3471,11 @@
return nullptr;
}
table->InsertStrongRoot(h_dex_cache.Get());
+ if (h_class_loader.Get() != nullptr) {
+ // Since we added a strong root to the class table, do the write barrier as required for
+ // remembered sets and generational GCs.
+ Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(h_class_loader.Get());
+ }
return h_dex_cache.Get();
}
@@ -3798,14 +3803,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 3d2fd0b..ebe625b 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,
@@ -335,6 +337,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_);
@@ -445,6 +450,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, int32_t offset = -1) 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());
@@ -2347,7 +2418,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 073326d..a0da9fc 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)
@@ -330,6 +336,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;
}
