From 7c02e918e752ab36f0b6cab7528f10c0cf55a4ee Mon Sep 17 00:00:00 2001 From: buzbee Date: Fri, 3 Oct 2014 13:14:17 -0700 Subject: [PATCH] Quick compiler: Fix ambiguous LoadValue() Internal b/17790197 & hat tip to Stephen Kyle The following custom-edited dex program demonstrated incorrect code generation caused by type confusion. In the example, the constant held in v0 is used in both float and int contexts, and the register class gets confused at the if-eq. .method private static getInt()I .registers 4 const/16 v0, 100 const/4 v1, 1 const/4 v2, 7 :loop if-eq v2, v0, :done add-int v2, v2, v1 goto :loop :done add-float v3, v0, v1 return v2 .end method The bug was introduced in c/96499, "Quick compiler: reference cleanup" That CL created a convenience variant of LoadValue which selected the target register type based on the type of the RegLocation. It should not have done so. The type of a RegLocation is the compiler's best guess of the Dalvik type - and Dalvik allows constants to be used in multiple type contexts. All code generation utilities must specify desired register class based on the capabilities of the instructions to be emitted. In the failing case, OpCmpImmBranch (and GenCompareZeroAndBranch) will be using core registers, so the LoadValue must specify either kCoreReg or kRefReg. The CL deletes the dangerous LoadValue() variant. Change-Id: Ie4ec6e51b19676dbbb9628c72c8b3473a419e7ec --- compiler/dex/quick/arm/int_arm.cc | 4 ++-- compiler/dex/quick/gen_common.cc | 11 +++++------ compiler/dex/quick/gen_invoke.cc | 2 +- compiler/dex/quick/gen_loadstore.cc | 4 ---- compiler/dex/quick/mir_to_lir.h | 2 -- compiler/dex/quick/x86/int_x86.cc | 2 +- 6 files changed, 9 insertions(+), 16 deletions(-) diff --git a/compiler/dex/quick/arm/int_arm.cc b/compiler/dex/quick/arm/int_arm.cc index 1a4b23e27..bf0944643 100644 --- a/compiler/dex/quick/arm/int_arm.cc +++ b/compiler/dex/quick/arm/int_arm.cc @@ -845,7 +845,7 @@ bool ArmMir2Lir::GenInlinedCas(CallInfo* info, bool is_long, bool is_object) { RegLocation rl_object = LoadValue(rl_src_obj, kRefReg); RegLocation rl_new_value; if (!is_long) { - rl_new_value = LoadValue(rl_src_new_value); + rl_new_value = LoadValue(rl_src_new_value, LocToRegClass(rl_src_new_value)); } else if (load_early) { rl_new_value = LoadValueWide(rl_src_new_value, kCoreReg); } @@ -868,7 +868,7 @@ bool ArmMir2Lir::GenInlinedCas(CallInfo* info, bool is_long, bool is_object) { RegLocation rl_expected; if (!is_long) { - rl_expected = LoadValue(rl_src_expected); + rl_expected = LoadValue(rl_src_expected, LocToRegClass(rl_src_new_value)); } else if (load_early) { rl_expected = LoadValueWide(rl_src_expected, kCoreReg); } else { diff --git a/compiler/dex/quick/gen_common.cc b/compiler/dex/quick/gen_common.cc index 9f7a8813c..3f7ecfe03 100644 --- a/compiler/dex/quick/gen_common.cc +++ b/compiler/dex/quick/gen_common.cc @@ -214,9 +214,8 @@ void Mir2Lir::ForceImplicitNullCheck(RegStorage reg, int opt_flags) { void Mir2Lir::GenCompareAndBranch(Instruction::Code opcode, RegLocation rl_src1, RegLocation rl_src2, LIR* taken, LIR* fall_through) { - DCHECK(!rl_src1.fp); - DCHECK(!rl_src2.fp); ConditionCode cond; + RegisterClass reg_class = (rl_src1.ref || rl_src2.ref) ? kRefReg : kCoreReg; switch (opcode) { case Instruction::IF_EQ: cond = kCondEq; @@ -249,7 +248,7 @@ void Mir2Lir::GenCompareAndBranch(Instruction::Code opcode, RegLocation rl_src1, cond = FlipComparisonOrder(cond); } - rl_src1 = LoadValue(rl_src1); + rl_src1 = LoadValue(rl_src1, reg_class); // Is this really an immediate comparison? if (rl_src2.is_const) { // If it's already live in a register or not easily materialized, just keep going @@ -273,15 +272,15 @@ void Mir2Lir::GenCompareAndBranch(Instruction::Code opcode, RegLocation rl_src1, } } - rl_src2 = LoadValue(rl_src2); + rl_src2 = LoadValue(rl_src2, reg_class); OpCmpBranch(cond, rl_src1.reg, rl_src2.reg, taken); } void Mir2Lir::GenCompareZeroAndBranch(Instruction::Code opcode, RegLocation rl_src, LIR* taken, LIR* fall_through) { ConditionCode cond; - DCHECK(!rl_src.fp); - rl_src = LoadValue(rl_src); + RegisterClass reg_class = rl_src.ref ? kRefReg : kCoreReg; + rl_src = LoadValue(rl_src, reg_class); switch (opcode) { case Instruction::IF_EQZ: cond = kCondEq; diff --git a/compiler/dex/quick/gen_invoke.cc b/compiler/dex/quick/gen_invoke.cc index c308932bc..bafb57d60 100755 --- a/compiler/dex/quick/gen_invoke.cc +++ b/compiler/dex/quick/gen_invoke.cc @@ -1643,7 +1643,7 @@ bool Mir2Lir::GenInlinedUnsafePut(CallInfo* info, bool is_long, FreeTemp(rl_temp_offset); } } else { - rl_value = LoadValue(rl_src_value); + rl_value = LoadValue(rl_src_value, LocToRegClass(rl_src_value)); if (rl_value.ref) { StoreRefIndexed(rl_object.reg, rl_offset.reg, rl_value.reg, 0); } else { diff --git a/compiler/dex/quick/gen_loadstore.cc b/compiler/dex/quick/gen_loadstore.cc index e5798fdc0..39b40a09a 100644 --- a/compiler/dex/quick/gen_loadstore.cc +++ b/compiler/dex/quick/gen_loadstore.cc @@ -166,10 +166,6 @@ RegLocation Mir2Lir::LoadValue(RegLocation rl_src, RegisterClass op_kind) { return rl_src; } -RegLocation Mir2Lir::LoadValue(RegLocation rl_src) { - return LoadValue(rl_src, LocToRegClass(rl_src)); -} - void Mir2Lir::StoreValue(RegLocation rl_dest, RegLocation rl_src) { /* * Sanity checking - should never try to store to the same diff --git a/compiler/dex/quick/mir_to_lir.h b/compiler/dex/quick/mir_to_lir.h index 67a8c0f40..3de4c563d 100644 --- a/compiler/dex/quick/mir_to_lir.h +++ b/compiler/dex/quick/mir_to_lir.h @@ -1008,8 +1008,6 @@ class Mir2Lir : public Backend { } // Load Dalvik value with 32-bit memory storage. If compressed object reference, decompress. virtual RegLocation LoadValue(RegLocation rl_src, RegisterClass op_kind); - // Same as above, but derive the target register class from the location record. - virtual RegLocation LoadValue(RegLocation rl_src); // Load Dalvik value with 64-bit memory storage. virtual RegLocation LoadValueWide(RegLocation rl_src, RegisterClass op_kind); // Load Dalvik value with 32-bit memory storage. If compressed object reference, decompress. diff --git a/compiler/dex/quick/x86/int_x86.cc b/compiler/dex/quick/x86/int_x86.cc index 435765768..863820487 100755 --- a/compiler/dex/quick/x86/int_x86.cc +++ b/compiler/dex/quick/x86/int_x86.cc @@ -1155,7 +1155,7 @@ bool X86Mir2Lir::GenInlinedCas(CallInfo* info, bool is_long, bool is_object) { LockTemp(rs_r0); RegLocation rl_object = LoadValue(rl_src_obj, kRefReg); - RegLocation rl_new_value = LoadValue(rl_src_new_value); + RegLocation rl_new_value = LoadValue(rl_src_new_value, LocToRegClass(rl_src_new_value)); if (is_object && !mir_graph_->IsConstantNullRef(rl_new_value)) { // Mark card for object assuming new value is stored. -- 2.11.0