Shared: Add and use a signature for basic blocks by aschackmull · Pull Request #20253 · github/codeql

aschackmull · 2025-08-20T09:39:53Z

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.

Copilot

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 CfgSig signature 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.}

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

     * 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) {


cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll

cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll

javascript/ql/lib/semmle/javascript/internal/BasicBlockInternal.qll

+  final private class FinalBasicBlock = BasicBlock;
+
+  module Cfg implements BB::CfgSig<DbLocation> {
+    private import javascript as Js


ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll

-deprecated private predicate lastRefSkipUncertainReadsExt(
-  Definition def, SsaInput::BasicBlock bb, int i
-) {
+deprecated private predicate lastRefSkipUncertainReadsExt(Definition def, BasicBlock bb, int i) {


ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll


  /** 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) {


hvitved

Overall LGTM, a few comments. Great to share this signature.

hvitved · 2025-08-29T11:10:23Z

shared/controlflow/codeql/controlflow/BasicBlock.qll

+  predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2);
+
+  /** Holds if `bb` is an entry basic block. */
+  predicate entryBlock(BasicBlock bb);


Why not a member predicate on BasicBlock?

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.

Then let's go for a sub class; I think we typically avoid top-level predicates.

hvitved · 2025-08-29T11:28:29Z

ruby/ql/lib/ruby.qll


 import codeql.ruby.AST as Ast
-import codeql.ruby.CFG as Cfg
+import codeql.ruby.CFG


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.

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

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?

Great, I prefer that (including a revert in ruby.qll).

hvitved · 2025-08-29T11:30:54Z

python/ql/lib/semmle/python/Flow.qll

+
+final private class FinalBasicBlock = BasicBlock;
+
+module Cfg implements BB::CfgSig<Location> {


I think should be moved into an internal file.

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.

Sorry, ignore my comment above.

Copilot AI review requested due to automatic review settings August 20, 2025 09:39

aschackmull requested review from a team as code owners August 20, 2025 09:39

aschackmull added the no-change-note-required This PR does not need a change note label Aug 20, 2025

aschackmull requested review from a team as code owners August 20, 2025 09:39

github-actions bot added C# JS C++ Java Python Ruby Rust Pull requests that update Rust code Swift DataFlow Library labels Aug 20, 2025

Copilot AI reviewed Aug 20, 2025

View reviewed changes

github-advanced-security bot found potential problems Aug 20, 2025

View reviewed changes

aschackmull force-pushed the shared/basicblock-signature2 branch 2 times, most recently from f8a1735 to ab8a1b4 Compare August 22, 2025 09:48

hvitved reviewed Aug 29, 2025

View reviewed changes

aschackmull added 6 commits September 1, 2025 11:19

BasicBlock: Add CFG signature.

119837b

SSA: Update input to use member predicates.

bb3abc8

SSA/VariableCapture: Use shared BasicBlock signature.

e53b22d

Languages: Adapt to api changes.

f459ddc

C++: Sync files.

a7b2a2f

Guards/Java: Use BasicBlock signature in Guards library.

3821f17

aschackmull added 3 commits September 1, 2025 11:26

C++: Extend final IRBlock

6ffb9b1

Ruby: Fix query compilation.

e2eb6db

BasicBlock: Replace entryBlock predicate with subclass.

09b2c5a

aschackmull force-pushed the shared/basicblock-signature2 branch from ab8a1b4 to 09b2c5a Compare September 1, 2025 09:49

hvitved approved these changes Sep 1, 2025

View reviewed changes

aschackmull merged commit 70a871c into github:main Sep 1, 2025
66 of 67 checks passed

aschackmull deleted the shared/basicblock-signature2 branch September 1, 2025 10:39

MathiasVP added a commit to microsoft/codeql that referenced this pull request Sep 10, 2025

PS: Fixup libraries after github#20253.

bce32b4


		final private class FinalBasicBlock = BasicBlock;

		module Cfg implements BB::CfgSig<Location> {

Conversation

aschackmull commented Aug 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants