Clean up Object size related read barriers.

Test: m test-art-host-gtest
Test: testrunner.py --host --interpreter
Test: testrunner.py --host --interpreter --gcstress
Bug: 119486698
Change-Id: I831838f230ebdd9e540462b2de56271895a01fad

10 files changed, 44 insertions, 37 deletions

diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index 854defe388..30da1ae72d 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -3220,7 +3220,7 @@ void ConcurrentCopying::FillWithDummyObject(Thread* const self,
   if (ReadBarrier::kEnableToSpaceInvariantChecks) {
     AssertToSpaceInvariant(nullptr, MemberOffset(0), int_array_class);
   }
-  size_t component_size = int_array_class->GetComponentSize<kWithoutReadBarrier>();
+  size_t component_size = int_array_class->GetComponentSize();
   CHECK_EQ(component_size, sizeof(int32_t));
   size_t data_offset = mirror::Array::DataOffset(component_size).SizeValue();
   if (data_offset > byte_size) {
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 73b9b69f45..d3e45827a4 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -728,7 +728,7 @@ void JitCodeCache::SweepRootTables(IsMarkedVisitor* visitor) {
       mirror::Object* object = roots[i].Read<kWithoutReadBarrier>();
       if (object == nullptr || object == weak_sentinel) {
         // entry got deleted in a previous sweep.
-      } else if (object->IsString<kDefaultVerifyFlags, kWithoutReadBarrier>()) {
+      } else if (object->IsString<kDefaultVerifyFlags>()) {
         mirror::Object* new_object = visitor->IsMarked(object);
         // We know the string is marked because it's a strongly-interned string that
         // is always alive. The IsMarked implementation of the CMS collector returns
diff --git a/runtime/mirror/array-inl.h b/runtime/mirror/array-inl.h
index 19d35a89ec..34925f52e2 100644
--- a/runtime/mirror/array-inl.h
+++ b/runtime/mirror/array-inl.h
@@ -35,14 +35,16 @@ inline uint32_t Array::ClassSize(PointerSize pointer_size) {
   return Class::ComputeClassSize(true, vtable_entries, 0, 0, 0, 0, 0, pointer_size);
 }
 
-template<VerifyObjectFlags kVerifyFlags, ReadBarrierOption kReadBarrierOption>
+template<VerifyObjectFlags kVerifyFlags>
 inline size_t Array::SizeOf() {
-  // This is safe from overflow because the array was already allocated, so we know it's sane.
-  size_t component_size_shift = GetClass<kVerifyFlags, kReadBarrierOption>()->
-      template GetComponentSizeShift<kReadBarrierOption>();
+  // No read barrier is needed for reading a constant primitive field through
+  // constant reference field chain. See ReadBarrierOption.
+  size_t component_size_shift =
+      GetClass<kVerifyFlags, kWithoutReadBarrier>()->GetComponentSizeShift();
   // Don't need to check this since we already check this in GetClass.
   int32_t component_count =
       GetLength<static_cast<VerifyObjectFlags>(kVerifyFlags & ~kVerifyThis)>();
+  // This is safe from overflow because the array was already allocated, so we know it's sane.
   size_t header_size = DataOffset(1U << component_size_shift).SizeValue();
   size_t data_size = component_count << component_size_shift;
   return header_size + data_size;
diff --git a/runtime/mirror/array.cc b/runtime/mirror/array.cc
index 05e397db95..d42f5a0099 100644
--- a/runtime/mirror/array.cc
+++ b/runtime/mirror/array.cc
@@ -139,7 +139,8 @@ void Array::ThrowArrayStoreException(ObjPtr<Object> object) {
 }
 
 ObjPtr<Array> Array::CopyOf(Thread* self, int32_t new_length) {
-  CHECK(GetClass()->GetComponentType()->IsPrimitive()) << "Will miss write barriers";
+  ObjPtr<Class> klass = GetClass();
+  CHECK(klass->IsPrimitiveArray()) << "Will miss write barriers";
   DCHECK_GE(new_length, 0);
   // We may get copied by a compacting GC.
   StackHandleScope<1> hs(self);
@@ -147,16 +148,16 @@ ObjPtr<Array> Array::CopyOf(Thread* self, int32_t new_length) {
   auto* heap = Runtime::Current()->GetHeap();
   gc::AllocatorType allocator_type = heap->IsMovableObject(this) ? heap->GetCurrentAllocator() :
       heap->GetCurrentNonMovingAllocator();
-  const auto component_size = GetClass()->GetComponentSize();
-  const auto component_shift = GetClass()->GetComponentSizeShift();
+  const auto component_size = klass->GetComponentSize();
+  const auto component_shift = klass->GetComponentSizeShift();
   ObjPtr<Array> new_array =
-      Alloc<true>(self, GetClass(), new_length, component_shift, allocator_type);
+      Alloc<true>(self, klass, new_length, component_shift, allocator_type);  // Invalidates klass.
   if (LIKELY(new_array != nullptr)) {
     memcpy(new_array->GetRawData(component_size, 0),
            h_this->GetRawData(component_size, 0),
            std::min(h_this->GetLength(), new_length) << component_shift);
   }
-  return new_array.Ptr();
+  return new_array;
 }
 
 // Explicitly instantiate all the primitive array types.
diff --git a/runtime/mirror/array.h b/runtime/mirror/array.h
index 593c0a8529..1ee4e50df8 100644
--- a/runtime/mirror/array.h
+++ b/runtime/mirror/array.h
@@ -55,8 +55,7 @@ class MANAGED Array : public Object {
       REQUIRES_SHARED(Locks::mutator_lock_)
       REQUIRES(!Roles::uninterruptible_);
 
-  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
-           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   size_t SizeOf() REQUIRES_SHARED(Locks::mutator_lock_);
   template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   ALWAYS_INLINE int32_t GetLength() REQUIRES_SHARED(Locks::mutator_lock_) {
diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index 2e8ced5e2f..3ee8bfe89b 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -1007,14 +1007,14 @@ inline void Class::SetComponentType(ObjPtr<Class> new_component_type) {
   SetFieldObject<false, false>(ComponentTypeOffset(), new_component_type);
 }
 
-template<ReadBarrierOption kReadBarrierOption>
 inline size_t Class::GetComponentSize() {
-  return 1U << GetComponentSizeShift<kReadBarrierOption>();
+  return 1U << GetComponentSizeShift();
 }
 
-template<ReadBarrierOption kReadBarrierOption>
 inline size_t Class::GetComponentSizeShift() {
-  return GetComponentType<kDefaultVerifyFlags, kReadBarrierOption>()->GetPrimitiveTypeSizeShift();
+  // No read barrier is needed for reading a constant primitive field through
+  // constant reference field. See ReadBarrierOption.
+  return GetComponentType<kDefaultVerifyFlags, kWithoutReadBarrier>()->GetPrimitiveTypeSizeShift();
 }
 
 inline bool Class::IsObjectClass() {
diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 3b4ae0f41e..5025c0295c 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -1003,7 +1003,10 @@ const char* Class::GetDescriptor(std::string* storage) {
   ObjPtr<mirror::Class> klass = this;
   while (klass->IsArrayClass()) {
     ++dim;
-    klass = klass->GetComponentType();
+    // No read barrier needed, we're reading a chain of constant references for comparison
+    // with null. Then we follow up below with reading constant references to read constant
+    // primitive data in both proxy and non-proxy paths. See ReadBarrierOption.
+    klass = klass->GetComponentType<kDefaultVerifyFlags, kWithoutReadBarrier>();
   }
   if (klass->IsProxyClass()) {
     // No read barrier needed, the `name` field is constant for proxy classes and
diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h
index 687b5100b1..ac5d52d4d1 100644
--- a/runtime/mirror/class.h
+++ b/runtime/mirror/class.h
@@ -446,10 +446,8 @@ class MANAGED Class final : public Object {
 
   void SetComponentType(ObjPtr<Class> new_component_type) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
   size_t GetComponentSize() REQUIRES_SHARED(Locks::mutator_lock_);
 
-  template<ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
   size_t GetComponentSizeShift() REQUIRES_SHARED(Locks::mutator_lock_);
 
   bool IsObjectClass() REQUIRES_SHARED(Locks::mutator_lock_);
@@ -478,8 +476,7 @@ class MANAGED Class final : public Object {
   template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   ALWAYS_INLINE bool IsVariableSize() REQUIRES_SHARED(Locks::mutator_lock_);
 
-  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
-           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   uint32_t SizeOf() REQUIRES_SHARED(Locks::mutator_lock_) {
     return GetField32<kVerifyFlags>(OFFSET_OF_OBJECT_MEMBER(Class, class_size_));
   }
diff --git a/runtime/mirror/object-inl.h b/runtime/mirror/object-inl.h
index 56e1807e41..2476b135ff 100644
--- a/runtime/mirror/object-inl.h
+++ b/runtime/mirror/object-inl.h
@@ -300,14 +300,16 @@ inline ObjPtr<DoubleArray> Object::AsDoubleArray() {
   return ObjPtr<DoubleArray>::DownCast(this);
 }
 
-template<VerifyObjectFlags kVerifyFlags, ReadBarrierOption kReadBarrierOption>
+template<VerifyObjectFlags kVerifyFlags>
 inline bool Object::IsString() {
-  return GetClass<kVerifyFlags, kReadBarrierOption>()->IsStringClass();
+  // No read barrier is needed for reading a constant primitive field through
+  // constant reference field. See ReadBarrierOption.
+  return GetClass<kVerifyFlags, kWithoutReadBarrier>()->IsStringClass();
 }
 
-template<VerifyObjectFlags kVerifyFlags, ReadBarrierOption kReadBarrierOption>
+template<VerifyObjectFlags kVerifyFlags>
 inline ObjPtr<String> Object::AsString() {
-  DCHECK((IsString<kVerifyFlags, kReadBarrierOption>()));
+  DCHECK((IsString<kVerifyFlags>()));
   return ObjPtr<String>::DownCast(this);
 }
 
@@ -347,19 +349,24 @@ template<VerifyObjectFlags kVerifyFlags>
 inline size_t Object::SizeOf() {
   // Read barrier is never required for SizeOf since objects sizes are constant. Reading from-space
   // values is OK because of that.
-  static constexpr ReadBarrierOption kRBO = kWithoutReadBarrier;
   size_t result;
   constexpr VerifyObjectFlags kNewFlags = RemoveThisFlags(kVerifyFlags);
   if (IsArrayInstance<kVerifyFlags>()) {
-    result = AsArray<kNewFlags>()->template SizeOf<kNewFlags, kRBO>();
+    result = AsArray<kNewFlags>()->template SizeOf<kNewFlags>();
   } else if (IsClass<kNewFlags>()) {
-    result = AsClass<kNewFlags>()->template SizeOf<kNewFlags, kRBO>();
-  } else if (GetClass<kNewFlags, kRBO>()->IsStringClass()) {
-    result = AsString<kNewFlags, kRBO>()->template SizeOf<kNewFlags>();
+    result = AsClass<kNewFlags>()->template SizeOf<kNewFlags>();
+  } else if (IsString<kNewFlags>()) {
+    result = AsString<kNewFlags>()->template SizeOf<kNewFlags>();
   } else {
-    result = GetClass<kNewFlags, kRBO>()->template GetObjectSize<kNewFlags>();
+    result = GetClass<kNewFlags, kWithoutReadBarrier>()->template GetObjectSize<kNewFlags>();
   }
-  DCHECK_GE(result, sizeof(Object)) << " class=" << Class::PrettyClass(GetClass<kNewFlags, kRBO>());
+  DCHECK_GE(result, sizeof(Object)) << " class="
+      // Note: Class::PrettyClass() is reading constant reference fields to get to constant
+      // primitive fields and safely avoids read barriers, so it is safe to call on a Class
+      // reference read without read barrier from a constant reference field.
+      // See ReadBarrierOption. And, for correctness, we actually have to avoid the read
+      // barrier here if Object::SizeOf() is called on a from-space reference.
+      << GetClass<kNewFlags, kWithoutReadBarrier>()->PrettyClass();
   return result;
 }
 
diff --git a/runtime/mirror/object.h b/runtime/mirror/object.h
index e68183554e..e6e91601d9 100644
--- a/runtime/mirror/object.h
+++ b/runtime/mirror/object.h
@@ -242,12 +242,10 @@ class MANAGED LOCKABLE Object {
   template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   ObjPtr<DoubleArray> AsDoubleArray() REQUIRES_SHARED(Locks::mutator_lock_);
 
-  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
-           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   bool IsString() REQUIRES_SHARED(Locks::mutator_lock_);
 
-  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags,
-           ReadBarrierOption kReadBarrierOption = kWithReadBarrier>
+  template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
   ObjPtr<String> AsString() REQUIRES_SHARED(Locks::mutator_lock_);
 
   template<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>