Address read barrier issue with cl/106467

And tidy/add a check.

Bug: 12687968
Change-Id: If63dc0d9d0a0ce5f2eeb81734ff8f4307865f67d

6 files changed, 18 insertions, 20 deletions

diff --git a/runtime/indirect_reference_table-inl.h b/runtime/indirect_reference_table-inl.h
index 9bf3ea258..9ee6d897a 100644
--- a/runtime/indirect_reference_table-inl.h
+++ b/runtime/indirect_reference_table-inl.h
@@ -27,15 +27,6 @@ namespace mirror {
 class Object;
 }  // namespace mirror
 
-inline void IrtIterator::SkipNullsAndTombstones() {
-  // We skip NULLs and tombstones. Clients don't want to see implementation details.
-  while (i_ < capacity_ &&
-         (table_[i_].IsNull() ||
-          Runtime::Current()->IsClearedJniWeakGlobal(table_[i_].Read<kWithoutReadBarrier>()))) {
-    ++i_;
-  }
-}
-
 // Verifies that the indirect table lookup is valid.
 // Returns "false" if something looks bad.
 inline bool IndirectReferenceTable::GetChecked(IndirectRef iref) const {
diff --git a/runtime/indirect_reference_table.cc b/runtime/indirect_reference_table.cc
index 49bffa404..2278408a5 100644
--- a/runtime/indirect_reference_table.cc
+++ b/runtime/indirect_reference_table.cc
@@ -161,10 +161,12 @@ IndirectRef IndirectReferenceTable::Add(uint32_t cookie, mirror::Object* obj) {
 }
 
 void IndirectReferenceTable::AssertEmpty() {
-  if (UNLIKELY(begin() != end())) {
-    ScopedObjectAccess soa(Thread::Current());
-    LOG(FATAL) << "Internal Error: non-empty local reference table\n"
-               << MutatorLockedDumpable<IndirectReferenceTable>(*this);
+  for (size_t i = 0; i < Capacity(); ++i) {
+    if (!table_[i].IsNull()) {
+      ScopedObjectAccess soa(Thread::Current());
+      LOG(FATAL) << "Internal Error: non-empty local reference table\n"
+                 << MutatorLockedDumpable<IndirectReferenceTable>(*this);
+    }
   }
 }
 
@@ -264,6 +266,11 @@ bool IndirectReferenceTable::Remove(uint32_t cookie, IndirectRef iref) {
 void IndirectReferenceTable::VisitRoots(RootCallback* callback, void* arg, uint32_t tid,
                                         RootType root_type) {
   for (auto ref : *this) {
+    if (*ref == nullptr) {
+      // Need to skip null entries to make it possible to do the
+      // non-null check after the call back.
+      continue;
+    }
     callback(ref, arg, tid, root_type);
     DCHECK(*ref != nullptr);
   }
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index 562ba1e8d..5291e508a 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -206,12 +206,10 @@ class IrtIterator {
   explicit IrtIterator(GcRoot<mirror::Object>* table, size_t i, size_t capacity)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       : table_(table), i_(i), capacity_(capacity) {
-    SkipNullsAndTombstones();
   }
 
   IrtIterator& operator++() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     ++i_;
-    SkipNullsAndTombstones();
     return *this;
   }
 
@@ -225,8 +223,6 @@ class IrtIterator {
   }
 
  private:
-  void SkipNullsAndTombstones() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
-
   GcRoot<mirror::Object>* const table_;
   size_t i_;
   const size_t capacity_;
diff --git a/runtime/java_vm_ext.cc b/runtime/java_vm_ext.cc
index 424addb2a..0ac5b88e8 100644
--- a/runtime/java_vm_ext.cc
+++ b/runtime/java_vm_ext.cc
@@ -760,6 +760,11 @@ void JavaVMExt::SweepJniWeakGlobals(IsMarkedCallback* callback, void* arg) {
   for (mirror::Object** entry : weak_globals_) {
     // Since this is called by the GC, we don't need a read barrier.
     mirror::Object* obj = *entry;
+    if (obj == nullptr) {
+      // Need to skip null here to distinguish between null entries
+      // and cleared weak ref entries.
+      continue;
+    }
     mirror::Object* new_obj = callback(obj, arg);
     if (new_obj == nullptr) {
       new_obj = Runtime::Current()->GetClearedJniWeakGlobal();
diff --git a/runtime/runtime-inl.h b/runtime/runtime-inl.h
index 8b632b245..fe05073fa 100644
--- a/runtime/runtime-inl.h
+++ b/runtime/runtime-inl.h
@@ -29,9 +29,7 @@ inline bool Runtime::IsClearedJniWeakGlobal(mirror::Object* obj) {
 
 inline mirror::Object* Runtime::GetClearedJniWeakGlobal() {
   mirror::Object* obj = sentinel_.Read();
-  if (obj == nullptr) {
-    LOG(ERROR) << "Failed to return cleared JNI weak global sentinel";
-  }
+  DCHECK(obj != nullptr);
   return obj;
 }
 
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 89ad50523..f258a309e 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -809,6 +809,7 @@ bool Runtime::Init(const RuntimeOptions& raw_options, bool ignore_unrecognized)
 
   // Initialize the special sentinel_ value early.
   sentinel_ = GcRoot<mirror::Object>(class_linker_->AllocObject(self));
+  CHECK(sentinel_.Read() != nullptr);
 
   verifier::MethodVerifier::Init();