Skip to content
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

Add option removeBranchingOnConstants #103

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

michael-schwarz
Copy link
Member

@michael-schwarz michael-schwarz commented Jul 15, 2022

This option can be used to disable the removal of the non-taken branches for branching over constants.

Enables analyzers to report such code as dead to the users. (c.f. goblint/analyzer#94)

@sim642
Copy link
Member

sim642 commented Jul 15, 2022

Unfortunately this constant dead code logic is not as localized, but there are at least four more places where it's done:

cil/src/frontc/cabs2cil.ml

Lines 4768 to 4770 in 7f428b6

CEExp (se1, e1') when isConstFalse e1' && canDrop se2 ->
finishExp (se1 @@ se3) (snd (castTo t3 tresult e3')) tresult
| CEExp (se1, e1') when isConstTrue e1' && canDrop se3 ->

cil/src/frontc/cabs2cil.ml

Lines 5084 to 5092 in 7f428b6

CEExp (se1, ((Const _) as ci1)), _ ->
if isConstTrue ci1 then
addChunkBeforeCE se1 ce2
else
(* se2 might contain labels so we cannot always drop it *)
if canDropCE ce2 then
ce1
else
CEAnd (ce1, ce2)

cil/src/frontc/cabs2cil.ml

Lines 5103 to 5111 in 7f428b6

CEExp (se1, (Const(CInt _) as ci1)), _ ->
if isConstFalse ci1 then
addChunkBeforeCE se1 ce2
else
(* se2 might contain labels so we cannot drop it *)
if canDropCE ce2 then
ce1
else
CEOr (ce1, ce2)

And I'm not sure if that even covers them all because there are so many different ways this is checked in different places: is_zero_cilint, isZero, isConstTrue, isConstFalse, etc.

@michael-schwarz
Copy link
Member Author

Unfortunately this constant dead code logic is not as localized, but there are at least four more places where it's done

As far as I can see, these cases are within expressions(!), and there is no real reason to disallow it for those, or is there?

@jerhard
Copy link
Member

jerhard commented Jul 15, 2022

Assuming we have code, such as

int truef(){
    return 1;
}

int main(){
    int x = 0 && truef();
    int y = 1 && truef();
    return x + y;
}

Goblint (on 924c219 with alldeadcode) will not warn that the call to truef in the first line of main will never happen. One might potentially be interested in such cases.

@sim642
Copy link
Member

sim642 commented Jul 16, 2022

As far as I can see, these cases are within expressions(!), and there is no real reason to disallow it for those, or is there?

It would make things fully consistent though. a && b is also an expression and in Goblint we do produce warnings about branches over a or b being dead if they happen to be constant. It would be weird if we suddenly didn't in the even simpler case where they are constant by literal, not by propagation.

And at least one of the linked snippets is about ?: which is explicitly about branching and which Goblint in non-constant-literal cases may produce dead branch warnings about.

So it's about dead branches, rather than dead lines.

@sim642 sim642 added this to the 2.0.0 milestone Jul 17, 2022
@michael-schwarz
Copy link
Member Author

I have now modified CIL to also respect removeBranchingOnConstants for expressions.
However, we still perform this simplification of expressions when they are used in a constant context (e.g. inside of definitions of enums etc.). If we also did not do it there, CIL will generate statements that have nowhere to go, and CIL will fail. This occurs e.g. frequently in Linux headers.

@michael-schwarz michael-schwarz requested a review from sim642 July 18, 2022 08:33
@michael-schwarz michael-schwarz merged commit 54b0c22 into develop Jul 18, 2022
@michael-schwarz michael-schwarz deleted the disable_if0_optimization branch July 18, 2022 15:21
sim642 added a commit to sim642/opam-repository that referenced this pull request Aug 12, 2022
CHANGES:

* Wrap library into `GoblintCil` module (goblint/cil#107).
* Remove all MSVC support (goblint/cil#52, goblint/cil#88).
* Port entire build process from configure/make to dune (goblint/cil#104).
* Add C11 `_Generic` support (goblint/cil#48).
* Add C11 `_Noreturn` support (goblint/cil#58).
* Add C11 `_Static_assert` support (goblint/cil#62).
* Add C11 `_Alignof` support (goblint/cil#66).
* Add C11 `_Alignas` support (goblint/cil#93, goblint/cil#108).
* Add partial C11 `_Atomic` support (goblint/cil#61).
* Add `_Float32`, `_Float64`, `_Float32x` and `_Float64x` type support (goblint/cil#8, goblint/cil#60).
* Add Universal Character Names, `char16_t` and `char32_t` type support (goblint/cil#80).
* Change locations to location spans and add additional expression locations (goblint/cil#51).
* Add synthetic marking for CIL-inserted statement locations (goblint/cil#98).
* Expose list of files from line control directives (goblint/cil#73).
* Add parsed location transformation hook (goblint/cil#89).
* Use Zarith for integer constants (goblint/cil#47, goblint/cil#53).
* Fix constant folding overflows (goblint/cil#59).
* Add option to disable constant branch removal (goblint/cil#103).
* Add standalone expression parsing and checking (goblint/cil#97, goblint/cil#96).
* Improve inline function merging (goblint/cil#72, goblint/cil#85, goblint/cil#84, goblint/cil#86).
* Fix some attribute parsing cases (goblint/cil#71, goblint/cil#75, goblint/cil#76, goblint/cil#77).
* Fix global NaN initializers (goblint/cil#78, goblint/cil#79).
* Fix `cilly` binary installation (goblint/cil#99, goblint/cil#100, goblint/cil#102).
* Remove batteries dependency to support OCaml 5 (goblint/cil#106).
sim642 added a commit to sim642/opam-repository that referenced this pull request Aug 12, 2022
CHANGES:

* Wrap library into `GoblintCil` module (goblint/cil#107).
* Remove all MSVC support (goblint/cil#52, goblint/cil#88).
* Port entire build process from configure/make to dune (goblint/cil#104).
* Add C11 `_Generic` support (goblint/cil#48).
* Add C11 `_Noreturn` support (goblint/cil#58).
* Add C11 `_Static_assert` support (goblint/cil#62).
* Add C11 `_Alignof` support (goblint/cil#66).
* Add C11 `_Alignas` support (goblint/cil#93, goblint/cil#108).
* Add partial C11 `_Atomic` support (goblint/cil#61).
* Add `_Float32`, `_Float64`, `_Float32x` and `_Float64x` type support (goblint/cil#8, goblint/cil#60).
* Add Universal Character Names, `char16_t` and `char32_t` type support (goblint/cil#80).
* Change locations to location spans and add additional expression locations (goblint/cil#51).
* Add synthetic marking for CIL-inserted statement locations (goblint/cil#98).
* Expose list of files from line control directives (goblint/cil#73).
* Add parsed location transformation hook (goblint/cil#89).
* Use Zarith for integer constants (goblint/cil#47, goblint/cil#53).
* Fix constant folding overflows (goblint/cil#59).
* Add option to disable constant branch removal (goblint/cil#103).
* Add standalone expression parsing and checking (goblint/cil#97, goblint/cil#96).
* Improve inline function merging (goblint/cil#72, goblint/cil#85, goblint/cil#84, goblint/cil#86).
* Fix some attribute parsing cases (goblint/cil#71, goblint/cil#75, goblint/cil#76, goblint/cil#77).
* Fix global NaN initializers (goblint/cil#78, goblint/cil#79).
* Fix `cilly` binary installation (goblint/cil#99, goblint/cil#100, goblint/cil#102).
* Remove batteries dependency to support OCaml 5 (goblint/cil#106).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants