Skip to content

V
V8
Commit 6ae7f2b5 authored by jacob.bramley's avatar jacob.bramley Committed by Commit bot
Browse files
Options

[arm64] Avoid signed arithmetic in AddWithCarry. 

This avoids implementation-defined signed overflow in the simulator's
AddWithCarry implementation. The implementation of AddWithCarry now uses
unsigned arithmetic exclusively.

Testing coverage is also significantly improved.

BUG=

Review-Url: https://codereview.chromium.org/2157283003
Cr-Commit-Position: refs/heads/master@{#37895}
parent f52263ec
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 418 additions and 175 deletions
src/arm64/simulator-arm64.cc
View file @ 6ae7f2b5
......@@ -888,36 +888,31 @@ int Simulator::CodeFromName(const char* name) {
// Helpers ---------------------------------------------------------------------
template <typename T>
T Simulator::AddWithCarry(bool set_flags,
T src1,
T src2,
T carry_in) {
typedef typename make_unsigned<T>::type unsignedT;
DCHECK((carry_in == 0) || (carry_in == 1));
T signed_sum = src1 + src2 + carry_in;
T result = signed_sum;
T Simulator::AddWithCarry(bool set_flags, T left, T right, int carry_in) {
// Use unsigned types to avoid implementation-defined overflow behaviour.
static_assert(std::is_unsigned<T>::value, "operands must be unsigned");
static_assert((sizeof(T) == kWRegSize) || (sizeof(T) == kXRegSize),
"Only W- or X-sized operands are tested");
bool N, Z, C, V;
DCHECK((carry_in == 0) || (carry_in == 1));
T result = left + right + carry_in;
// Compute the C flag
unsignedT u1 = static_cast<unsignedT>(src1);
unsignedT u2 = static_cast<unsignedT>(src2);
unsignedT urest = std::numeric_limits<unsignedT>::max() - u1;
C = (u2 > urest) || (carry_in && (((u2 + 1) > urest) || (u2 > (urest - 1))));
if (set_flags) {
nzcv().SetN(CalcNFlag(result));
nzcv().SetZ(CalcZFlag(result));
// Overflow iff the sign bit is the same for the two inputs and different
// for the result.
V = ((src1 ^ src2) >= 0) && ((src1 ^ result) < 0);
// Compute the C flag by comparing the result to the max unsigned integer.
T max_uint_2op = std::numeric_limits<T>::max() - carry_in;
nzcv().SetC((left > max_uint_2op) || ((max_uint_2op - left) < right));
N = CalcNFlag(result);
Z = CalcZFlag(result);
// Overflow iff the sign bit is the same for the two inputs and different
// for the result.
T sign_mask = T(1) << (sizeof(T) * 8 - 1);
T left_sign = left & sign_mask;
T right_sign = right & sign_mask;
T result_sign = result & sign_mask;
nzcv().SetV((left_sign == right_sign) && (left_sign != result_sign));
if (set_flags) {
nzcv().SetN(N);
nzcv().SetZ(Z);
nzcv().SetC(C);
nzcv().SetV(V);
LogSystemRegister(NZCV);
}
return result;
......@@ -926,6 +921,9 @@ T Simulator::AddWithCarry(bool set_flags,
template<typename T>
void Simulator::AddSubWithCarry(Instruction* instr) {
// Use unsigned types to avoid implementation-defined overflow behaviour.
static_assert(std::is_unsigned<T>::value, "operands must be unsigned");
T op2 = reg<T>(instr->Rm());
T new_val;
......@@ -1418,6 +1416,9 @@ void Simulator::VisitCompareBranch(Instruction* instr) {
template<typename T>
void Simulator::AddSubHelper(Instruction* instr, T op2) {
// Use unsigned types to avoid implementation-defined overflow behaviour.
static_assert(std::is_unsigned<T>::value, "operands must be unsigned");
bool set_flags = instr->FlagsUpdate();
T new_val = 0;
Instr operation = instr->Mask(AddSubOpMask);
......@@ -1450,11 +1451,10 @@ void Simulator::VisitAddSubShifted(Instruction* instr) {
unsigned shift_amount = instr->ImmDPShift();
if (instr->SixtyFourBits()) {
int64_t op2 = ShiftOperand(xreg(instr->Rm()), shift_type, shift_amount);
uint64_t op2 = ShiftOperand(xreg(instr->Rm()), shift_type, shift_amount);
AddSubHelper(instr, op2);
} else {
int32_t op2 = static_cast<int32_t>(
ShiftOperand(wreg(instr->Rm()), shift_type, shift_amount));
uint32_t op2 = ShiftOperand(wreg(instr->Rm()), shift_type, shift_amount);
AddSubHelper(instr, op2);
}
}
......@@ -1463,9 +1463,9 @@ void Simulator::VisitAddSubShifted(Instruction* instr) {
void Simulator::VisitAddSubImmediate(Instruction* instr) {
int64_t op2 = instr->ImmAddSub() << ((instr->ShiftAddSub() == 1) ? 12 : 0);
if (instr->SixtyFourBits()) {
AddSubHelper<int64_t>(instr, op2);
AddSubHelper(instr, static_cast<uint64_t>(op2));
} else {
AddSubHelper<int32_t>(instr, static_cast<int32_t>(op2));
AddSubHelper(instr, static_cast<uint32_t>(op2));
}
}
......@@ -1474,10 +1474,10 @@ void Simulator::VisitAddSubExtended(Instruction* instr) {
Extend ext = static_cast<Extend>(instr->ExtendMode());
unsigned left_shift = instr->ImmExtendShift();
if (instr->SixtyFourBits()) {
int64_t op2 = ExtendValue(xreg(instr->Rm()), ext, left_shift);
uint64_t op2 = ExtendValue(xreg(instr->Rm()), ext, left_shift);
AddSubHelper(instr, op2);
} else {
int32_t op2 = ExtendValue(wreg(instr->Rm()), ext, left_shift);
uint32_t op2 = ExtendValue(wreg(instr->Rm()), ext, left_shift);
AddSubHelper(instr, op2);
}
}
......@@ -1485,9 +1485,9 @@ void Simulator::VisitAddSubExtended(Instruction* instr) {
void Simulator::VisitAddSubWithCarry(Instruction* instr) {
if (instr->SixtyFourBits()) {
AddSubWithCarry<int64_t>(instr);
AddSubWithCarry<uint64_t>(instr);
} else {
AddSubWithCarry<int32_t>(instr);
AddSubWithCarry<uint32_t>(instr);
}
}
......@@ -1497,22 +1497,22 @@ void Simulator::VisitLogicalShifted(Instruction* instr) {
unsigned shift_amount = instr->ImmDPShift();
if (instr->SixtyFourBits()) {
int64_t op2 = ShiftOperand(xreg(instr->Rm()), shift_type, shift_amount);
uint64_t op2 = ShiftOperand(xreg(instr->Rm()), shift_type, shift_amount);
op2 = (instr->Mask(NOT) == NOT) ? ~op2 : op2;
LogicalHelper<int64_t>(instr, op2);
LogicalHelper(instr, op2);
} else {
int32_t op2 = ShiftOperand(wreg(instr->Rm()), shift_type, shift_amount);
uint32_t op2 = ShiftOperand(wreg(instr->Rm()), shift_type, shift_amount);
op2 = (instr->Mask(NOT) == NOT) ? ~op2 : op2;
LogicalHelper<int32_t>(instr, op2);
LogicalHelper(instr, op2);
}
}
void Simulator::VisitLogicalImmediate(Instruction* instr) {
if (instr->SixtyFourBits()) {
LogicalHelper<int64_t>(instr, instr->ImmLogical());
LogicalHelper(instr, static_cast<uint64_t>(instr->ImmLogical()));
} else {
LogicalHelper<int32_t>(instr, static_cast<int32_t>(instr->ImmLogical()));
LogicalHelper(instr, static_cast<uint32_t>(instr->ImmLogical()));
}
}
......@@ -1548,24 +1548,27 @@ void Simulator::LogicalHelper(Instruction* instr, T op2) {
void Simulator::VisitConditionalCompareRegister(Instruction* instr) {
if (instr->SixtyFourBits()) {
ConditionalCompareHelper(instr, xreg(instr->Rm()));
ConditionalCompareHelper(instr, static_cast<uint64_t>(xreg(instr->Rm())));
} else {
ConditionalCompareHelper(instr, wreg(instr->Rm()));
ConditionalCompareHelper(instr, static_cast<uint32_t>(wreg(instr->Rm())));
}
}
void Simulator::VisitConditionalCompareImmediate(Instruction* instr) {
if (instr->SixtyFourBits()) {
ConditionalCompareHelper<int64_t>(instr, instr->ImmCondCmp());
ConditionalCompareHelper(instr, static_cast<uint64_t>(instr->ImmCondCmp()));
} else {
ConditionalCompareHelper<int32_t>(instr, instr->ImmCondCmp());
ConditionalCompareHelper(instr, static_cast<uint32_t>(instr->ImmCondCmp()));
}
}
template<typename T>
void Simulator::ConditionalCompareHelper(Instruction* instr, T op2) {
// Use unsigned types to avoid implementation-defined overflow behaviour.
static_assert(std::is_unsigned<T>::value, "operands must be unsigned");
T op1 = reg<T>(instr->Rn());
if (ConditionPassed(static_cast<Condition>(instr->Condition()))) {
......
src/arm64/simulator-arm64.h
View file @ 6ae7f2b5
......@@ -652,11 +652,8 @@ class Simulator : public DecoderVisitor {
template<typename T>
void AddSubHelper(Instruction* instr, T op2);
template<typename T>
T AddWithCarry(bool set_flags,
T src1,
T src2,
T carry_in = 0);
template <typename T>
T AddWithCarry(bool set_flags, T left, T right, int carry_in = 0);
template<typename T>
void AddSubWithCarry(Instruction* instr);
template<typename T>
......
test/cctest/test-assembler-arm64.cc
View file @ 6ae7f2b5
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment