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
diff --git a/runtime/mirror/dex_cache-inl.h b/runtime/mirror/dex_cache-inl.h
index a3071b7..220979a 100644
--- a/runtime/mirror/dex_cache-inl.h
+++ b/runtime/mirror/dex_cache-inl.h
@@ -44,13 +44,29 @@
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 caf00c2..7d4021f 100644
--- a/runtime/mirror/dex_cache.h
+++ b/runtime/mirror/dex_cache.h
@@ -56,12 +56,20 @@
// 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 @@
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 @@
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 a365a73..ba12d33 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1929,6 +1929,12 @@
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 44f765a..dc14c04 100644
--- a/runtime/runtime.h
+++ b/runtime/runtime.h
@@ -55,8 +55,9 @@
} // 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 @@
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 d91860b..9f8d981 100644
--- a/runtime/transaction.cc
+++ b/runtime/transaction.cc
@@ -49,13 +49,15 @@
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 @@
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 @@
UndoObjectModifications();
UndoArrayModifications();
UndoInternStringTableModifications();
+ UndoResolveStringModifications();
}
void Transaction::UndoObjectModifications() {
@@ -230,11 +240,19 @@
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::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 @@
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 bc9c640..584dfb8 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 @@
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 @@
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 @@
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 @@
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 @@
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 8279a26..82e529c 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 @@
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 00e1fbb..e7085c1 100644
--- a/test/Transaction/Transaction.java
+++ b/test/Transaction/Transaction.java
@@ -18,6 +18,10 @@
static class EmptyStatic {
}
+ static class ResolveString {
+ static String s = "ResolvedString";
+ }
+
static class StaticFieldClass {
public static int intField;
static {