Shared: Add and use a signature for basic blocks #20253
Conversation
aschackmull
added
the
no-change-note-required
github-actions
bot
added
C#
JS
C++
Java
Python
Ruby
Rust
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a canonical BasicBlock signature for the shared library and refactors existing SSA and dataflow implementations to use it instead of duplicating BasicBlock API definitions.
Key changes:
- Introduces a canonical
CfgSigsignature in the shared controlflow library - Refactors SSA implementations across multiple languages to use the shared signature
- Updates Variable Capture modules to use the standardized interface
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/ssa/codeql/ssa/Ssa.qll | Updates SSA implementation to use the new BasicBlock signature |
| shared/dataflow/codeql/dataflow/VariableCapture.qll | Refactors variable capture to use the shared BasicBlock signature |
| shared/controlflow/codeql/controlflow/BasicBlock.qll | Adds the canonical CfgSig signature interface |
| swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll | Updates Swift dataflow to use shared BasicBlock interface |
| swift/ql/lib/codeql/swift/dataflow/Ssa.qll | Refactors Swift SSA to use the new signature |
| swift/ql/lib/codeql/swift/controlflow/BasicBlocks.qll | Implements the CfgSig signature for Swift |
| Multiple language-specific files | Updates various language implementations to use the shared interface |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1953,7 +1951,7 @@ | |||
| * Holds if `(bb, i)` contains a write to an iterator that may have been obtained | |||
| * by calling `begin` (or related functions) on the variable `v`. | |||
| */ | |||
| predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) { | |||
| predicate variableWrite(IRCfg::BasicBlock bb, int i, SourceVariable v, boolean certain) { | |||
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| final private class FinalBasicBlock = BasicBlock; | ||
|
|
||
| module Cfg implements BB::CfgSig<DbLocation> { | ||
| private import javascript as Js |
Check warning
Code scanning / CodeQL
Names only differing by case Warning
| deprecated private predicate lastRefSkipUncertainReadsExt( | ||
| Definition def, SsaInput::BasicBlock bb, int i | ||
| ) { | ||
| deprecated private predicate lastRefSkipUncertainReadsExt(Definition def, BasicBlock bb, int i) { |
Check warning
Code scanning / CodeQL
Missing QLDoc for parameter Warning
| this.hasValueBranchEdge(bb1, bb2, branch) | ||
| } | ||
| } | ||
|
|
||
| /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ | ||
| predicate guardDirectlyControlsBlock(Guard guard, SsaInput::BasicBlock bb, GuardValue branch) { | ||
| predicate guardDirectlyControlsBlock(Guard guard, BasicBlock bb, GuardValue branch) { |
Check warning
Code scanning / CodeQL
Dead code Warning
aschackmull
force-pushed
the
shared/basicblock-signature2
branch
2 times, most recently
from
f8a1735 to
ab8a1b4
Compare
| predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2); | ||
|
|
||
| /** Holds if `bb` is an entry basic block. */ | ||
| predicate entryBlock(BasicBlock bb); |
There was a problem hiding this comment.
Why not a member predicate on BasicBlock?
There was a problem hiding this comment.
Partly historical inertia, I guess. But I think I'm also leaning slightly towards not having it as a member. We could do a subclass EntryBasicBlock, I guess - that's probably a bit more in line with what we generally have already. Of the three options, I think I like the member predicate the least, although admittedly it's a minor thing.
There was a problem hiding this comment.
Then let's go for a sub class; I think we typically avoid top-level predicates.
ruby/ql/lib/ruby.qll
Outdated
| @@ -3,5 +3,5 @@ | |||
| */ | |||
|
|
|||
| import codeql.ruby.AST as Ast | |||
| import codeql.ruby.CFG as Cfg | |||
| import codeql.ruby.CFG | |||
There was a problem hiding this comment.
I'm not a fan of bringing everything from codeql.ruby.CFG into the default namespace; Do you remember where things went wrong? I tried reverting this line and the changes to WeakSensitiveDataHashingCustomizations.qll, and with that the query compiles just fine.
There was a problem hiding this comment.
It goes wrong everywhere you do an unqualified CFG import like import codeql.ruby.CFG together with import ruby. But grepping only finds ruby/ql/src/queries/performance/DatabaseQueryInLoop.ql. I seem to recall other cases, but I can't remember the details, so I'll try running it through CI on a parallel draft PR: #20318
There was a problem hiding this comment.
Alright, I think I found most cases of this, so here's an alternative commit I could swap in: b548685
So you prefer that or something else?
There was a problem hiding this comment.
Great, I prefer that (including a revert in ruby.qll).
|
|
||
| final private class FinalBasicBlock = BasicBlock; | ||
|
|
||
| module Cfg implements BB::CfgSig<Location> { |
There was a problem hiding this comment.
I think should be moved into an internal file.
There was a problem hiding this comment.
Rhetorical question: Specifically for python or for all languages? I chose to put it at about the same scope as the existing BasicBlock class for all languages, since I figured we'd eventually want to reference it in a bunch of places that current reference BasicBlock. Eventually, I'm thinking that this module perhaps should become the main api for exposing basic blocks.
There was a problem hiding this comment.
Sorry, ignore my comment above.
aschackmull
force-pushed
the
shared/basicblock-signature2
branch
from
ab8a1b4 to
09b2c5a
Compare
We already have a fairly canonical BasicBlock api in the shared library. This PR takes this one step further by canonicalizing that api in a signature and using it in mainly SSA and a few other places.