-
Notifications
You must be signed in to change notification settings - Fork 273
Get lhs_object from byte_extract for goto trace #4480
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
Get lhs_object from byte_extract for goto trace #4480
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this improving - I don't think you'd even need to add a new test, existing tests like regression/cbmc/array_constraints1
also have this problem.
src.id() == ID_byte_extract_little_endian || | ||
src.id() == ID_byte_extract_big_endian) | ||
{ | ||
return get_object_rec(to_byte_extract_expr(src).op()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me, do we actually consider the offset? What you propose is consistent with what's done for index and member expressions, but I'm still wondering whether the overall result is meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was not considered. The meaningfulness is questionable, but that probably goes beyond this fix.
It might be good to actually specify what these fields are supposed to mean precisely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example to validate:
#include <assert.h>
int main()
{
int x;
char *p = (char *)&x + 2;
*p = 1;
assert(x == 0);
}
The trace currently includes byte_extract_little_endian(x, 2l, char)=1 (00000001)
- which might not be downstream-consumable, but at least is accurate. If after this PR the result is x=1
then there's a problem I'm afraid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goto_trace_steps offer two bits of information:
- The thing returned by
get_lhs_object()
-- that is the object as defined in the C standard, andx
is the right answer. - The
full_lhs
, which is documented to be "after dereferencing". It is supposed to be exactly what gets assigned. The byte_extract expression above is the best answer for that, in my view.
The trace given to the user should show the latter. We may wish to print byte_extract expressions there as a pointer typecast plus dereferencing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarifying! +1 for printing byte_extract
as some C expression, which will be important for SV-COMP as well. But I accept that this might be outside the scope of this PR.
regression/cbmc/xml-trace/test.desc
Outdated
<assignment assignment_type="state" base_name="u" display_name="test::u" hidden="false" identifier="test::u" mode="C" step_nr="\d+" thread="0"> | ||
<location file=".*" function="test" line="\d+" working-directory=".*"/> | ||
<type>union myunion</type> | ||
<full_lhs>byte_extract_little_endian\(u, 0l, .*int.*\)</full_lhs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add --little-endian
to the command line to make sure this test is portable.
5a2c9a6
to
8d4a15f
Compare
65de48f
to
670219f
Compare
Otherwise 'identifier' is not displayed in XML output
670219f
to
60b5328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
Passed Diffblue compatibility checks (cbmc commit: 60b5328).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/107058275
Otherwise fields are missing from the XML output, which breaks downstream integration of a commercial CBMC user.