Shared: Add and use a signature for basic blocks #20253
Conversation
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.
| @@ -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
5230eb5 to
f8a1735
Compare
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?
| @@ -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
|
|
||
| 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.
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.