Fix transaction records for `fill-array-data`.

We need to record old array elements, not the new ones, so
call `RecordArrayElementsInTransaction()` earlier. Move the
implementation where it's actually needed, check for NPE
(due to the reordering) and avoid unnecessary read barriers.

Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing --interp-ac
Change-Id: I6282afed941b1a32468aceb970c3e72283b445ba

5 files changed, 65 insertions, 55 deletions

diff --git a/runtime/interpreter/active_transaction_checker.h b/runtime/interpreter/active_transaction_checker.h
index 63dcc99cdb..f299bbc290 100644
--- a/runtime/interpreter/active_transaction_checker.h
+++ b/runtime/interpreter/active_transaction_checker.h
@@ -87,6 +87,9 @@ class ActiveTransactionChecker {
     return GetClassLinker()->IsTransactionAborted();
   }
 
+  static void RecordArrayElementsInTransaction(ObjPtr<mirror::Object> array, int32_t count)
+      REQUIRES_SHARED(Locks::mutator_lock_);
+
  private:
   static AotClassLinker* GetClassLinker() {
     return down_cast<AotClassLinker*>(Runtime::Current()->GetClassLinker());
diff --git a/runtime/interpreter/interpreter_common.cc b/runtime/interpreter/interpreter_common.cc
index 24f614e94f..280136d6a2 100644
--- a/runtime/interpreter/interpreter_common.cc
+++ b/runtime/interpreter/interpreter_common.cc
@@ -1508,55 +1508,6 @@ bool DoFilledNewArray(const Instruction* inst,
   return true;
 }
 
-// TODO: Use ObjPtr here.
-template<typename T>
-static void RecordArrayElementsInTransactionImpl(ObjPtr<mirror::PrimitiveArray<T>> array,
-                                                 int32_t count)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  Runtime* runtime = Runtime::Current();
-  for (int32_t i = 0; i < count; ++i) {
-    runtime->GetClassLinker()->RecordWriteArray(array.Ptr(), i, array->GetWithoutChecks(i));
-  }
-}
-
-void RecordArrayElementsInTransaction(ObjPtr<mirror::Array> array, int32_t count)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  DCHECK(Runtime::Current()->IsActiveTransaction());
-  DCHECK(array != nullptr);
-  DCHECK_LE(count, array->GetLength());
-  Primitive::Type primitive_component_type = array->GetClass()->GetComponentType()->GetPrimitiveType();
-  switch (primitive_component_type) {
-    case Primitive::kPrimBoolean:
-      RecordArrayElementsInTransactionImpl(array->AsBooleanArray(), count);
-      break;
-    case Primitive::kPrimByte:
-      RecordArrayElementsInTransactionImpl(array->AsByteArray(), count);
-      break;
-    case Primitive::kPrimChar:
-      RecordArrayElementsInTransactionImpl(array->AsCharArray(), count);
-      break;
-    case Primitive::kPrimShort:
-      RecordArrayElementsInTransactionImpl(array->AsShortArray(), count);
-      break;
-    case Primitive::kPrimInt:
-      RecordArrayElementsInTransactionImpl(array->AsIntArray(), count);
-      break;
-    case Primitive::kPrimFloat:
-      RecordArrayElementsInTransactionImpl(array->AsFloatArray(), count);
-      break;
-    case Primitive::kPrimLong:
-      RecordArrayElementsInTransactionImpl(array->AsLongArray(), count);
-      break;
-    case Primitive::kPrimDouble:
-      RecordArrayElementsInTransactionImpl(array->AsDoubleArray(), count);
-      break;
-    default:
-      LOG(FATAL) << "Unsupported primitive type " << primitive_component_type
-                 << " in fill-array-data";
-      UNREACHABLE();
-  }
-}
-
 void UnlockHeldMonitors(Thread* self, ShadowFrame* shadow_frame)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   DCHECK(shadow_frame->GetForcePopFrame() ||
diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h
index d9e0c2f955..912beb352b 100644
--- a/runtime/interpreter/interpreter_common.h
+++ b/runtime/interpreter/interpreter_common.h
@@ -100,6 +100,10 @@ class InactiveTransactionChecker {
   ALWAYS_INLINE static bool IsTransactionAborted() {
     return false;
   }
+
+  static void RecordArrayElementsInTransaction([[maybe_unused]] ObjPtr<mirror::Object> array,
+                                               [[maybe_unused]] int32_t count)
+      REQUIRES_SHARED(Locks::mutator_lock_) {}
 };
 
 void ThrowNullPointerExceptionFromInterpreter()
@@ -146,9 +150,6 @@ static inline bool DoMonitorCheckOnExit(Thread* self, ShadowFrame* frame)
   return true;
 }
 
-void RecordArrayElementsInTransaction(ObjPtr<mirror::Array> array, int32_t count)
-    REQUIRES_SHARED(Locks::mutator_lock_);
-
 // Invokes the given method. This is part of the invocation support and is used by DoInvoke,
 // DoFastInvoke and DoInvokeVirtualQuick functions.
 // Returns true on success, otherwise throws an exception and returns false.
diff --git a/runtime/interpreter/interpreter_switch_impl-inl.h b/runtime/interpreter/interpreter_switch_impl-inl.h
index ec152a5d07..ce0117f724 100644
--- a/runtime/interpreter/interpreter_switch_impl-inl.h
+++ b/runtime/interpreter/interpreter_switch_impl-inl.h
@@ -721,12 +721,11 @@ class InstructionHandler {
     const Instruction::ArrayDataPayload* payload =
         reinterpret_cast<const Instruction::ArrayDataPayload*>(payload_addr);
     ObjPtr<mirror::Object> obj = GetVRegReference(A());
+    // If we have an active transaction, record old values before we overwrite them.
+    TransactionChecker::RecordArrayElementsInTransaction(obj, payload->element_count);
     if (!FillArrayData(obj, payload)) {
       return false;  // Pending exception.
     }
-    if (transaction_active) {
-      RecordArrayElementsInTransaction(obj->AsArray(), payload->element_count);
-    }
     return true;
   }
 
diff --git a/runtime/interpreter/interpreter_switch_impl1.cc b/runtime/interpreter/interpreter_switch_impl1.cc
index e56c58c2bc..8c8db386b3 100644
--- a/runtime/interpreter/interpreter_switch_impl1.cc
+++ b/runtime/interpreter/interpreter_switch_impl1.cc
@@ -24,6 +24,62 @@
 namespace art HIDDEN {
 namespace interpreter {
 
+// TODO: Use ObjPtr here.
+template<typename T>
+static void RecordArrayElementsInTransactionImpl(ObjPtr<mirror::PrimitiveArray<T>> array,
+                                                 int32_t count)
+    REQUIRES_SHARED(Locks::mutator_lock_) {
+  Runtime* runtime = Runtime::Current();
+  for (int32_t i = 0; i < count; ++i) {
+    runtime->GetClassLinker()->RecordWriteArray(array.Ptr(), i, array->GetWithoutChecks(i));
+  }
+}
+
+void ActiveTransactionChecker::RecordArrayElementsInTransaction(ObjPtr<mirror::Object> array,
+                                                                int32_t count) {
+  DCHECK(Runtime::Current()->IsActiveTransaction());
+  if (array == nullptr) {
+    return;  // The interpreter shall throw NPE.
+  }
+  DCHECK(array->IsArrayInstance());
+  DCHECK_LE(count, array->AsArray()->GetLength());
+  // No read barrier is needed for reading a chain of constant references
+  // for reading a constant primitive value, see `ReadBarrierOption`.
+  Primitive::Type primitive_component_type =
+      array->GetClass<kDefaultVerifyFlags, kWithoutReadBarrier>()
+          ->GetComponentType<kDefaultVerifyFlags, kWithoutReadBarrier>()->GetPrimitiveType();
+  switch (primitive_component_type) {
+    case Primitive::kPrimBoolean:
+      RecordArrayElementsInTransactionImpl(array->AsBooleanArray(), count);
+      break;
+    case Primitive::kPrimByte:
+      RecordArrayElementsInTransactionImpl(array->AsByteArray(), count);
+      break;
+    case Primitive::kPrimChar:
+      RecordArrayElementsInTransactionImpl(array->AsCharArray(), count);
+      break;
+    case Primitive::kPrimShort:
+      RecordArrayElementsInTransactionImpl(array->AsShortArray(), count);
+      break;
+    case Primitive::kPrimInt:
+      RecordArrayElementsInTransactionImpl(array->AsIntArray(), count);
+      break;
+    case Primitive::kPrimFloat:
+      RecordArrayElementsInTransactionImpl(array->AsFloatArray(), count);
+      break;
+    case Primitive::kPrimLong:
+      RecordArrayElementsInTransactionImpl(array->AsLongArray(), count);
+      break;
+    case Primitive::kPrimDouble:
+      RecordArrayElementsInTransactionImpl(array->AsDoubleArray(), count);
+      break;
+    default:
+      LOG(FATAL) << "Unsupported primitive type " << primitive_component_type
+                 << " in fill-array-data";
+      UNREACHABLE();
+  }
+}
+
 // Explicit definition of ExecuteSwitchImplCpp.
 template
 void ExecuteSwitchImplCpp<true>(SwitchImplContext* ctx);