Fix transaction bookkeeping for field CAS operations.HEADmastermain

Record the old value only if the CAS is successful to avoid
"restoring" an old value that wasn't there during rollback.

Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Change-Id: I6459b680c216c0b9f7ac67f7b53f79ad37b39b80

3 files changed, 76 insertions, 23 deletions

diff --git a/runtime/mirror/object-inl.h b/runtime/mirror/object-inl.h
index 60b8a0512e..09f1c39058 100644
--- a/runtime/mirror/object-inl.h
+++ b/runtime/mirror/object-inl.h
@@ -570,13 +570,14 @@ inline bool Object::CasFieldWeakSequentiallyConsistent64(MemberOffset field_offs
                                                          int64_t old_value,
                                                          int64_t new_value) {
   VerifyTransaction<kTransactionActive, kCheckTransaction>();
-  if (kTransactionActive) {
-    Runtime::Current()->RecordWriteField64(this, field_offset, old_value, true);
-  }
   Verify<kVerifyFlags>();
   uint8_t* raw_addr = reinterpret_cast<uint8_t*>(this) + field_offset.Int32Value();
   Atomic<int64_t>* atomic_addr = reinterpret_cast<Atomic<int64_t>*>(raw_addr);
-  return atomic_addr->CompareAndSetWeakSequentiallyConsistent(old_value, new_value);
+  bool success = atomic_addr->CompareAndSetWeakSequentiallyConsistent(old_value, new_value);
+  if (kTransactionActive && success) {
+    Runtime::Current()->RecordWriteField64(this, field_offset, old_value, /*is_volatile=*/ true);
+  }
+  return success;
 }
 
 template<bool kTransactionActive, bool kCheckTransaction, VerifyObjectFlags kVerifyFlags>
@@ -584,13 +585,14 @@ inline bool Object::CasFieldStrongSequentiallyConsistent64(MemberOffset field_of
                                                            int64_t old_value,
                                                            int64_t new_value) {
   VerifyTransaction<kTransactionActive, kCheckTransaction>();
-  if (kTransactionActive) {
-    Runtime::Current()->RecordWriteField64(this, field_offset, old_value, true);
-  }
   Verify<kVerifyFlags>();
   uint8_t* raw_addr = reinterpret_cast<uint8_t*>(this) + field_offset.Int32Value();
   Atomic<int64_t>* atomic_addr = reinterpret_cast<Atomic<int64_t>*>(raw_addr);
-  return atomic_addr->CompareAndSetStrongSequentiallyConsistent(old_value, new_value);
+  bool success = atomic_addr->CompareAndSetStrongSequentiallyConsistent(old_value, new_value);
+  if (kTransactionActive && success) {
+    Runtime::Current()->RecordWriteField64(this, field_offset, old_value, /*is_volatile=*/ true);
+  }
+  return success;
 }
 
 /*
@@ -626,13 +628,9 @@ inline void Object::SetFieldObjectWithoutWriteBarrier(MemberOffset field_offset,
                                                       ObjPtr<Object> new_value) {
   VerifyTransaction<kTransactionActive, kCheckTransaction>();
   if (kTransactionActive) {
-    ObjPtr<Object> obj;
-    if (kIsVolatile) {
-      obj = GetFieldObjectVolatile<Object>(field_offset);
-    } else {
-      obj = GetFieldObject<Object>(field_offset);
-    }
-    Runtime::Current()->RecordWriteFieldReference(this, field_offset, obj, true);
+    ObjPtr<Object> old_value =
+        GetFieldObject<Object, kVerifyFlags, kWithReadBarrier, kIsVolatile>(field_offset);
+    Runtime::Current()->RecordWriteFieldReference(this, field_offset, old_value, kIsVolatile);
   }
   Verify<kVerifyFlags>();
   VerifyWrite<kVerifyFlags>(new_value);
@@ -685,14 +683,16 @@ inline bool Object::CasFieldObjectWithoutWriteBarrier(MemberOffset field_offset,
                                                       std::memory_order memory_order) {
   VerifyTransaction<kTransactionActive, kCheckTransaction>();
   VerifyCAS<kVerifyFlags>(new_value, old_value);
-  if (kTransactionActive) {
-    Runtime::Current()->RecordWriteFieldReference(this, field_offset, old_value, true);
-  }
   uint32_t old_ref(PtrCompression<kPoisonHeapReferences, Object>::Compress(old_value));
   uint32_t new_ref(PtrCompression<kPoisonHeapReferences, Object>::Compress(new_value));
   uint8_t* raw_addr = reinterpret_cast<uint8_t*>(this) + field_offset.Int32Value();
   Atomic<uint32_t>* atomic_addr = reinterpret_cast<Atomic<uint32_t>*>(raw_addr);
-  return atomic_addr->CompareAndSet(old_ref, new_ref, mode, memory_order);
+  bool success = atomic_addr->CompareAndSet(old_ref, new_ref, mode, memory_order);
+  if (kTransactionActive && success) {
+    Runtime::Current()->RecordWriteFieldReference(
+        this, field_offset, old_value, /*is_volatile=*/ true);
+  }
+  return success;
 }
 
 template<bool kTransactionActive, bool kCheckTransaction, VerifyObjectFlags kVerifyFlags>
diff --git a/runtime/mirror/object-readbarrier-inl.h b/runtime/mirror/object-readbarrier-inl.h
index 15179b93ff..6c201f2277 100644
--- a/runtime/mirror/object-readbarrier-inl.h
+++ b/runtime/mirror/object-readbarrier-inl.h
@@ -46,16 +46,17 @@ inline bool Object::CasField32(MemberOffset field_offset,
   if (kCheckTransaction) {
     DCHECK_EQ(kTransactionActive, Runtime::Current()->IsActiveTransaction());
   }
-  if (kTransactionActive) {
-    Runtime::Current()->RecordWriteField32(this, field_offset, old_value, true);
-  }
   if (kVerifyFlags & kVerifyThis) {
     VerifyObject(this);
   }
   uint8_t* raw_addr = reinterpret_cast<uint8_t*>(this) + field_offset.Int32Value();
   AtomicInteger* atomic_addr = reinterpret_cast<AtomicInteger*>(raw_addr);
 
-  return atomic_addr->CompareAndSet(old_value, new_value, mode, memory_order);
+  bool success = atomic_addr->CompareAndSet(old_value, new_value, mode, memory_order);
+  if (kTransactionActive && success) {
+    Runtime::Current()->RecordWriteField32(this, field_offset, old_value, /*is_volatile=*/ true);
+  }
+  return success;
 }
 
 inline bool Object::CasLockWord(LockWord old_val,
diff --git a/runtime/transaction_test.cc b/runtime/transaction_test.cc
index db9d123521..e52cef46e0 100644
--- a/runtime/transaction_test.cc
+++ b/runtime/transaction_test.cc
@@ -357,6 +357,58 @@ TEST_F(TransactionTest, InstanceFieldsTest) {
   EXPECT_FLOAT_EQ(floatField->GetFloat(h_instance.Get()), static_cast<float>(0.0f));
   EXPECT_DOUBLE_EQ(doubleField->GetDouble(h_instance.Get()), static_cast<double>(0.0));
   EXPECT_EQ(objectField->GetObject(h_instance.Get()), nullptr);
+
+  // Fail to modify fields with strong CAS inside transaction, then rollback changes.
+  EnterTransactionMode();
+  bool cas_success = h_instance->CasField32</*kTransactionActive=*/ true>(
+      intField->GetOffset(),
+      /*old_value=*/ 1,
+      /*new_value=*/ 2,
+      CASMode::kStrong,
+      std::memory_order_seq_cst);
+  EXPECT_FALSE(cas_success);
+  cas_success = h_instance->CasFieldStrongSequentiallyConsistent64</*kTransactionActive=*/ true>(
+      longField->GetOffset(), /*old_value=*/ INT64_C(1), /*new_value=*/ INT64_C(2));
+  EXPECT_FALSE(cas_success);
+  cas_success = h_instance->CasFieldObject</*kTransactionActive=*/ true>(
+      objectField->GetOffset(),
+      /*old_value=*/ h_instance.Get(),
+      /*new_value=*/ nullptr,
+      CASMode::kStrong,
+      std::memory_order_seq_cst);
+  EXPECT_FALSE(cas_success);
+  RollbackAndExitTransactionMode();
+
+  // Check values have properly been restored to their original (default) value.
+  EXPECT_EQ(intField->GetInt(h_instance.Get()), 0);
+  EXPECT_EQ(longField->GetLong(h_instance.Get()), static_cast<int64_t>(0));
+  EXPECT_EQ(objectField->GetObject(h_instance.Get()), nullptr);
+
+  // Fail to modify fields with weak CAS inside transaction, then rollback changes.
+  EnterTransactionMode();
+  cas_success = h_instance->CasField32</*kTransactionActive=*/ true>(
+      intField->GetOffset(),
+      /*old_value=*/ 3,
+      /*new_value=*/ 4,
+      CASMode::kWeak,
+      std::memory_order_seq_cst);
+  EXPECT_FALSE(cas_success);
+  cas_success = h_instance->CasFieldWeakSequentiallyConsistent64</*kTransactionActive=*/ true>(
+      longField->GetOffset(), /*old_value=*/ INT64_C(3), /*new_value=*/ INT64_C(4));
+  EXPECT_FALSE(cas_success);
+  cas_success = h_instance->CasFieldObject</*kTransactionActive=*/ true>(
+      objectField->GetOffset(),
+      /*old_value=*/ h_klass.Get(),
+      /*new_value=*/ nullptr,
+      CASMode::kWeak,
+      std::memory_order_seq_cst);
+  EXPECT_FALSE(cas_success);
+  RollbackAndExitTransactionMode();
+
+  // Check values have properly been restored to their original (default) value.
+  EXPECT_EQ(intField->GetInt(h_instance.Get()), 0);
+  EXPECT_EQ(longField->GetLong(h_instance.Get()), static_cast<int64_t>(0));
+  EXPECT_EQ(objectField->GetObject(h_instance.Get()), nullptr);
 }
 
 // Tests static array fields are reset to their default value after transaction rollback.