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
Agent-Logs-Url: https://github.com/github/codeql/sessions/011f8465-5ad3-44e2-9d84-f5314ea1afb2 Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
…In guards, mark implicitFieldSelection as additional Agent-Logs-Url: https://github.com/github/codeql/sessions/8d96128c-44db-4ba1-9eca-1a5b37f54442 Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/codeql/sessions/2867f173-c3f1-4564-9885-3ff4517bbf30 Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
… branches Agent-Logs-Url: https://github.com/github/codeql/sessions/12c0f9c8-1b85-4db4-b26f-8dbce818aa5a Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/codeql/sessions/3f96ead2-cda4-479c-9e37-f38ace035870 Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
… Ast module for updated shared CFG AstSig Agent-Logs-Url: https://github.com/github/codeql/sessions/005953d9-1d54-4071-8cd2-5a7d26f65f61 Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
owen-mc
force-pushed
the
copilot/vscode-mndb17m3-9fpv
branch
from
39697ee to
061a460
Compare
To determine that test9 can't return normally, you have to use the fact that test5 can't return normally. This would make CFG construction recursive, which would be bad for performance. Therefore we accept the limitation that we cannot detect that test9 can't return normally, and we change the test output.
There was a loop in the definitions which meant nothing ever got made
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.