[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](4b01ed0f14),
[fix for the
fix](e0c5d8ddd7)).

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.
This commit is contained in:
martinboehme 2024-01-16 12:52:55 +01:00 committed by GitHub
parent 032c832719
commit c19cacfa34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 8 additions and 6 deletions

View File

@ -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<ControlFlowContext> build(const FunctionDecl &Func);
/// Builds a ControlFlowContext from an AST node. `D` is the function in which

View File

@ -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.

View File

@ -69,7 +69,7 @@ static llvm::BitVector findReachableBlocks(const CFG &Cfg) {
llvm::Expected<ControlFlowContext>
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");

View File

@ -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);

View File

@ -386,7 +386,7 @@ void Environment::initialize() {
return;
if (const auto *FuncDecl = dyn_cast<FunctionDecl>(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<const VarDecl *> Vars;