Fix ARM & ARM64 UnsafeGetObject intrinsics with read barriers.
The implementation was incorrectly interpreting the 'offset'
input as an index in a (4-byte) object reference array,
whereas it is a (1-byte) offset to an object reference field
within the 'base' (object) input.
Bug: 29516905
Change-Id: I4da5be0193217965f25e5d141c242592dea6ffe8
Test: Covered by test/004-UnsafeTest.
diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc
index eca9e2c..2b63f3d 100644
--- a/compiler/optimizing/code_generator_arm.cc
+++ b/compiler/optimizing/code_generator_arm.cc
@@ -6085,8 +6085,9 @@
// /* HeapReference<Object> */ ref = *(obj + offset)
Location no_index = Location::NoLocation();
+ ScaleFactor no_scale_factor = TIMES_1;
GenerateReferenceLoadWithBakerReadBarrier(
- instruction, ref, obj, offset, no_index, temp, needs_null_check);
+ instruction, ref, obj, offset, no_index, no_scale_factor, temp, needs_null_check);
}
void CodeGeneratorARM::GenerateArrayLoadWithBakerReadBarrier(HInstruction* instruction,
@@ -6099,10 +6100,14 @@
DCHECK(kEmitCompilerReadBarrier);
DCHECK(kUseBakerReadBarrier);
+ static_assert(
+ sizeof(mirror::HeapReference<mirror::Object>) == sizeof(int32_t),
+ "art::mirror::HeapReference<art::mirror::Object> and int32_t have different sizes.");
// /* HeapReference<Object> */ ref =
// *(obj + data_offset + index * sizeof(HeapReference<Object>))
+ ScaleFactor scale_factor = TIMES_4;
GenerateReferenceLoadWithBakerReadBarrier(
- instruction, ref, obj, data_offset, index, temp, needs_null_check);
+ instruction, ref, obj, data_offset, index, scale_factor, temp, needs_null_check);
}
void CodeGeneratorARM::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
@@ -6110,6 +6115,7 @@
Register obj,
uint32_t offset,
Location index,
+ ScaleFactor scale_factor,
Location temp,
bool needs_null_check) {
DCHECK(kEmitCompilerReadBarrier);
@@ -6164,17 +6170,22 @@
// The actual reference load.
if (index.IsValid()) {
- static_assert(
- sizeof(mirror::HeapReference<mirror::Object>) == sizeof(int32_t),
- "art::mirror::HeapReference<art::mirror::Object> and int32_t have different sizes.");
- // /* HeapReference<Object> */ ref =
- // *(obj + offset + index * sizeof(HeapReference<Object>))
+ // Load types involving an "index": ArrayGet and
+ // UnsafeGetObject/UnsafeGetObjectVolatile intrinsics.
+ // /* HeapReference<Object> */ ref = *(obj + offset + (index << scale_factor))
if (index.IsConstant()) {
size_t computed_offset =
- (index.GetConstant()->AsIntConstant()->GetValue() << TIMES_4) + offset;
+ (index.GetConstant()->AsIntConstant()->GetValue() << scale_factor) + offset;
__ LoadFromOffset(kLoadWord, ref_reg, obj, computed_offset);
} else {
- __ add(IP, obj, ShifterOperand(index.AsRegister<Register>(), LSL, TIMES_4));
+ // Handle the special case of the
+ // UnsafeGetObject/UnsafeGetObjectVolatile intrinsics, which use
+ // a register pair as index ("long offset"), of which only the low
+ // part contains data.
+ Register index_reg = index.IsRegisterPair()
+ ? index.AsRegisterPairLow<Register>()
+ : index.AsRegister<Register>();
+ __ add(IP, obj, ShifterOperand(index_reg, LSL, scale_factor));
__ LoadFromOffset(kLoadWord, ref_reg, IP, offset);
}
} else {
diff --git a/compiler/optimizing/code_generator_arm.h b/compiler/optimizing/code_generator_arm.h
index 0020f7b..f485924 100644
--- a/compiler/optimizing/code_generator_arm.h
+++ b/compiler/optimizing/code_generator_arm.h
@@ -464,6 +464,16 @@
Location index,
Location temp,
bool needs_null_check);
+ // Factored implementation used by GenerateFieldLoadWithBakerReadBarrier
+ // and GenerateArrayLoadWithBakerReadBarrier.
+ void GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
+ Location ref,
+ Register obj,
+ uint32_t offset,
+ Location index,
+ ScaleFactor scale_factor,
+ Location temp,
+ bool needs_null_check);
// Generate a read barrier for a heap reference within `instruction`
// using a slow path.
@@ -519,16 +529,6 @@
void GenerateExplicitNullCheck(HNullCheck* instruction);
private:
- // Factored implementation of GenerateFieldLoadWithBakerReadBarrier
- // and GenerateArrayLoadWithBakerReadBarrier.
- void GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
- Location ref,
- Register obj,
- uint32_t offset,
- Location index,
- Location temp,
- bool needs_null_check);
-
Register GetInvokeStaticOrDirectExtraParameter(HInvokeStaticOrDirect* invoke, Register temp);
using Uint32ToLiteralMap = ArenaSafeMap<uint32_t, Literal*>;
diff --git a/compiler/optimizing/code_generator_arm64.cc b/compiler/optimizing/code_generator_arm64.cc
index 5d3c8c5..dc5802a 100644
--- a/compiler/optimizing/code_generator_arm64.cc
+++ b/compiler/optimizing/code_generator_arm64.cc
@@ -4940,8 +4940,16 @@
// /* HeapReference<Object> */ ref = *(obj + offset)
Location no_index = Location::NoLocation();
- GenerateReferenceLoadWithBakerReadBarrier(
- instruction, ref, obj, offset, no_index, temp, needs_null_check, use_load_acquire);
+ size_t no_scale_factor = 0U;
+ GenerateReferenceLoadWithBakerReadBarrier(instruction,
+ ref,
+ obj,
+ offset,
+ no_index,
+ no_scale_factor,
+ temp,
+ needs_null_check,
+ use_load_acquire);
}
void CodeGeneratorARM64::GenerateArrayLoadWithBakerReadBarrier(HInstruction* instruction,
@@ -4958,10 +4966,21 @@
// never use Load-Acquire instructions on ARM64.
const bool use_load_acquire = false;
+ static_assert(
+ sizeof(mirror::HeapReference<mirror::Object>) == sizeof(int32_t),
+ "art::mirror::HeapReference<art::mirror::Object> and int32_t have different sizes.");
// /* HeapReference<Object> */ ref =
// *(obj + data_offset + index * sizeof(HeapReference<Object>))
- GenerateReferenceLoadWithBakerReadBarrier(
- instruction, ref, obj, data_offset, index, temp, needs_null_check, use_load_acquire);
+ size_t scale_factor = Primitive::ComponentSizeShift(Primitive::kPrimNot);
+ GenerateReferenceLoadWithBakerReadBarrier(instruction,
+ ref,
+ obj,
+ data_offset,
+ index,
+ scale_factor,
+ temp,
+ needs_null_check,
+ use_load_acquire);
}
void CodeGeneratorARM64::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
@@ -4969,15 +4988,16 @@
vixl::Register obj,
uint32_t offset,
Location index,
+ size_t scale_factor,
Register temp,
bool needs_null_check,
bool use_load_acquire) {
DCHECK(kEmitCompilerReadBarrier);
DCHECK(kUseBakerReadBarrier);
- // If `index` is a valid location, then we are emitting an array
- // load, so we shouldn't be using a Load Acquire instruction.
- // In other words: `index.IsValid()` => `!use_load_acquire`.
- DCHECK(!index.IsValid() || !use_load_acquire);
+ // If we are emitting an array load, we should not be using a
+ // Load Acquire instruction. In other words:
+ // `instruction->IsArrayGet()` => `!use_load_acquire`.
+ DCHECK(!instruction->IsArrayGet() || !use_load_acquire);
MacroAssembler* masm = GetVIXLAssembler();
UseScratchRegisterScope temps(masm);
@@ -5034,20 +5054,33 @@
// The actual reference load.
if (index.IsValid()) {
- static_assert(
- sizeof(mirror::HeapReference<mirror::Object>) == sizeof(int32_t),
- "art::mirror::HeapReference<art::mirror::Object> and int32_t have different sizes.");
- // /* HeapReference<Object> */ ref =
- // *(obj + offset + index * sizeof(HeapReference<Object>))
- const size_t shift_amount = Primitive::ComponentSizeShift(type);
- if (index.IsConstant()) {
- uint32_t computed_offset = offset + (Int64ConstantFrom(index) << shift_amount);
- Load(type, ref_reg, HeapOperand(obj, computed_offset));
+ // Load types involving an "index".
+ if (use_load_acquire) {
+ // UnsafeGetObjectVolatile intrinsic case.
+ // Register `index` is not an index in an object array, but an
+ // offset to an object reference field within object `obj`.
+ DCHECK(instruction->IsInvoke()) << instruction->DebugName();
+ DCHECK(instruction->GetLocations()->Intrinsified());
+ DCHECK(instruction->AsInvoke()->GetIntrinsic() == Intrinsics::kUnsafeGetObjectVolatile)
+ << instruction->AsInvoke()->GetIntrinsic();
+ DCHECK_EQ(offset, 0U);
+ DCHECK_EQ(scale_factor, 0U);
+ DCHECK_EQ(needs_null_check, 0U);
+ // /* HeapReference<Object> */ ref = *(obj + index)
+ MemOperand field = HeapOperand(obj, XRegisterFrom(index));
+ LoadAcquire(instruction, ref_reg, field, /* needs_null_check */ false);
} else {
- temp2 = temps.AcquireW();
- __ Add(temp2, obj, offset);
- Load(type, ref_reg, HeapOperand(temp2, XRegisterFrom(index), LSL, shift_amount));
- temps.Release(temp2);
+ // ArrayGet and UnsafeGetObject intrinsics cases.
+ // /* HeapReference<Object> */ ref = *(obj + offset + (index << scale_factor))
+ if (index.IsConstant()) {
+ uint32_t computed_offset = offset + (Int64ConstantFrom(index) << scale_factor);
+ Load(type, ref_reg, HeapOperand(obj, computed_offset));
+ } else {
+ temp2 = temps.AcquireW();
+ __ Add(temp2, obj, offset);
+ Load(type, ref_reg, HeapOperand(temp2, XRegisterFrom(index), LSL, scale_factor));
+ temps.Release(temp2);
+ }
}
} else {
// /* HeapReference<Object> */ ref = *(obj + offset)
diff --git a/compiler/optimizing/code_generator_arm64.h b/compiler/optimizing/code_generator_arm64.h
index 422963e..55d232a 100644
--- a/compiler/optimizing/code_generator_arm64.h
+++ b/compiler/optimizing/code_generator_arm64.h
@@ -515,6 +515,17 @@
Location index,
vixl::Register temp,
bool needs_null_check);
+ // Factored implementation used by GenerateFieldLoadWithBakerReadBarrier
+ // and GenerateArrayLoadWithBakerReadBarrier.
+ void GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
+ Location ref,
+ vixl::Register obj,
+ uint32_t offset,
+ Location index,
+ size_t scale_factor,
+ vixl::Register temp,
+ bool needs_null_check,
+ bool use_load_acquire);
// Generate a read barrier for a heap reference within `instruction`
// using a slow path.
@@ -570,17 +581,6 @@
void GenerateExplicitNullCheck(HNullCheck* instruction);
private:
- // Factored implementation of GenerateFieldLoadWithBakerReadBarrier
- // and GenerateArrayLoadWithBakerReadBarrier.
- void GenerateReferenceLoadWithBakerReadBarrier(HInstruction* instruction,
- Location ref,
- vixl::Register obj,
- uint32_t offset,
- Location index,
- vixl::Register temp,
- bool needs_null_check,
- bool use_load_acquire);
-
using Uint64ToLiteralMap = ArenaSafeMap<uint64_t, vixl::Literal<uint64_t>*>;
using Uint32ToLiteralMap = ArenaSafeMap<uint32_t, vixl::Literal<uint32_t>*>;
using MethodToLiteralMap = ArenaSafeMap<MethodReference,
diff --git a/compiler/optimizing/intrinsics_arm.cc b/compiler/optimizing/intrinsics_arm.cc
index 29f7672..f094062 100644
--- a/compiler/optimizing/intrinsics_arm.cc
+++ b/compiler/optimizing/intrinsics_arm.cc
@@ -524,8 +524,8 @@
if (kEmitCompilerReadBarrier) {
if (kUseBakerReadBarrier) {
Location temp = locations->GetTemp(0);
- codegen->GenerateArrayLoadWithBakerReadBarrier(
- invoke, trg_loc, base, 0U, offset_loc, temp, /* needs_null_check */ false);
+ codegen->GenerateReferenceLoadWithBakerReadBarrier(
+ invoke, trg_loc, base, 0U, offset_loc, TIMES_1, temp, /* needs_null_check */ false);
if (is_volatile) {
__ dmb(ISH);
}
@@ -581,10 +581,11 @@
locations->SetInAt(0, Location::NoLocation()); // Unused receiver.
locations->SetInAt(1, Location::RequiresRegister());
locations->SetInAt(2, Location::RequiresRegister());
- locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
+ locations->SetOut(Location::RequiresRegister(),
+ can_call ? Location::kOutputOverlap : Location::kNoOutputOverlap);
if (type == Primitive::kPrimNot && kEmitCompilerReadBarrier && kUseBakerReadBarrier) {
// We need a temporary register for the read barrier marking slow
- // path in InstructionCodeGeneratorARM::GenerateArrayLoadWithBakerReadBarrier.
+ // path in InstructionCodeGeneratorARM::GenerateReferenceLoadWithBakerReadBarrier.
locations->AddTemp(Location::RequiresRegister());
}
}
diff --git a/compiler/optimizing/intrinsics_arm64.cc b/compiler/optimizing/intrinsics_arm64.cc
index c8d6ddc..12d65be 100644
--- a/compiler/optimizing/intrinsics_arm64.cc
+++ b/compiler/optimizing/intrinsics_arm64.cc
@@ -791,8 +791,15 @@
// UnsafeGetObject/UnsafeGetObjectVolatile with Baker's read barrier case.
UseScratchRegisterScope temps(masm);
Register temp = temps.AcquireW();
- codegen->GenerateArrayLoadWithBakerReadBarrier(
- invoke, trg_loc, base, 0U, offset_loc, temp, /* needs_null_check */ false);
+ codegen->GenerateReferenceLoadWithBakerReadBarrier(invoke,
+ trg_loc,
+ base,
+ /* offset */ 0U,
+ /* index */ offset_loc,
+ /* scale_factor */ 0U,
+ temp,
+ /* needs_null_check */ false,
+ is_volatile);
} else {
// Other cases.
MemOperand mem_op(base.X(), offset);
@@ -821,7 +828,8 @@
locations->SetInAt(0, Location::NoLocation()); // Unused receiver.
locations->SetInAt(1, Location::RequiresRegister());
locations->SetInAt(2, Location::RequiresRegister());
- locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
+ locations->SetOut(Location::RequiresRegister(),
+ can_call ? Location::kOutputOverlap : Location::kNoOutputOverlap);
}
void IntrinsicLocationsBuilderARM64::VisitUnsafeGet(HInvoke* invoke) {