Add read barriers to the weak roots in the intern table.

Bug: 12687968
Change-Id: I424f1df76a7e3d7154fb9f3c951c973d19bd640f

6 files changed, 68 insertions, 24 deletions

diff --git a/runtime/intern_table.cc b/runtime/intern_table.cc
index 339eb36..f12043e 100644
--- a/runtime/intern_table.cc
+++ b/runtime/intern_table.cc
@@ -82,11 +82,25 @@ void InternTable::VisitRoots(RootCallback* callback, void* arg, VisitRootFlags f
   // Note: we deliberately don't visit the weak_interns_ table and the immutable image roots.
 }
 
-mirror::String* InternTable::Lookup(Table& table, mirror::String* s, int32_t hash_code) {
+mirror::String* InternTable::LookupStrong(mirror::String* s, int32_t hash_code) {
+  return Lookup<kWithoutReadBarrier>(&strong_interns_, s, hash_code);
+}
+
+mirror::String* InternTable::LookupWeak(mirror::String* s, int32_t hash_code) {
+  // Weak interns need a read barrier because they are weak roots.
+  return Lookup<kWithReadBarrier>(&weak_interns_, s, hash_code);
+}
+
+template<ReadBarrierOption kReadBarrierOption>
+mirror::String* InternTable::Lookup(Table* table, mirror::String* s, int32_t hash_code) {
+  CHECK_EQ(table == &weak_interns_, kReadBarrierOption == kWithReadBarrier)
+      << "Only weak_interns_ needs a read barrier.";
   Locks::intern_table_lock_->AssertHeld(Thread::Current());
-  for (auto it = table.lower_bound(hash_code), end = table.end();
+  for (auto it = table->lower_bound(hash_code), end = table->end();
        it != end && it->first == hash_code; ++it) {
-    mirror::String* existing_string = it->second;
+    mirror::String** weak_root = &it->second;
+    mirror::String* existing_string =
+        ReadBarrier::BarrierForWeakRoot<mirror::String, kReadBarrierOption>(weak_root);
     if (existing_string->Equals(s)) {
       return existing_string;
     }
@@ -115,19 +129,29 @@ mirror::String* InternTable::InsertWeak(mirror::String* s, int32_t hash_code) {
   return s;
 }
 
+void InternTable::RemoveStrong(mirror::String* s, int32_t hash_code) {
+  Remove<kWithoutReadBarrier>(&strong_interns_, s, hash_code);
+}
+
 void InternTable::RemoveWeak(mirror::String* s, int32_t hash_code) {
   Runtime* runtime = Runtime::Current();
   if (runtime->IsActiveTransaction()) {
     runtime->RecordWeakStringRemoval(s, hash_code);
   }
-  Remove(weak_interns_, s, hash_code);
+  Remove<kWithReadBarrier>(&weak_interns_, s, hash_code);
 }
 
-void InternTable::Remove(Table& table, mirror::String* s, int32_t hash_code) {
-  for (auto it = table.lower_bound(hash_code), end = table.end();
+template<ReadBarrierOption kReadBarrierOption>
+void InternTable::Remove(Table* table, mirror::String* s, int32_t hash_code) {
+  CHECK_EQ(table == &weak_interns_, kReadBarrierOption == kWithReadBarrier)
+      << "Only weak_interns_ needs a read barrier.";
+  for (auto it = table->lower_bound(hash_code), end = table->end();
        it != end && it->first == hash_code; ++it) {
-    if (it->second == s) {
-      table.erase(it);
+    mirror::String** weak_root = &it->second;
+    mirror::String* existing_string =
+        ReadBarrier::BarrierForWeakRoot<mirror::String, kReadBarrierOption>(weak_root);
+    if (existing_string == s) {
+      table->erase(it);
       return;
     }
   }
@@ -144,11 +168,11 @@ mirror::String* InternTable::InsertWeakFromTransaction(mirror::String* s, int32_
 }
 void InternTable::RemoveStrongFromTransaction(mirror::String* s, int32_t hash_code) {
   DCHECK(!Runtime::Current()->IsActiveTransaction());
-  Remove(strong_interns_, s, hash_code);
+  RemoveStrong(s, hash_code);
 }
 void InternTable::RemoveWeakFromTransaction(mirror::String* s, int32_t hash_code) {
   DCHECK(!Runtime::Current()->IsActiveTransaction());
-  Remove(weak_interns_, s, hash_code);
+  RemoveWeak(s, hash_code);
 }
 
 static mirror::String* LookupStringFromImage(mirror::String* s)
@@ -202,7 +226,7 @@ mirror::String* InternTable::Insert(mirror::String* s, bool is_strong) {
 
   if (is_strong) {
     // Check the strong table for a match.
-    mirror::String* strong = Lookup(strong_interns_, s, hash_code);
+    mirror::String* strong = LookupStrong(s, hash_code);
     if (strong != NULL) {
       return strong;
     }
@@ -214,7 +238,7 @@ mirror::String* InternTable::Insert(mirror::String* s, bool is_strong) {
     }
 
     // There is no match in the strong table, check the weak table.
-    mirror::String* weak = Lookup(weak_interns_, s, hash_code);
+    mirror::String* weak = LookupWeak(s, hash_code);
     if (weak != NULL) {
       // A match was found in the weak table. Promote to the strong table.
       RemoveWeak(weak, hash_code);
@@ -227,7 +251,7 @@ mirror::String* InternTable::Insert(mirror::String* s, bool is_strong) {
   }
 
   // Check the strong table for a match.
-  mirror::String* strong = Lookup(strong_interns_, s, hash_code);
+  mirror::String* strong = LookupStrong(s, hash_code);
   if (strong != NULL) {
     return strong;
   }
@@ -237,7 +261,7 @@ mirror::String* InternTable::Insert(mirror::String* s, bool is_strong) {
     return InsertWeak(image, hash_code);
   }
   // Check the weak table for a match.
-  mirror::String* weak = Lookup(weak_interns_, s, hash_code);
+  mirror::String* weak = LookupWeak(s, hash_code);
   if (weak != NULL) {
     return weak;
   }
@@ -272,13 +296,14 @@ mirror::String* InternTable::InternWeak(mirror::String* s) {
 
 bool InternTable::ContainsWeak(mirror::String* s) {
   MutexLock mu(Thread::Current(), *Locks::intern_table_lock_);
-  const mirror::String* found = Lookup(weak_interns_, s, s->GetHashCode());
+  const mirror::String* found = LookupWeak(s, s->GetHashCode());
   return found == s;
 }
 
 void InternTable::SweepInternTableWeaks(IsMarkedCallback* callback, void* arg) {
   MutexLock mu(Thread::Current(), *Locks::intern_table_lock_);
   for (auto it = weak_interns_.begin(), end = weak_interns_.end(); it != end;) {
+    // This does not need a read barrier because this is called by GC.
     mirror::Object* object = it->second;
     mirror::Object* new_object = callback(object, arg);
     if (new_object == nullptr) {
diff --git a/runtime/intern_table.h b/runtime/intern_table.h
index 47d5e09..3df2aeb 100644
--- a/runtime/intern_table.h
+++ b/runtime/intern_table.h
@@ -79,15 +79,26 @@ class InternTable {
       LOCKS_EXCLUDED(Locks::intern_table_lock_)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
-  mirror::String* Lookup(Table& table, mirror::String* s, int32_t hash_code)
+  mirror::String* LookupStrong(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+  mirror::String* LookupWeak(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
+  template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  mirror::String* Lookup(Table* table, mirror::String* s, int32_t hash_code)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   mirror::String* InsertStrong(mirror::String* s, int32_t hash_code)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
   mirror::String* InsertWeak(mirror::String* s, int32_t hash_code)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
+  void RemoveStrong(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+      EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
   void RemoveWeak(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
-  void Remove(Table& table, mirror::String* s, int32_t hash_code)
+  template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  void Remove(Table* table, mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
 
   // Transaction rollback access.
@@ -96,8 +107,10 @@ class InternTable {
   mirror::String* InsertWeakFromTransaction(mirror::String* s, int32_t hash_code)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
   void RemoveStrongFromTransaction(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
   void RemoveWeakFromTransaction(mirror::String* s, int32_t hash_code)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
   friend class Transaction;
 
@@ -107,6 +120,9 @@ class InternTable {
   Table strong_interns_ GUARDED_BY(Locks::intern_table_lock_);
   std::vector<std::pair<int32_t, mirror::String*>> new_strong_intern_roots_
       GUARDED_BY(Locks::intern_table_lock_);
+  // Since weak_interns_ contain weak roots, they need a read
+  // barrier. Do not directly access the strings in it. Use functions
+  // that contain read barriers.
   Table weak_interns_ GUARDED_BY(Locks::intern_table_lock_);
 };
 
diff --git a/runtime/monitor.h b/runtime/monitor.h
index 9e6d255..bd0e23c 100644
--- a/runtime/monitor.h
+++ b/runtime/monitor.h
@@ -94,8 +94,8 @@ class Monitor {
   static bool IsValidLockWord(LockWord lock_word);
 
   template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
-  mirror::Object* GetObject() const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
-    return ReadBarrier::BarrierForWeakRoot<mirror::Object, kReadBarrierOption>(obj_);
+  mirror::Object* GetObject() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+    return ReadBarrier::BarrierForWeakRoot<mirror::Object, kReadBarrierOption>(&obj_);
   }
 
   void SetObject(mirror::Object* object);
diff --git a/runtime/read_barrier-inl.h b/runtime/read_barrier-inl.h
index 4302c9e..e252b7b 100644
--- a/runtime/read_barrier-inl.h
+++ b/runtime/read_barrier-inl.h
@@ -44,8 +44,8 @@ inline MirrorType* ReadBarrier::Barrier(
 }
 
 template <typename MirrorType, ReadBarrierOption kReadBarrierOption>
-inline MirrorType* ReadBarrier::BarrierForWeakRoot(MirrorType* ref) {
-  UNUSED(ref);
+inline MirrorType* ReadBarrier::BarrierForWeakRoot(MirrorType** weak_root) {
+  MirrorType* ref = *weak_root;
   const bool with_read_barrier = kReadBarrierOption == kWithReadBarrier;
   if (with_read_barrier && kUseBakerReadBarrier) {
     // To be implemented.
diff --git a/runtime/read_barrier.h b/runtime/read_barrier.h
index e40e8ea..7232a3f 100644
--- a/runtime/read_barrier.h
+++ b/runtime/read_barrier.h
@@ -39,7 +39,7 @@ class ReadBarrier {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   template <typename MirrorType, ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
-  ALWAYS_INLINE static MirrorType* BarrierForWeakRoot(MirrorType* ref)
+  ALWAYS_INLINE static MirrorType* BarrierForWeakRoot(MirrorType** weak_root)
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 };
 
diff --git a/runtime/transaction.h b/runtime/transaction.h
index 6fd86c8..7859126 100644
--- a/runtime/transaction.h
+++ b/runtime/transaction.h
@@ -147,7 +147,9 @@ class Transaction {
       DCHECK(s != nullptr);
     }
 
-    void Undo(InternTable* intern_table) EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
+    void Undo(InternTable* intern_table)
+        SHARED_LOCKS_REQUIRED(Locks::mutator_lock_)
+        EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_);
     void VisitRoots(RootCallback* callback, void* arg);
 
    private:
@@ -169,7 +171,8 @@ class Transaction {
       SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
   void UndoInternStringTableModifications()
       EXCLUSIVE_LOCKS_REQUIRED(Locks::intern_table_lock_)
-      EXCLUSIVE_LOCKS_REQUIRED(log_lock_);
+      EXCLUSIVE_LOCKS_REQUIRED(log_lock_)
+      SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
 
   void VisitObjectLogs(RootCallback* callback, void* arg)
       EXCLUSIVE_LOCKS_REQUIRED(log_lock_)