Simplify and avoid redundant typecasts in build_object_descriptor #4183
Conversation
These can fool later simplifications by appearing to be non-constant.
Leaving this with trivially simplifiable expressions such as (0 + 0) means subsequent simplifications that check for offset.is_constant() may be wrongly skipped.
smowton
requested review from
kroening,
peterschrammel,
pkesseli and
tautschnig
as code owners
| @@ -145,6 +146,7 @@ void object_descriptor_exprt::build( | |||
| offset()=from_integer(0, index_type()); | |||
|
|
|||
| build_object_descriptor_rec(ns, expr, *this); | |||
| simplify(offset(), ns); | |||
There was a problem hiding this comment.
I'm a bit worried about the cost, can you check that callees don't currently run this after invoking build? (And if they do, remove it from those places.)
There was a problem hiding this comment.
Checked, there was one redundant simplify (presumably working around the same issue). A couple of others use the offset to build a composite expression then simplify that, and so would have coincidentally dodged this problem. Some others uses ode.offset().is_zero(), which will probably have failed spuriously before when the offset was composite-but-constant.
This test indirectly uses object_descriptor_exprt, which uses simplify_sum, which in turn requires integer constants to have defined widths.
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 8c231e9).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/100903764
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 7d0ce45).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/100908538
Otherwise this produces odd expressions like (1 + (int)2), which can lead to missed opportunities since that doesn't pass
is_constant()