Go: migrate control flow graph to shared CFG library#21614
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@copilot Please fix the following errors from the compiler: |
Fixed in 1db6bac:
|
|
@copilot Unlike java, go has AST nodes which aren't in a function definition, so we have to allow |
Fixed in c0a59ad. |
|
@copilot Please fix these warnings, as long as they are in go code and not shared libraries. |
Fixed in 66a191d. Added QLdoc comments to all 96 items in IR.qll — classes, member predicates, newtype branches, and classless predicates — matching the documentation from the old code where applicable. |
|
@copilot Many tests are failing. Please investigate and in each case see if the test needs to be changed or the CFG code needs to be updated. |
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Fix the following errors from CI: ERROR: module Ast does not declare GotoStmt, required by implemented signature ControlFlowGraph::AstSigLocations::Location (/home/runner/work/codeql/codeql/go/ql/lib/semmle/go/controlflow/ControlFlowGraphShared.qll:34,33-61) |
Fixed in 2aaf88b. The shared CFG library on |
owen-mc
force-pushed
the
copilot/vscode-mndb17m3-9fpv
branch
from
2aaf88b to
39697ee
Compare
Add opt-in InputSig2 predicates to support a function-exit epilogue whose placement depends on reachability (such as Go's deferred calls, run at exit in last-in-first-out order): - deferExitStep: language-provided exit-epilogue edges, wired into explicitStep but excluded from the defer-free reachability. - overridesCallableBodyExit: suppresses the default fall-through edge from a body's "after" node to the normal exit, so the epilogue can be interposed on fall-through paths. To let a language compute the reachability gate for those edges without observing them (and without a non-monotonic cycle through reachable): - explicitStep is split into explicitStepCommon (defer-free) plus deferExitStep, and defaultCfg now negates explicitStepCommon so default evaluation does not depend on deferExitStep. - succIgnoringDeferExit exposes the defer-free successor relation, typed over PreControlFlowNode so it does not depend on reachable. - getASuccessorIgnoringDeferredExit exposes the same relation as a ControlFlowNode member for general use. - isInOrderNode exposes a structural, reachability-free node-identity test for use inside the negations a language needs when computing its gate. - EntryNodeImpl is no longer private, so a language can identify the entry node over PreControlFlowNode. All InputSig2 additions default to none(), leaving other languages unchanged.
Model `defer`ed calls so the call runs at function exit rather than inline at the `defer` statement, reproducing the previous control-flow semantics: - Add a per-defer "defer-invoke" node for the deferred call. - deferExitStep wires normal-exit predecessors (return nodes and body fall-through) through the active deferred-call invocations in last-in-first-out order, then on to the normal exit target (the result-read epilogue for named results, or the normal exit node). - The chain is reachability-gated using the defer-free successor relation (succIgnoringDeferExit / isInOrderNode), so only deferred calls that were actually registered on a path are run on that path. - overridesCallableBodyExit / overridesCallableEndAbruptCompletion suppress the default body-exit and return routing for functions containing `defer`, so the epilogue is interposed instead.
…h via fallsThrough
This is now represented by `SendStmt` `isIn` node
This occurs as a direct consequence of the CFG migration. IR::EvalInstruction now deliberately creates a value-producing instruction for a NotExpr (or LogicalBinaryExpr) from its after-node, but only when the expression is not in a boolean conditional contex. !d here sits in a const initializer, not an if/for/switch condition, so isInBooleanCondContext is false and the after-node becomes an EvalInstruction — surfacing as the After !... data-flow node. That node reports getBoolValue() = true and isPlatformIndependentConstant(), so GVN can match it to the true literal. On main, a NotExpr in this position produced no such value node (the old hand-written CFG split the ! evaluation across branch outcomes rather than exposing a single combined value node), so there was no DataFlow::Node for !d to be numbered, and hence no row. The new design intentionally re-introduces a single combined value node in non-conditional contexts, which is exactly what makes this (correct) GVN equivalence visible. So: the line is right, and it's expected fallout from the migration's handling of !/&&/|| value nodes.
The test shouldn't have an alert because the panic happens before the deferred call to recover.
- An empty select statement is correctly identified as a dead end. - Select statements have multiple successors. - Return statements have multiple successors when there are conditionally called defer statements.
Category 1 — GVN named-result zero-init: Named result variables now get an explicit result-zero-init:0 node (the migrated CFG models result vars as zero-initialized in the prologue). Test passes; intended modeling. Category 2 — ParenExpr (...) missingInNodeForPostOrInOrder. postOrInOrder starts with n instanceof Go::ReferenceExpr, and ReferenceExpr includes the this.(ParenExpr).getExpr() instanceof ReferenceExpr disjunct, so parenthesized reference expressions are correctly treated pre-order and excluded. Accepted. Category 3 — select multipleSuccessors/deadEnd: Same pattern already accepted in the committed ControlFlowGraph consistency file. Category 4 — defer-invoke / after-block multipleSuccessors: A return/after-block node flows both to Normal Exit and into the deferred-call chain. Already accepted in committed ControlFlowGraph and experimental/InconsistentCode.
owen-mc
force-pushed
the
copilot/vscode-mndb17m3-9fpv
branch
from
7c52413 to
9f1a48f
Compare
Adopts the shared
codeql/controlflowlibrary for Go's CFG implementation, replacing the bespokeControlFlowGraphImpl.qll(2133 lines) with a shared-library instantiation.Core architecture changes
ControlFlowGraphShared.qll(new): InstantiatesCfgLib::Make0/Make1/Make2with Go-specificAstandInputmodules. Wraps everything inGoCfgto isolate from the old API during transition.ControlFlowGraph.qll:ControlFlow::Nodenow extendsGoCfg::ControlFlowNodeinstead of the oldTControlFlowNodenewtype.BasicBlocks.qll:BasicBlockaliased toGoCfg::Cfg::BasicBlock.IR.qll: All 30+ instruction classes rewritten to identify themselves viaisAdditional/isIn/isBefore/isAfterpredicates instead ofMk*newtype constructors.ControlFlowGraphImpl.qll: fully superseded.File-level CFG roots
Unlike Java, Go has AST nodes (e.g. top-level
var,const,typedeclarations) that are not inside any function definition. To preserve CFG coverage for these nodes,Callableincludes bothFuncDefs with bodies andFiles with declarations.getEnclosingCallablefalls back tonode.getFile()for nodes not inside any function, andControlFlow::RootincludesFiles with declarations — matching the behavior of the oldControlFlowGraphImpl.qll.Function prologue/epilogue routing
Changed
callableGetBody(c) = c(instead ofc.getBody()) so the shared library routesEntry(fd) → Before(fd), allowingfuncDefStepto insert prologue and epilogue nodes:Before(fd)→arg:i→param-init:i(per parameter) →result-zero-init:j→result-init:j(per result var) →Before(body)After(body)→result-read:j(per result var) →After(fd)→NormalExitThis makes
InitParameterInstructionand related classes reachable in the CFG, which is required for SSA and data flow (e.g.,SsaParameterNodeviaIR::initParamInstruction).Implicit promoted-field reads
Fixed
selectorExprStepto route throughimplicit-field:iadditional nodes for promoted field accesses, makingImplicitFieldReadInstructionreachable. The chain descends from the outermost embedded field (highest index) to the innermost (index 1) before reachingIn(sel):Minor fixes
SsaImpl.qll: castbb.getNode(i)toControlFlow::Nodebefore callingisEntryNode().Revel.qll: usegetBaseInstruction()instead of the removedgetBase()onImplicitFieldReadInstruction.Original prompt
Continue
[Chronological Review: The conversation began with the user requesting a list of commits merged as part of a specific commit hash (e695477). Following that, the user asked to adopt a new shared CFG library for Go based on those commits. The user then prompted to "Continue to iterate?" multiple times, indicating a desire to progress with the implementation. The user also requested to "Do phase 2 now," suggesting a structured approach to the work. Throughout the conversation, there were indications of changes made to specific files, which required checking the current contents before proceeding with edits.][Intent Mapping:
[Technical Inventory:
[Code Archaeology:
[Progress Assessment:
[Context Validation: All critical information for continuation is captured, including the need to check file contents before making edits and the structured approach to the project.]
[Recent Commands Analysis:
1. Conversation Overview: - Primary Objectives: - "List commits merged as part of e695477" - Understanding changes related to a specific commit. - "Using those commits as a guideline, adopt the new shared CFG library for go." - Implementing a new library based on previous changes. - "Continue to iterate?" - Seeking confirmation to proceed with ongoing work. - "Do phase 2 now" - Moving to the next phase of the project. - Session Context: The conversation has focused on implementing changes to the Go control flow library, with multiple requests to continue iterating on the work. - User Intent Evolution: The user has consistently sought to progress through the implementation phases, indicating a structured approach to the project.- Technical Foundation:
- GitHub CodeQL: A tool for code analysis being used to facilitate the implementation of changes.
- Files:
- IR.qll: The current working file for implementing changes.
- ControlFlowGraph.qll and ControlFlowGraphShared.qll: Files mentioned as having recent edits that need to be checked.
- Codebase Status:
- IR.qll:
- Purpose: Central file for implementing the new CFG library.
- Current State: User is actively working on this file.
- Key Code Segments: Specific functions or classes were not detailed in the conversation.
- Dependencies: Relates to the overall control flow implementation in Go.
- ControlFlowGraph.qll and ControlFlowGraphShared.qll:
- Purpose: Supporting files for the control flow library.
- Current State: Recent edits made, requiring review before further changes.
- Problem Resolution:
- Issues Encountered: Need to check file contents before making edits due to recent changes.
- Solutions Implemented: User has been iterating through the implementation process.
- Debugging Context: No specific debugging issues were mentioned, but the user is cautious about recent changes.
- Lessons Learned: Importance of reviewing file changes before proceeding with edits.
- Progress Tracking:
- Completed Tasks: User has identified the need for ...
Created from VS Code.