-
Notifications
You must be signed in to change notification settings - Fork 285
dump-c: handle further byte_update union initializers #5799
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
Conversation
|
Thanks for this! We had a similar issue crop up in https://github.com/diffblue/cbmc/runs/1830186932. Is that also a related issue? On a more general note, I'm not very familiar with CSmith. Is there any material to quickly familiarise myself with it to that I can interpret those reports better, or is it better if I get to spend more time diving deeper on it? |
Codecov Report
@@ Coverage Diff @@
## develop #5799 +/- ##
========================================
Coverage 69.72% 69.72%
========================================
Files 1242 1242
Lines 100907 100908 +1
========================================
+ Hits 70361 70363 +2
+ Misses 30546 30545 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
It's the very same issue and should be fixed by this PR.
CSmith lives here: https://embed.cs.utah.edu/csmith/ - but our |
martin-cs
left a comment
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.
I'm not sure if I see how this fixes the issue and the commit message doesn't say but it seems like the kind of change that would.
The changes of a8592a7 made the C front-end introduce byte updates to ensure compliance with the behaviour of popular compilers when initialising unions. As byte updates aren't part the C language, dump-c must make sure such union initialisers using byte updates do not end up in the (re-)generated output. db4f25c was the first step in dump-c in this direction, and 3a24545 introduced further rules. The CSmith test generated with random seed 1612048908, however, demonstrated that dump-c's handling still didn't cover all cases of byte_update expressions introduced by the C front-end. This commit now handles those further cases. The regression test is a minified version of that CSmith test.
33fc7ee to
cfeb536
Compare
Thank you for calling this out! I have updated the text of the commit message (and also the pull request summary) to hopefully clarify what is going on here. |
martin-cs
left a comment
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.
Much clearer. Thank you!
The changes of a8592a7 made the C front-end introduce byte updates to
ensure compliance with the behaviour of popular compilers when
initialising unions. As byte updates aren't part the C language, dump-c
must make sure such union initialisers using byte updates do not end up
in the (re-)generated output.
db4f25c was the first step in dump-c in this direction, and 3a24545
introduced further rules. The CSmith test generated with random seed
1612048908, however, demonstrated that dump-c's handling still didn't
cover all cases of byte_update expressions introduced by the C
front-end. This commit now handles those further cases.
The regression test is a minified version of that CSmith test.