Skip to content

shift left overflow check now done in goto_check #3662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions regression/cbmc/Overflow_Leftshift1/main.c
Original file line number Diff line number Diff line change
@@ -1,11 +1,24 @@
int main()
{
unsigned char x;

// signed, owing to promotion, and may overflow
unsigned r=x << ((sizeof(unsigned)-1)*8);

// signed, owing to promotion, and cannot overflow
r=x << ((sizeof(unsigned)-1)*8-1);

// unsigned
r=(unsigned)x << ((sizeof(unsigned)-1)*8);

int s=-1 << ((sizeof(int)-1)*8);
s=-256 << ((sizeof(int)-1)*8);
// negative value or zero, not an overflow
int s = -x << ((sizeof(int) - 1) * 8);

// overflow
s = 1 << x;

// distance too far, not an overflow
s = s << 100;

return 0;
}
9 changes: 7 additions & 2 deletions regression/cbmc/Overflow_Leftshift1/test.desc
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@ main.c
--signed-overflow-check
^EXIT=10$
^SIGNAL=0$
^\[.*\] .* arithmetic overflow on signed shl in .*: FAILURE$
^\*\* 2 of 4 failed
^\[.*\] line 6 arithmetic overflow on signed shl in .*: FAILURE$
^\[.*\] line 9 arithmetic overflow on signed shl in .*: SUCCESS$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a must-not-match for "line 12 arithmetic overflow" if that's intentionally not tested?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

^\[.*\] line 15 arithmetic overflow on signed shl in .*: SUCCESS$
^\[.*\] line 18 arithmetic overflow on signed shl in .*: FAILURE$
^\*\* 2 of 5 failed
^VERIFICATION FAILED$
--
^warning: ignoring
^\[.*\] line 12 arithmetic overflow on signed shl in .*: .*
^\[.*\] line 21 arithmetic overflow on signed shl in .*: .*
2 changes: 0 additions & 2 deletions scripts/delete_failing_smt2_solver_tests
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ rm Linking7/test.desc
rm Linking7/member-name-mismatch.desc
rm Malloc23/test.desc
rm Multi_Dimensional_Array2/test.desc
rm Overflow_Leftshift1/test.desc
rm Overflow_Subtraction1/test.desc
rm Pointer_Arithmetic11/test.desc
rm Pointer_byte_extract2/test.desc
rm Pointer_byte_extract5/no-simplify.desc
Expand Down
75 changes: 75 additions & 0 deletions src/analyses/goto_check.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,81 @@ void goto_checkt::integer_overflow_check(

return;
}
else if(expr.id() == ID_shl)
{
if(type.id() == ID_signedbv)
{
const auto &shl_expr = to_shl_expr(expr);
const auto &op = shl_expr.op();
const auto &op_type = to_signedbv_type(type);
const auto op_width = op_type.get_width();
const auto &distance = shl_expr.distance();
const auto &distance_type = distance.type();

// a left shift of a negative value is undefined;
// yet this isn't an overflow
exprt neg_value_shift;

if(op_type.id() == ID_unsignedbv)
neg_value_shift = false_exprt();
else
neg_value_shift =
binary_relation_exprt(op, ID_lt, from_integer(0, op_type));

// a shift with negative distance is undefined;
// yet this isn't an overflow
exprt neg_dist_shift;

if(distance_type.id() == ID_unsignedbv)
neg_dist_shift = false_exprt();
else
neg_dist_shift =
binary_relation_exprt(op, ID_lt, from_integer(0, distance_type));

// shifting a non-zero value by more than its width is undefined;
// yet this isn't an overflow
const exprt dist_too_large = binary_predicate_exprt(
distance, ID_gt, from_integer(op_width, distance_type));

const exprt op_zero = equal_exprt(op, from_integer(0, op_type));

auto new_type = to_bitvector_type(op_type);
new_type.set_width(op_width * 2);

const exprt op_ext = typecast_exprt(op, new_type);

const exprt op_ext_shifted = shl_exprt(op_ext, distance);

// get top bits of the shifted operand
const exprt top_bits = extractbits_exprt(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Why shift and then extract? All the bits we're interested in testing are present in the original value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our extactbits matches the one in the SMT-LIB, and thus, only does constants.

op_ext_shifted,
new_type.get_width() - 1,
op_width - 1,
unsignedbv_typet(op_width + 1));

const exprt top_bits_zero =
equal_exprt(top_bits, from_integer(0, top_bits.type()));

// a negative distance shift isn't an overflow;
// a negative value shift isn't an overflow;
// a shift that's too far isn't an overflow;
// a shift of zero isn't overflow;
// else check the top bits
add_guarded_claim(
disjunction({neg_value_shift,
neg_dist_shift,
dist_too_large,
op_zero,
top_bits_zero}),
"arithmetic overflow on signed shl",
"overflow",
expr.find_source_location(),
expr,
guard);
}

return;
}

exprt overflow("overflow-"+expr.id_string(), bool_typet());
overflow.operands()=expr.operands();
Expand Down
23 changes: 23 additions & 0 deletions src/util/std_expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -2960,6 +2960,29 @@ class shl_exprt:public shift_exprt
}
};

/// \brief Cast an exprt to a \ref shl_exprt
///
/// \a expr must be known to be \ref shl_exprt.
///
/// \param expr: Source expression
/// \return Object of type \ref shl_exprt
inline const shl_exprt &to_shl_expr(const exprt &expr)
{
PRECONDITION(expr.id() == ID_shl);
DATA_INVARIANT(
expr.operands().size() == 2, "Bit-wise shl must have two operands");
return static_cast<const shl_exprt &>(expr);
}

/// \copydoc to_shl_expr(const exprt &)
inline shl_exprt &to_shl_expr(exprt &expr)
{
PRECONDITION(expr.id() == ID_shl);
DATA_INVARIANT(
expr.operands().size() == 2, "Bit-wise shl must have two operands");
return static_cast<shl_exprt &>(expr);
}

/// \brief Arithmetic right shift
class ashr_exprt:public shift_exprt
{
Expand Down