Add transactions for string resolve

Fixes a bug where resolved strings can be left in the dex cache after
a transaction is rolled back even though the interned string was
removed.

Added test in transaction_test.

Bug: 31239436

Test: test-art-host

Change-Id: I42c67bcefeae8db134cde34c480261f52db4102e

8 files changed, 177 insertions, 14 deletions

diff --git a/runtime/mirror/dex_cache-inl.h b/runtime/mirror/dex_cache-inl.h
index a3071b7f63..220979a152 100644
--- a/runtime/mirror/dex_cache-inl.h
+++ b/runtime/mirror/dex_cache-inl.h
@@ -44,13 +44,29 @@ inline mirror::String* DexCache::GetResolvedString(uint32_t string_idx) {
 
 inline void DexCache::SetResolvedString(uint32_t string_idx, mirror::String* resolved) {
   DCHECK_LT(string_idx % NumStrings(), NumStrings());
-  // TODO default transaction support.
-  StringDexCachePair idx_ptr;
-  idx_ptr.string_index = string_idx;
-  idx_ptr.string_pointer = GcRoot<String>(resolved);
-  GetStrings()[string_idx % NumStrings()].store(idx_ptr, std::memory_order_relaxed);
+  GetStrings()[string_idx % NumStrings()].store(
+      StringDexCachePair(resolved, string_idx),
+      std::memory_order_relaxed);
+  Runtime* const runtime = Runtime::Current();
+  if (UNLIKELY(runtime->IsActiveTransaction())) {
+    DCHECK(runtime->IsAotCompiler());
+    runtime->RecordResolveString(this, string_idx);
+  }
   // TODO: Fine-grained marking, so that we don't need to go through all arrays in full.
-  Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(this);
+  runtime->GetHeap()->WriteBarrierEveryFieldOf(this);
+}
+
+inline void DexCache::ClearString(uint32_t string_idx) {
+  const uint32_t slot_idx = string_idx % NumStrings();
+  DCHECK(Runtime::Current()->IsAotCompiler());
+  StringDexCacheType* slot = &GetStrings()[slot_idx];
+  // This is racy but should only be called from the transactional interpreter.
+  if (slot->load(std::memory_order_relaxed).string_index == string_idx) {
+    StringDexCachePair cleared(
+        nullptr,
+        StringDexCachePair::InvalidStringIndexForSlot(slot_idx));
+    slot->store(cleared, std::memory_order_relaxed);
+  }
 }
 
 inline Class* DexCache::GetResolvedType(uint32_t type_idx) {
diff --git a/runtime/mirror/dex_cache.h b/runtime/mirror/dex_cache.h
index caf00c220f..7d4021fe68 100644
--- a/runtime/mirror/dex_cache.h
+++ b/runtime/mirror/dex_cache.h
@@ -56,12 +56,20 @@ struct PACKED(8) StringDexCachePair {
   // it's always non-null if the string id branch succeeds (except for the 0th string id).
   // Set the initial state for the 0th entry to be {0,1} which is guaranteed to fail
   // the lookup string id == stored id branch.
+  StringDexCachePair(String* string, uint32_t string_idx)
+      : string_pointer(string),
+        string_index(string_idx) {}
+  StringDexCachePair() = default;
+  StringDexCachePair(const StringDexCachePair&) = default;
+  StringDexCachePair& operator=(const StringDexCachePair&) = default;
+
   static void Initialize(StringDexCacheType* strings) {
     mirror::StringDexCachePair first_elem;
     first_elem.string_pointer = GcRoot<String>(nullptr);
-    first_elem.string_index = 1;
+    first_elem.string_index = InvalidStringIndexForSlot(0);
     strings[0].store(first_elem, std::memory_order_relaxed);
   }
+
   static GcRoot<String> LookupString(StringDexCacheType* dex_cache,
                                      uint32_t string_idx,
                                      uint32_t cache_size) {
@@ -71,10 +79,15 @@ struct PACKED(8) StringDexCachePair {
     DCHECK(!index_string.string_pointer.IsNull());
     return index_string.string_pointer;
   }
+
+  static uint32_t InvalidStringIndexForSlot(uint32_t slot) {
+    // Since the cache size is a power of two, 0 will always map to slot 0.
+    // Use 1 for slot 0 and 0 for all other slots.
+    return (slot == 0) ? 1u : 0u;
+  }
 };
 using StringDexCacheType = std::atomic<StringDexCachePair>;
 
-
 // C++ mirror of java.lang.DexCache.
 class MANAGED DexCache FINAL : public Object {
  public:
@@ -164,6 +177,10 @@ class MANAGED DexCache FINAL : public Object {
   void SetResolvedString(uint32_t string_idx, mirror::String* resolved) ALWAYS_INLINE
       REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // Clear a string for a string_idx, used to undo string intern transactions to make sure
+  // the string isn't kept live.
+  void ClearString(uint32_t string_idx) REQUIRES_SHARED(Locks::mutator_lock_);
+
   Class* GetResolvedType(uint32_t type_idx) REQUIRES_SHARED(Locks::mutator_lock_);
 
   void SetResolvedType(uint32_t type_idx, Class* resolved) REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index a365a737d8..ba12d33ca2 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1929,6 +1929,12 @@ void Runtime::RecordWeakStringRemoval(mirror::String* s) const {
   preinitialization_transaction_->RecordWeakStringRemoval(s);
 }
 
+void Runtime::RecordResolveString(mirror::DexCache* dex_cache, uint32_t string_idx) const {
+  DCHECK(IsAotCompiler());
+  DCHECK(IsActiveTransaction());
+  preinitialization_transaction_->RecordResolveString(dex_cache, string_idx);
+}
+
 void Runtime::SetFaultMessage(const std::string& message) {
   MutexLock mu(Thread::Current(), fault_message_lock_);
   fault_message_ = message;
diff --git a/runtime/runtime.h b/runtime/runtime.h
index 44f765ac37..dc14c04719 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -55,8 +55,9 @@ namespace jit {
 }  // namespace jit
 
 namespace mirror {
-  class ClassLoader;
   class Array;
+  class ClassLoader;
+  class DexCache;
   template<class T> class ObjectArray;
   template<class T> class PrimitiveArray;
   typedef PrimitiveArray<int8_t> ByteArray;
@@ -508,6 +509,8 @@ class Runtime {
       REQUIRES(Locks::intern_table_lock_);
   void RecordWeakStringRemoval(mirror::String* s) const
       REQUIRES(Locks::intern_table_lock_);
+  void RecordResolveString(mirror::DexCache* dex_cache, uint32_t string_idx) const
+      REQUIRES_SHARED(Locks::mutator_lock_);
 
   void SetFaultMessage(const std::string& message) REQUIRES(!fault_message_lock_);
   // Only read by the signal handler, NO_THREAD_SAFETY_ANALYSIS to prevent lock order violations
diff --git a/runtime/transaction.cc b/runtime/transaction.cc
index d91860bd83..9f8d9816f2 100644
--- a/runtime/transaction.cc
+++ b/runtime/transaction.cc
@@ -49,13 +49,15 @@ Transaction::~Transaction() {
     for (auto it : array_logs_) {
       array_values_count += it.second.Size();
     }
-    size_t string_count = intern_string_logs_.size();
+    size_t intern_string_count = intern_string_logs_.size();
+    size_t resolve_string_count = resolve_string_logs_.size();
     LOG(INFO) << "Transaction::~Transaction"
               << ": objects_count=" << objects_count
               << ", field_values_count=" << field_values_count
               << ", array_count=" << array_count
               << ", array_values_count=" << array_values_count
-              << ", string_count=" << string_count;
+              << ", intern_string_count=" << intern_string_count
+              << ", resolve_string_count=" << resolve_string_count;
   }
 }
 
@@ -165,6 +167,13 @@ void Transaction::RecordWriteArray(mirror::Array* array, size_t index, uint64_t
   array_log.LogValue(index, value);
 }
 
+void Transaction::RecordResolveString(mirror::DexCache* dex_cache, uint32_t string_idx) {
+  DCHECK(dex_cache != nullptr);
+  DCHECK_LT(string_idx, dex_cache->GetDexFile()->NumStringIds());
+  MutexLock mu(Thread::Current(), log_lock_);
+  resolve_string_logs_.push_back(ResolveStringLog(dex_cache, string_idx));
+}
+
 void Transaction::RecordStrongStringInsertion(mirror::String* s) {
   InternStringLog log(s, InternStringLog::kStrongString, InternStringLog::kInsert);
   LogInternedString(log);
@@ -200,6 +209,7 @@ void Transaction::Rollback() {
   UndoObjectModifications();
   UndoArrayModifications();
   UndoInternStringTableModifications();
+  UndoResolveStringModifications();
 }
 
 void Transaction::UndoObjectModifications() {
@@ -230,11 +240,19 @@ void Transaction::UndoInternStringTableModifications() {
   intern_string_logs_.clear();
 }
 
+void Transaction::UndoResolveStringModifications() {
+  for (ResolveStringLog& string_log : resolve_string_logs_) {
+    string_log.Undo();
+  }
+  resolve_string_logs_.clear();
+}
+
 void Transaction::VisitRoots(RootVisitor* visitor) {
   MutexLock mu(Thread::Current(), log_lock_);
   VisitObjectLogs(visitor);
   VisitArrayLogs(visitor);
-  VisitStringLogs(visitor);
+  VisitInternStringLogs(visitor);
+  VisitResolveStringLogs(visitor);
 }
 
 void Transaction::VisitObjectLogs(RootVisitor* visitor) {
@@ -292,12 +310,18 @@ void Transaction::VisitArrayLogs(RootVisitor* visitor) {
   }
 }
 
-void Transaction::VisitStringLogs(RootVisitor* visitor) {
+void Transaction::VisitInternStringLogs(RootVisitor* visitor) {
   for (InternStringLog& log : intern_string_logs_) {
     log.VisitRoots(visitor);
   }
 }
 
+void Transaction::VisitResolveStringLogs(RootVisitor* visitor) {
+  for (ResolveStringLog& log : resolve_string_logs_) {
+    log.VisitRoots(visitor);
+  }
+}
+
 void Transaction::ObjectLog::LogBooleanValue(MemberOffset offset, uint8_t value, bool is_volatile) {
   LogValue(ObjectLog::kBoolean, offset, value, is_volatile);
 }
@@ -481,6 +505,21 @@ void Transaction::InternStringLog::VisitRoots(RootVisitor* visitor) {
   visitor->VisitRoot(reinterpret_cast<mirror::Object**>(&str_), RootInfo(kRootInternedString));
 }
 
+void Transaction::ResolveStringLog::Undo() {
+  dex_cache_.Read()->ClearString(string_idx_);
+}
+
+Transaction::ResolveStringLog::ResolveStringLog(mirror::DexCache* dex_cache, uint32_t string_idx)
+    : dex_cache_(dex_cache),
+      string_idx_(string_idx) {
+  DCHECK(dex_cache != nullptr);
+  DCHECK_LT(string_idx_, dex_cache->GetDexFile()->NumStringIds());
+}
+
+void Transaction::ResolveStringLog::VisitRoots(RootVisitor* visitor) {
+  dex_cache_.VisitRoot(visitor, RootInfo(kRootVMInternal));
+}
+
 void Transaction::ArrayLog::LogValue(size_t index, uint64_t value) {
   auto it = array_values_.find(index);
   if (it == array_values_.end()) {
diff --git a/runtime/transaction.h b/runtime/transaction.h
index bc9c640303..584dfb8986 100644
--- a/runtime/transaction.h
+++ b/runtime/transaction.h
@@ -32,6 +32,7 @@
 namespace art {
 namespace mirror {
 class Array;
+class DexCache;
 class Object;
 class String;
 }
@@ -95,6 +96,11 @@ class Transaction FINAL {
       REQUIRES(Locks::intern_table_lock_)
       REQUIRES(!log_lock_);
 
+  // Record resolve string.
+  void RecordResolveString(mirror::DexCache* dex_cache, uint32_t string_idx)
+      REQUIRES_SHARED(Locks::mutator_lock_)
+      REQUIRES(!log_lock_);
+
   // Abort transaction by undoing all recorded changes.
   void Rollback()
       REQUIRES_SHARED(Locks::mutator_lock_)
@@ -192,6 +198,19 @@ class Transaction FINAL {
     const StringOp string_op_;
   };
 
+  class ResolveStringLog : public ValueObject {
+   public:
+    ResolveStringLog(mirror::DexCache* dex_cache, uint32_t string_idx);
+
+    void Undo() REQUIRES_SHARED(Locks::mutator_lock_);
+
+    void VisitRoots(RootVisitor* visitor) REQUIRES_SHARED(Locks::mutator_lock_);
+
+   private:
+    GcRoot<mirror::DexCache> dex_cache_;
+    const uint32_t string_idx_;
+  };
+
   void LogInternedString(const InternStringLog& log)
       REQUIRES(Locks::intern_table_lock_)
       REQUIRES(!log_lock_);
@@ -206,6 +225,9 @@ class Transaction FINAL {
       REQUIRES(Locks::intern_table_lock_)
       REQUIRES(log_lock_)
       REQUIRES_SHARED(Locks::mutator_lock_);
+  void UndoResolveStringModifications()
+      REQUIRES(log_lock_)
+      REQUIRES_SHARED(Locks::mutator_lock_);
 
   void VisitObjectLogs(RootVisitor* visitor)
       REQUIRES(log_lock_)
@@ -213,7 +235,10 @@ class Transaction FINAL {
   void VisitArrayLogs(RootVisitor* visitor)
       REQUIRES(log_lock_)
       REQUIRES_SHARED(Locks::mutator_lock_);
-  void VisitStringLogs(RootVisitor* visitor)
+  void VisitInternStringLogs(RootVisitor* visitor)
+      REQUIRES(log_lock_)
+      REQUIRES_SHARED(Locks::mutator_lock_);
+  void VisitResolveStringLogs(RootVisitor* visitor)
       REQUIRES(log_lock_)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
@@ -223,6 +248,7 @@ class Transaction FINAL {
   std::map<mirror::Object*, ObjectLog> object_logs_ GUARDED_BY(log_lock_);
   std::map<mirror::Array*, ArrayLog> array_logs_  GUARDED_BY(log_lock_);
   std::list<InternStringLog> intern_string_logs_ GUARDED_BY(log_lock_);
+  std::list<ResolveStringLog> resolve_string_logs_ GUARDED_BY(log_lock_);
   bool aborted_ GUARDED_BY(log_lock_);
   std::string abort_message_ GUARDED_BY(log_lock_);
 
diff --git a/runtime/transaction_test.cc b/runtime/transaction_test.cc
index 8279a26f52..82e529c5ab 100644
--- a/runtime/transaction_test.cc
+++ b/runtime/transaction_test.cc
@@ -20,11 +20,14 @@
 #include "art_method-inl.h"
 #include "class_linker-inl.h"
 #include "common_runtime_test.h"
+#include "dex_file.h"
 #include "mirror/array-inl.h"
 #include "scoped_thread_state_change.h"
 
 namespace art {
 
+static const size_t kDexNoIndex = DexFile::kDexNoIndex;  // Make copy to prevent linking errors.
+
 class TransactionTest : public CommonRuntimeTest {
  public:
   // Tests failing class initialization due to native call with transaction rollback.
@@ -482,6 +485,55 @@ TEST_F(TransactionTest, StaticArrayFieldsTest) {
   EXPECT_EQ(objectArray->GetWithoutChecks(0), nullptr);
 }
 
+// Tests rolling back interned strings and resolved strings.
+TEST_F(TransactionTest, ResolveString) {
+  ScopedObjectAccess soa(Thread::Current());
+  StackHandleScope<3> hs(soa.Self());
+  Handle<mirror::ClassLoader> class_loader(
+      hs.NewHandle(soa.Decode<mirror::ClassLoader*>(LoadDex("Transaction"))));
+  ASSERT_TRUE(class_loader.Get() != nullptr);
+
+  Handle<mirror::Class> h_klass(
+      hs.NewHandle(class_linker_->FindClass(soa.Self(), "LTransaction$ResolveString;",
+                                            class_loader)));
+  ASSERT_TRUE(h_klass.Get() != nullptr);
+
+  Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(h_klass->GetDexCache()));
+  ASSERT_TRUE(h_dex_cache.Get() != nullptr);
+  const DexFile* const dex_file = h_dex_cache->GetDexFile();
+  ASSERT_TRUE(dex_file != nullptr);
+
+  // Go search the dex file to find the string id of our string.
+  static const char* kResolvedString = "ResolvedString";
+  const DexFile::StringId* string_id = dex_file->FindStringId(kResolvedString);
+  ASSERT_TRUE(string_id != nullptr);
+  uint32_t string_idx = dex_file->GetIndexForStringId(*string_id);
+  ASSERT_NE(string_idx, kDexNoIndex);
+  // String should only get resolved by the initializer.
+  EXPECT_TRUE(class_linker_->LookupString(*dex_file, string_idx, h_dex_cache) == nullptr);
+  EXPECT_TRUE(h_dex_cache->GetResolvedString(string_idx) == nullptr);
+  // Do the transaction, then roll back.
+  Transaction transaction;
+  Runtime::Current()->EnterTransactionMode(&transaction);
+  bool success = class_linker_->EnsureInitialized(soa.Self(), h_klass, true, true);
+  ASSERT_TRUE(success);
+  ASSERT_TRUE(h_klass->IsInitialized());
+  // Make sure the string got resolved by the transaction.
+  {
+    mirror::String* s = class_linker_->LookupString(*dex_file, string_idx, h_dex_cache);
+    ASSERT_TRUE(s != nullptr);
+    EXPECT_STREQ(s->ToModifiedUtf8().c_str(), kResolvedString);
+    EXPECT_EQ(s, h_dex_cache->GetResolvedString(string_idx));
+  }
+  Runtime::Current()->ExitTransactionMode();
+  transaction.Rollback();
+  // Check that the string did not stay resolved.
+  EXPECT_TRUE(class_linker_->LookupString(*dex_file, string_idx, h_dex_cache) == nullptr);
+  EXPECT_TRUE(h_dex_cache->GetResolvedString(string_idx) == nullptr);
+  ASSERT_FALSE(h_klass->IsInitialized());
+  ASSERT_FALSE(soa.Self()->IsExceptionPending());
+}
+
 // Tests successful class initialization without class initializer.
 TEST_F(TransactionTest, EmptyClass) {
   ScopedObjectAccess soa(Thread::Current());
diff --git a/test/Transaction/Transaction.java b/test/Transaction/Transaction.java
index 00e1fbb2c1..e7085c1734 100644
--- a/test/Transaction/Transaction.java
+++ b/test/Transaction/Transaction.java
@@ -18,6 +18,10 @@ public class Transaction {
     static class EmptyStatic {
     }
 
+    static class ResolveString {
+      static String s = "ResolvedString";
+    }
+
     static class StaticFieldClass {
       public static int intField;
       static {