-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Cleanup reporting of dead code #785
Conversation
| true, Some exp -> M.warn ~loc ~category:Deadcode ~tags:[CWE (if tv then 570 else 571)] "the %s branch %sover expression '%a' is dead" (str tv) (cilinserted loc.synthetic) d_exp exp | ||
| true, None -> M.warn ~loc ~category:Deadcode ~tags:[CWE (if tv then 570 else 571)] "an %s branch %sis dead" (str tv) (cilinserted loc.synthetic) |
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 loc.synthetic
is the appropriate thing to base this output on, because the location being synthetic doesn't necessarily correspond to an If
that CIL created. In some cases it does, e.g. in a && b
, when b
has a constant value, then there's a dead branch in that CIL-created If
and its location is synthetic because there's no program point in the original source code between the evaluation of a
and b
, which is why the location is synthetic.
I haven't checked with this PR, but I'm suspicious about cases like the following:
- Again in
a && b
, whena
has a constant value and there's a dead branch in that CIL-createdIf
, then there is a program point (right before the whole expression) where an invariant assert (which the synthetic locations are for) abouta
could be inserted. So that location needn't be synthetic, it might currently be since synthetic locations are probably assigned more crudely than necessary, but if that is ever improved, then the output about a dead branch would actually become more wrong. - When
for
loops are transformed by CIL, the branching node (for which the branching expression literally exists in the original source code!) has a synthetic location, because an (assert) statement cannot be inserted between thefor
initializer and condition.
Maybe it's something we should address separately in the future, for example by adding synthetic
fields to CIL statements (such that it's explicit which If
s were added by CIL) and variables (such that it's explicit which temporaries, etc it came up with) instead of semi-heuristics for these.
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.
Yes, that sounds like a good idea. However, I'd think that we should do this in an new issue & PR.
"description": "Print information about dead code", | ||
"type": "boolean", | ||
"default": false | ||
}, |
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.
Just as a post-merge note: the renaming of dbg.print_dead_code
and dbg.uncalled
will probably require a bunch of renaming in the bench repo as well.
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.
True!
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
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.
One last remark since this is supposed to close #94. That issue has an example program. We should check that this PR indeed fixes the problems on that example and possibly add it as a test with some // WARN
.
It does indeed solve the issues with that example program, but currently our regression test annotations can not currently express the needed properties, so we'll have to do without automatic tests for now. |
CHANGES: Goblint "lean" release after a lot of cleanup. * Remove unmaintained analyses: OSEK, ARINC, shapes, containment, deadlocksByRaces (goblint/analyzer#460, goblint/analyzer#736, goblint/analyzer#812, goblint/analyzer#474). * Add interactive analysis (goblint/analyzer#391). * Add server mode (goblint/analyzer#522). * Add Compilation Database support (goblint/analyzer#406, goblint/analyzer#448). * Add floating point domain, unrolled array domain and improved struct domains (goblint/analyzer#734, goblint/analyzer#761, goblint/analyzer#577, goblint/analyzer#419). * Add static loop unrolling and heap variable unrolling (goblint/analyzer#563, goblint/analyzer#722). * Improve race detection with may-happen-in-parallel analysis (goblint/analyzer#529, goblint/analyzer#518, goblint/analyzer#595). * Reimplement lockset and deadlock analyses (goblint/analyzer#659, goblint/analyzer#662, goblint/analyzer#650, goblint/analyzer#655). * Add pthread extraction to Promela (goblint/analyzer#220). * Add location spans to output (goblint/analyzer#428, goblint/analyzer#449). * Improve race reporting (goblint/analyzer#550, goblint/analyzer#551). * Improve dead code reporting (goblint/analyzer#94, goblint/analyzer#353, goblint/analyzer#785). * Refactor warnings (goblint/analyzer#55, goblint/analyzer#783). * Add JSON schema for configuration (goblint/analyzer#476, goblint/analyzer#499). * Refactor option names (goblint/analyzer#28, goblint/analyzer#192, goblint/analyzer#516, goblint/analyzer#675). * Add bash completion (goblint/analyzer#669). * Add OCaml 4.13 and 4.14 support, remove OCaml 4.09 support (goblint/analyzer#503, goblint/analyzer#672).
CHANGES: Goblint "lean" release after a lot of cleanup. * Remove unmaintained analyses: OSEK, ARINC, shapes, containment, deadlocksByRaces (goblint/analyzer#460, goblint/analyzer#736, goblint/analyzer#812, goblint/analyzer#474). * Add interactive analysis (goblint/analyzer#391). * Add server mode (goblint/analyzer#522). * Add Compilation Database support (goblint/analyzer#406, goblint/analyzer#448). * Add floating point domain, unrolled array domain and improved struct domains (goblint/analyzer#734, goblint/analyzer#761, goblint/analyzer#577, goblint/analyzer#419). * Add static loop unrolling and heap variable unrolling (goblint/analyzer#563, goblint/analyzer#722). * Improve race detection with may-happen-in-parallel analysis (goblint/analyzer#529, goblint/analyzer#518, goblint/analyzer#595). * Reimplement lockset and deadlock analyses (goblint/analyzer#659, goblint/analyzer#662, goblint/analyzer#650, goblint/analyzer#655). * Add pthread extraction to Promela (goblint/analyzer#220). * Add location spans to output (goblint/analyzer#428, goblint/analyzer#449). * Improve race reporting (goblint/analyzer#550, goblint/analyzer#551). * Improve dead code reporting (goblint/analyzer#94, goblint/analyzer#353, goblint/analyzer#785). * Refactor warnings (goblint/analyzer#55, goblint/analyzer#783). * Add JSON schema for configuration (goblint/analyzer#476, goblint/analyzer#499). * Refactor option names (goblint/analyzer#28, goblint/analyzer#192, goblint/analyzer#516, goblint/analyzer#675). * Add bash completion (goblint/analyzer#669). * Add OCaml 4.13 and 4.14 support, remove OCaml 4.09 support (goblint/analyzer#503, goblint/analyzer#672).
Automatic testing wasn't possible in #785 (comment), but is now with cram.
This addresses the shortcomings identified in #94:
synthetic
attribute of the location is set.alldeadcode
that also reports on dead code in branches over constants that are not taken. Include hint that this option exists whenprint_dead_code
is on, butalldeadcode
is off.TODO:
removeBranchingOnConstants
cil#103 to be mergedCloses #94.