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
diff --git a/compiler/dex/quick/arm/int_arm.cc b/compiler/dex/quick/arm/int_arm.cc
index 1a4b23e..bf09446 100644
--- a/compiler/dex/quick/arm/int_arm.cc
+++ b/compiler/dex/quick/arm/int_arm.cc
@@ -845,7 +845,7 @@
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 @@
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 9f7a881..3f7ecfe 100644
--- a/compiler/dex/quick/gen_common.cc
+++ b/compiler/dex/quick/gen_common.cc
@@ -214,9 +214,8 @@
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 @@
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 @@
}
}
- 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 c308932..bafb57d 100755
--- a/compiler/dex/quick/gen_invoke.cc
+++ b/compiler/dex/quick/gen_invoke.cc
@@ -1643,7 +1643,7 @@
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 e5798fd..39b40a0 100644
--- a/compiler/dex/quick/gen_loadstore.cc
+++ b/compiler/dex/quick/gen_loadstore.cc
@@ -166,10 +166,6 @@
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 67a8c0f..3de4c56 100644
--- a/compiler/dex/quick/mir_to_lir.h
+++ b/compiler/dex/quick/mir_to_lir.h
@@ -1008,8 +1008,6 @@
}
// 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 4357657..8638204 100755
--- a/compiler/dex/quick/x86/int_x86.cc
+++ b/compiler/dex/quick/x86/int_x86.cc
@@ -1155,7 +1155,7 @@
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.