Add shared basic block library

[paldepind]

Adds a shared basic block library in the controlflow pack 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.BasicBlock or through the existing codeql.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 instantiating codeql.controlflow.BasicBlock for Go and that seems to work fine as well.

• The BasicBlock class for C# now has both the current getASuccessorByType/1 method and a new getASuccessor/1 method that it inherits from the basic block library and which is the name used in Ruby, Rust, and Swift. We could consider deprecating getASuccessorByType/1 in order to not have two methods doing the same and to increase consistency between language libraries.

• For the BasicBlock subclasses in Cfg.qll I've changed the current names a bit, such that they are all consistently of the form ${name}BasicBlock. For instance JoinBlock is instead JoinBasicBlock. For the existing language-level basic block libraries I've kept the current names for backwards compatibility, so only Rust use the new names.

[hvitved]

• We could consider deprecating getASuccessorByType/1 in order to not have two methods doing the same and to increase consistency between language libraries.

I think we should do that.

[hvitved]

Very nice work, great to see even more shared code 🎉

[paldepind]

Thanks for the review and comments. I've addressed all but one.

[paldepind]

• We could consider deprecating getASuccessorByType/1 in order to not have two methods doing the same and to increase consistency between language libraries.

I think we should do that.

I'll do that in a follow up PR 👍

[geoffw0]

Swift changes LGTM. I'd like to see DCA runs at some point before this is merged.

[aschackmull]

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:

Suggested change

	* 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.

[paldepind]

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 stmt in 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.

[aschackmull]

Sorry, it should have been if expression, 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.

[paldepind]

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 👍

[geoffw0]

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)

[paldepind]

@geoffw0

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.

[hvitved]

I have started DCA runs for the relevant languages.

[hvitved]

DCA looks good 🎉

[aschackmull]

Maybe just replace if expr with a name like e.g. A in the ascii graph to identify the subsequent basic block?

Suggested change

	// `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.

[paldepind]

Thanks for running the remaining DCAs @hvitved.

[geoffw0]

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'm satisfied with this + lack of wobble for languages where we get better data. 👍

[paldepind]

Thanks for reviewing. Really nice that we also got some performance improvements in, to the benefit of the languages that now use the library 🎉