From bfea33585e229973f7887afbf51fe45c2ba41e91 Mon Sep 17 00:00:00 2001 From: Roland Levillain Date: Thu, 23 Jun 2016 13:48:47 +0100 Subject: [PATCH] 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. --- compiler/optimizing/code_generator_arm.cc | 29 +++++++---- compiler/optimizing/code_generator_arm.h | 20 ++++---- compiler/optimizing/code_generator_arm64.cc | 75 +++++++++++++++++++++-------- compiler/optimizing/code_generator_arm64.h | 22 ++++----- compiler/optimizing/intrinsics_arm.cc | 9 ++-- compiler/optimizing/intrinsics_arm64.cc | 14 ++++-- 6 files changed, 111 insertions(+), 58 deletions(-) diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc index eca9e2c29..2b63f3d0f 100644 --- a/compiler/optimizing/code_generator_arm.cc +++ b/compiler/optimizing/code_generator_arm.cc @@ -6085,8 +6085,9 @@ void CodeGeneratorARM::GenerateFieldLoadWithBakerReadBarrier(HInstruction* instr // /* HeapReference */ 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 @@ void CodeGeneratorARM::GenerateArrayLoadWithBakerReadBarrier(HInstruction* instr DCHECK(kEmitCompilerReadBarrier); DCHECK(kUseBakerReadBarrier); + static_assert( + sizeof(mirror::HeapReference) == sizeof(int32_t), + "art::mirror::HeapReference and int32_t have different sizes."); // /* HeapReference */ ref = // *(obj + data_offset + index * sizeof(HeapReference)) + 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 @@ void CodeGeneratorARM::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* i Register obj, uint32_t offset, Location index, + ScaleFactor scale_factor, Location temp, bool needs_null_check) { DCHECK(kEmitCompilerReadBarrier); @@ -6164,17 +6170,22 @@ void CodeGeneratorARM::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* i // The actual reference load. if (index.IsValid()) { - static_assert( - sizeof(mirror::HeapReference) == sizeof(int32_t), - "art::mirror::HeapReference and int32_t have different sizes."); - // /* HeapReference */ ref = - // *(obj + offset + index * sizeof(HeapReference)) + // Load types involving an "index": ArrayGet and + // UnsafeGetObject/UnsafeGetObjectVolatile intrinsics. + // /* HeapReference */ 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(), 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() + : index.AsRegister(); + __ 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 0020f7b4f..f48592477 100644 --- a/compiler/optimizing/code_generator_arm.h +++ b/compiler/optimizing/code_generator_arm.h @@ -464,6 +464,16 @@ class CodeGeneratorARM : public CodeGenerator { 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 @@ class CodeGeneratorARM : public CodeGenerator { 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; diff --git a/compiler/optimizing/code_generator_arm64.cc b/compiler/optimizing/code_generator_arm64.cc index 5d3c8c559..dc5802aff 100644 --- a/compiler/optimizing/code_generator_arm64.cc +++ b/compiler/optimizing/code_generator_arm64.cc @@ -4940,8 +4940,16 @@ void CodeGeneratorARM64::GenerateFieldLoadWithBakerReadBarrier(HInstruction* ins // /* HeapReference */ 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 @@ void CodeGeneratorARM64::GenerateArrayLoadWithBakerReadBarrier(HInstruction* ins // never use Load-Acquire instructions on ARM64. const bool use_load_acquire = false; + static_assert( + sizeof(mirror::HeapReference) == sizeof(int32_t), + "art::mirror::HeapReference and int32_t have different sizes."); // /* HeapReference */ ref = // *(obj + data_offset + index * sizeof(HeapReference)) - 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 @@ void CodeGeneratorARM64::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* 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 @@ void CodeGeneratorARM64::GenerateReferenceLoadWithBakerReadBarrier(HInstruction* // The actual reference load. if (index.IsValid()) { - static_assert( - sizeof(mirror::HeapReference) == sizeof(int32_t), - "art::mirror::HeapReference and int32_t have different sizes."); - // /* HeapReference */ ref = - // *(obj + offset + index * sizeof(HeapReference)) - 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 */ 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 */ 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 */ ref = *(obj + offset) diff --git a/compiler/optimizing/code_generator_arm64.h b/compiler/optimizing/code_generator_arm64.h index 422963e7d..55d232a11 100644 --- a/compiler/optimizing/code_generator_arm64.h +++ b/compiler/optimizing/code_generator_arm64.h @@ -515,6 +515,17 @@ class CodeGeneratorARM64 : public CodeGenerator { 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 @@ class CodeGeneratorARM64 : public CodeGenerator { 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*>; using Uint32ToLiteralMap = ArenaSafeMap*>; using MethodToLiteralMap = ArenaSafeMapGetTemp(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 @@ static void CreateIntIntIntToIntLocations(ArenaAllocator* arena, 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 c8d6ddc8f..12d65beab 100644 --- a/compiler/optimizing/intrinsics_arm64.cc +++ b/compiler/optimizing/intrinsics_arm64.cc @@ -791,8 +791,15 @@ static void GenUnsafeGet(HInvoke* invoke, // 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 @@ static void CreateIntIntIntToIntLocations(ArenaAllocator* arena, HInvoke* invoke 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) { -- 2.11.0