Add shared basic block library #18497
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
shared-basic-block-library
branch
from
1c9e7cc to
5bfeff6
Compare
paldepind
force-pushed
the
shared-basic-block-library
branch
5 times, most recently
from
7112872 to
b313a48
Compare
paldepind
force-pushed
the
shared-basic-block-library
branch
from
b313a48 to
8b20b0d
Compare
I think we should do that. |
|
Thanks for the review and comments. I've addressed all but one. |
I'll do that in a follow up PR 👍 |
| predicate immediatelyControls(BasicBlock succ, SuccessorType s) { | ||
| succ = this.getASuccessor(s) and | ||
| forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != this | | ||
| succ.dominates(pred) | ||
| ) | ||
| } |
There was a problem hiding this comment.
In Java, this predicate is called dominatingEdge and it includes bbIDominates(this, succ) as an additional conjunct. I think that ought to be semantically identical, but it might offer a better join-order since immediate dominance joined with the successor relation yields a much stronger context for the forall.
Also, in this general library, it's probably worth it to expand the qldoc a bit on the relationship between the concept of controls and dominance. In short: controls equals edge dominance. See also the qldoc for dominatingEdge in Java.
There was a problem hiding this comment.
Also, the concept of dominating edge aka immediatelyControls might very well be applicable beyond ConditionBasicBlocks, so it should possibly be available as a top-level predicate between basic blocks.
There was a problem hiding this comment.
In particular, this concept can be put into BasicBlocks.qll as it's independent of .isCondition().
There was a problem hiding this comment.
I think that ought to be semantically identical
That seems true to me.
it might offer a better join-order since immediate dominance joined with the successor relation yields a much stronger context for the
forall.
How can I validate that? I tried looking at the evaluator logs for the predicate with and without that conjunct and didn't see any noticeable difference.
There was a problem hiding this comment.
Also, in this general library, it's probably worth it to expand the qldoc a bit on the relationship between the concept of controls and dominance.
I've written some stuff now for the immediatelyControls predicate. Let me know if there's more I should add.
There was a problem hiding this comment.
How can I validate that? I tried looking at the evaluator logs for the predicate with and without that conjunct and didn't see any noticeable difference
Look at the RA with tuple counts and compare the two versions.
There was a problem hiding this comment.
Thanks, including the extra conjunct did indeed produce fewer tuples during the calculation 👍
paldepind
force-pushed
the
shared-basic-block-library
branch
from
468707e to
62a459d
Compare
| * The above implies that this block immediately dominates `succ`. But | ||
| * "controls" is a stronger notion than "dominates". It is not the case | ||
| * that any immediate successor that is immediately dominated by this block | ||
| * is also immediately controlled by this block. To see why, consider this | ||
| * example corresponding to an `if`-statement without an `else` block: | ||
| * ``` | ||
| * ... --> cond --[true]--> ... --> if stmt | ||
| * \ / | ||
| * ----[false]----------- | ||
| * ``` | ||
| * The basic block for `cond` immediately dominates the immediately | ||
| * succeeding basic block for the `if` statement. But the `if` statement | ||
| * is not immediately controlled by the `cond` basic block and the `false` | ||
| * edge since it is also possible to reach the `if` statement via a path | ||
| * through the `true` edge. |
There was a problem hiding this comment.
I found this hard to read and also missing some main points (also, the placement of if stmt in that graph is non-sensical to me). How about this:
| * The above implies that this block immediately dominates `succ`. But | |
| * "controls" is a stronger notion than "dominates". It is not the case | |
| * that any immediate successor that is immediately dominated by this block | |
| * is also immediately controlled by this block. To see why, consider this | |
| * example corresponding to an `if`-statement without an `else` block: | |
| * ``` | |
| * ... --> cond --[true]--> ... --> if stmt | |
| * \ / | |
| * ----[false]----------- | |
| * ``` | |
| * The basic block for `cond` immediately dominates the immediately | |
| * succeeding basic block for the `if` statement. But the `if` statement | |
| * is not immediately controlled by the `cond` basic block and the `false` | |
| * edge since it is also possible to reach the `if` statement via a path | |
| * through the `true` edge. | |
| * The concept of an edge `E` controlling a node `N` in a graph can also be | |
| * described as _edge dominance_ in the sense that if `E` was split in two | |
| * with an added node in the middle then "controlled by `E`" would be | |
| * equivalent to dominance by that added node. | |
| * Note that controls/edge-dominance is stronger than node dominance as | |
| * it implies node dominance (by either endpoint), but the converse is not | |
| * true, hence the need for this concept. |
There was a problem hiding this comment.
I feel that this explains the notion of "control" by introducing a synonym for it, "edge dominance" and explaining the synonym instead. Then afterwards we have to write "controls/edge-dominance" because there is now two names for the same thing.
Instead, I've now tried to spell out the analogy/relation between dominance and control but without introducing another term helps (it's not standard nor used elsewhere in the library).
-
the placement of if
stmtin that graph is non-sensical to me
Sorry, it should have been if expression, not statement. I've fixed the example and moved it inside the body as internal documentation.
Also, as the "immediately" in immediatelyControls is very different from the "immediately" in immediatelyDominates, I've moved the controls vs. dominates explanation to the controls predicate which corresponds more nicely with the dominates/strictlyDominates predicates.
There was a problem hiding this comment.
Sorry, it should have been
ifexpression, not statement.
That's still doesn't make sense to me - if anything, it's even more confusing. I don't know what an "if expression" is, if anything, I would guess it to be the ternary operator ? : known from e.g. Java/C++/C# etc. But that cannot exist "without an else branch". I expect the false-successor of a condition in an if statement without an else branch to be whatever statement that follows the entire if statement chunk.
There was a problem hiding this comment.
Expression oriented languages (without if statements) often have an if expression where the else branch is optional. For instance, Rust, Ruby, and OCaml. In Rust if cond { ... } would give rise to the graph in the example.
But let's change it to an if statement as in your suggestion. I've done that 👍
|
Swift DCA results look good but there's a 16.2% analysis time regression - which sounds like a lot, but we get a lot of wobble for metrics like this on the Swift DCA job (e.g. a 17.8% slowdown wobble a month ago). I did another run for another data point and got 13.4% slowdown (and a random failure), which is a little better but far from reassuring. The C# run is showing no such pattern. Can you say anything about the expected performance impact of this work? (I'm suspicious my concern amounts to nothing) |
The expectation is that performance should be the same or improved. Most of the code is identical to the previous code for Swift and in the cases where it is not it should be better performing. |
|
I have started DCA runs for the relevant languages. |
| // `if` expression without an `else` block: | ||
| // ``` | ||
| // ... --> cond --[true]--> ... --> if expr | ||
| // \ / | ||
| // ----[false]----------- | ||
| // ``` | ||
| // The basic block for `cond` immediately dominates the directly | ||
| // succeeding basic block for the `if` expression. But the `if` | ||
| // expression is not immediately controlled by the `cond` basic block and | ||
| // the `false` edge since it is also possible to reach the `if` | ||
| // expression via the `true` edge. |
There was a problem hiding this comment.
Maybe just replace if expr with a name like e.g. A in the ascii graph to identify the subsequent basic block?
| // `if` expression without an `else` block: | |
| // ``` | |
| // ... --> cond --[true]--> ... --> if expr | |
| // \ / | |
| // ----[false]----------- | |
| // ``` | |
| // The basic block for `cond` immediately dominates the directly | |
| // succeeding basic block for the `if` expression. But the `if` | |
| // expression is not immediately controlled by the `cond` basic block and | |
| // the `false` edge since it is also possible to reach the `if` | |
| // expression via the `true` edge. | |
| // `if` statement without an `else` block: | |
| // ``` | |
| // ... --> cond --[true]--> ... --> A | |
| // \ / | |
| // ----[false]----------- | |
| // ``` | |
| // The basic block for `cond` immediately dominates the basic block `A` | |
| // that follows the `if` statement. But `A` | |
| // is not controlled by the `cond` basic block and | |
| // the `false` edge since it is also possible to reach `A` | |
| // via the `true` edge. |
|
Thanks for running the remaining DCAs @hvitved. |
I'm satisfied with this + lack of wobble for languages where we get better data. 👍 |
|
Thanks for reviewing. Really nice that we also got some performance improvements in, to the benefit of the languages that now use the library 🎉 |
Adds a shared basic block library in the
controlflowpack and modifies the basic block implementation of Swift, Ruby, C#, and Rust to use it.A few notes:
There's two ways to use the basic block library. Either through the new
codeql.controlflow.BasicBlockor through the existingcodeql.controlflow.Cfg. The former is suitable for languages that don't use the control graph library and the latter for those that do. In this PR all languages use the latter, but I've tried instantiatingcodeql.controlflow.BasicBlockfor Go and that seems to work fine as well.The
BasicBlockclass for C# now has both the currentgetASuccessorByType/1method and a newgetASuccessor/1method that it inherits from the basic block library and which is the name used in Ruby, Rust, and Swift. We could consider deprecatinggetASuccessorByType/1in order to not have two methods doing the same and to increase consistency between language libraries.For the
BasicBlocksubclasses inCfg.qllI've changed the current names a bit, such that they are all consistently of the form${name}BasicBlock. For instanceJoinBlockis insteadJoinBasicBlock. For the existing language-level basic block libraries I've kept the current names for backwards compatibility, so only Rust use the new names.