fixes the free-vars-by-dominators computation algorithm #907
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This algorithm kicks in in the optimization-level=3 and ... breaks everything. The bug was trivial, the kill set was computed before the live set, hence a block was defining its own variables before it was used. This bug affects only 1.5 version with optimization-level=3. Note, there is still a subtle problem with this algorithm that comes from the "Everything Dominates Unreachable Block" lemma in the dominators algorithm. And altough this theorem is valid wrt to the graph theoretic definition of unreachable, which indeed implies that a block is not reachable under no circumstances, in the binary analysis an unreachable block is just a block that might be reachable we just don't know by which paths. In other words, we have an under-approximation of a control flow graph, and the unsoundness of the graph is propagated to the unsoundness of the dominator tree computation. That lies more or less in the vein with the idea of the optimization-level=3 which is as as unsound as the underlying graph.
ivg
added a commit
to ivg/bap
that referenced
this pull request
Dec 11, 2018
note: this PR shall be merged after BinaryAnalysisPlatform#907 is merged
gitoleg
approved these changes
Dec 11, 2018
gitoleg
pushed a commit
that referenced
this pull request
Dec 11, 2018
* maked dead code elimination less conservative As was noted in #905 the optimization passes sometimes misses an opportunity to remove obvious assignments. The underlying reason is that we compute a very conservative set of variables which are used for interprocedural communication. And if a variable is a member of this set then we do not delete it (under assumption, that it could be used in other subroutines). What was overconservative is that all versions of the same variable were protected, while only the last definition one could be used for interprocedural communication. The propsed change relaxes the protected set, and removes only the last version of a variable from the set of the dead variables. Fixes #905 * makes it sound Unfortunately, the original proposal was unsound, as the assumption that only the last definition should be preserved is implied by the assumption that only the last definition is used for interprocedural communication, which is totally wrong. The liveness property is propagated from each call (and indirect jump) to all members of the protected set. We're using the SSA form for def-use analysis which is inherently intraprocedural, so to preserve the liveness of the variables with the interprocedural scope we use the protected set as an overapproximation. A tighter approximation would be to reinstantiate their liveness after each call or indirect jump. However, this would require tampering with the ssa algorithm, that we don't want to do right now. The updated solution reinstantiates liveness of protected variables at the end of each block, even the block is not a call or even jump. The liveness is killed by the first use of protection, so it is propagated only to the last (if any) definition. * removes a dead function note: this PR shall be merged after #907 is merged * drops debug info * drops a change that is irrelevant to the proposal to make it cleaner
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This algorithm kicks in in the optimization-level=3 and ... breaks
everything. The bug was trivial, the kill set was computed before
the live set, hence a block was defining its own variables before it
was used.
This bug affects only 1.5 version with optimization-level=3.
Note, there is still a subtle problem with this algorithm that comes
from the "Everything Dominates Unreachable Block" lemma in the
dominators algorithm. And altough this theorem is valid wrt to the
graph theoretic definition of unreachable, which indeed implies that a
block is not reachable under no circumstances, in the binary analysis
an unreachable block is just a block that might be reachable we just
don't know by which paths. In other words, we have an
under-approximation of a control flow graph, and the unsoundness of
the graph is propagated to the unsoundness of the dominator tree
computation. That lies more or less in the vein with the idea of the
optimization-level=3 which is as as unsound as the underlying graph.