From c19cacfa34f52b65addeb7239d564b20e3cf2c61 Mon Sep 17 00:00:00 2001 From: martinboehme Date: Tue, 16 Jan 2024 12:52:55 +0100 Subject: [PATCH] [clang][dataflow] Tighten checking for existence of a function body. (#78163) In various places, we would previously call `FunctionDecl::hasBody()` (which checks whether any redeclaration of the function has a body, not necessarily the one on which `hasBody()` is being called). This is bug-prone, as a recent bug in Crubit's nullability checker has shown ([fix](https://github.com/google/crubit/commit/4b01ed0f14d953cda20f92d62256e7365d206b2e), [fix for the fix](https://github.com/google/crubit/commit/e0c5d8ddd7d647da483c2ae198ff91d131c12055)). Instead, we now use `FunctionDecl::doesThisDeclarationHaveABody()` which, as the name implies, checks whether the specific redeclaration it is being called on has a body. Alternatively, I considered being more lenient and "canonicalizing" to the `FunctionDecl` that has the body if the `FunctionDecl` being passed is a different redeclaration. However, this also risks hiding bugs: A caller might inadverently perform the analysis for all redeclarations of a function and end up duplicating work without realizing it. By accepting only the redeclaration that contains the body, we prevent this. I've checked, and all clients that I'm aware of do currently pass in the redeclaration that contains the function body. Typically this is because they use the `ast_matchers::hasBody()` matcher which, unlike `FunctionDecl::hasBody()`, only matches for the redeclaration containing the body. --- .../include/clang/Analysis/FlowSensitive/ControlFlowContext.h | 3 ++- .../clang/Analysis/FlowSensitive/DataflowEnvironment.h | 3 ++- clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp | 2 +- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp | 2 +- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | 4 ++-- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h b/clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h index 768387a121b9..405e93287a05 100644 --- a/clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h +++ b/clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h @@ -32,7 +32,8 @@ namespace dataflow { class ControlFlowContext { public: /// Builds a ControlFlowContext from a `FunctionDecl`. - /// `Func.hasBody()` must be true, and `Func.isTemplated()` must be false. + /// `Func.doesThisDeclarationHaveABody()` must be true, and + /// `Func.isTemplated()` must be false. static llvm::Expected build(const FunctionDecl &Func); /// Builds a ControlFlowContext from an AST node. `D` is the function in which diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index e8c27d6c1203..1543f900e401 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -172,7 +172,8 @@ public: /// /// Requirements: /// - /// The function must have a body. + /// The function must have a body, i.e. + /// `FunctionDecl::doesThisDecalarationHaveABody()` must be true. void initialize(); /// Returns a new environment that is a copy of this one. diff --git a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp index 56246066e4aa..c9ebffe6f378 100644 --- a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp @@ -69,7 +69,7 @@ static llvm::BitVector findReachableBlocks(const CFG &Cfg) { llvm::Expected ControlFlowContext::build(const FunctionDecl &Func) { - if (!Func.hasBody()) + if (!Func.doesThisDeclarationHaveABody()) return llvm::createStringError( std::make_error_code(std::errc::invalid_argument), "Cannot analyze function without a body"); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp index d95b3324ef6b..f4c4af022f51 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp @@ -298,7 +298,7 @@ DataflowAnalysisContext::getControlFlowContext(const FunctionDecl *F) { if (It != FunctionContexts.end()) return &It->second; - if (F->hasBody()) { + if (F->doesThisDeclarationHaveABody()) { auto CFCtx = ControlFlowContext::build(*F); // FIXME: Handle errors. assert(CFCtx); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 96fe6df88dbb..a50ee57a3c11 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -386,7 +386,7 @@ void Environment::initialize() { return; if (const auto *FuncDecl = dyn_cast(DeclCtx)) { - assert(FuncDecl->getBody() != nullptr); + assert(FuncDecl->doesThisDeclarationHaveABody()); initFieldsGlobalsAndFuncs(FuncDecl); @@ -426,7 +426,7 @@ void Environment::initialize() { // FIXME: Add support for resetting globals after function calls to enable // the implementation of sound analyses. void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { - assert(FuncDecl->getBody() != nullptr); + assert(FuncDecl->doesThisDeclarationHaveABody()); FieldSet Fields; llvm::DenseSet Vars;