[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
Dana Jansens via cfe-commits
cfe-commits at lists.llvm.org
Fri May 31 10:24:01 PDT 2024
https://github.com/danakj updated https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/2] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 ++++++++++++------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++++++++++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+ const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
- if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
- for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
- EmitSuggestions = false;
- break;
- }
- }
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation and consider it done. No need to deal
@@ -3106,8 +3076,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
if (WarningGadgets.empty())
return;
- UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
- FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
+ WarningGadgetSets UnsafeOps =
+ groupWarningGadgetsByVar(std::move(WarningGadgets));
+ FixableGadgetSets FixablesForAllVars =
+ groupFixablesByVar(std::move(FixableGadgets));
std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector<Stmt *> Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
+ if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+ for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+ EmitSuggestions = false;
+ break;
+ }
+ }
+
+ Stmts.push_back(FD->getBody());
+
+ if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+ Stmts.push_back(CI->getInit());
+ }
+ }
+ }
+
+ if (const auto *FD = dyn_cast<FieldDecl>(D)) {
+ // Visit in-class initializers for fields.
+ if (!FD->hasInClassInitializer())
+ return;
+
+ Stmts.push_back(FD->getInClassInitializer());
+ }
+
+ if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
+ Stmts.push_back(D->getBody());
+ }
+
+ assert(!Stmts.empty());
+
+ for (Stmt *S : Stmts) {
+ auto [FixableGadgets, WarningGadgets, Tracker] =
+ findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
+ applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
+ std::move(Tracker), Handler, EmitSuggestions);
+ }
+}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index b9d0b59ef1db7..ceedeffd20763 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2471,6 +2471,14 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> {
CallableVisitor(llvm::function_ref<void(const Decl *)> Callback)
: Callback(Callback) {}
+ bool VisitFieldDecl(FieldDecl *Node) {
+ if (cast<DeclContext>(Node->getParent())->isDependentContext())
+ return true; // Not to analyze dependent decl
+ if (Node->hasInClassInitializer())
+ Callback(Node);
+ return true;
+ }
+
bool VisitFunctionDecl(FunctionDecl *Node) {
if (cast<DeclContext>(Node)->isDependentContext())
return true; // Not to analyze dependent decl
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
index bfc34b55c1f66..a0efecefe1c89 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -111,6 +111,42 @@ int testFoldExpression(Vs&&... v) {
return (... + v); // expected-warning{{function introduces unsafe buffer manipulation}}
}
+struct HoldsUnsafeMembers {
+ HoldsUnsafeMembers()
+ : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ [[clang::unsafe_buffer_usage]]
+ HoldsUnsafeMembers(int i)
+ : FromCtor(i), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{i} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ HoldsUnsafeMembers(float f)
+ : HoldsUnsafeMembers(0) {} // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ UnsafeMembers FromCtor;
+ UnsafeMembers FromCtor2;
+ UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
+
+struct SubclassUnsafeMembers : public UnsafeMembers {
+ SubclassUnsafeMembers()
+ : UnsafeMembers(3) // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ [[clang::unsafe_buffer_usage]]
+ SubclassUnsafeMembers(int i)
+ : UnsafeMembers(i) // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+};
+
+struct AggregateUnsafeMembers {
+ UnsafeMembers f1;
+ UnsafeMembers f2{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
+
// https://github.com/llvm/llvm-project/issues/80482
void testClassMembers() {
UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
@@ -122,4 +158,35 @@ void testClassMembers() {
UnsafeMembers()(); // expected-warning{{function introduces unsafe buffer manipulation}}
testFoldExpression(UnsafeMembers(), UnsafeMembers());
+
+ HoldsUnsafeMembers();
+ HoldsUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ SubclassUnsafeMembers();
+ SubclassUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ (void)AggregateUnsafeMembers{
+ .f1 = UnsafeMembers(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ };
+
+ (void)AggregateUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ // FIXME: This should generate a warning but InitListExpr construction is
+ // treated as calling an implicit constructor, while ParentListInitExpr (the
+ // one above) is not. So `MatchDescendantVisitor::shouldVisitImplicitCode()`
+ // must return true for this to generate a warning. However that moves the
+ // SourceLocation of warnings from field initializers that construct through
+ // InitListExpr, preventing us from matching warnings against them.
+ (void)AggregateUnsafeMembers{3};
}
+
+struct NotCalledHoldsUnsafeMembers {
+ NotCalledHoldsUnsafeMembers()
+ : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ UnsafeMembers FromCtor;
+ UnsafeMembers FromCtor2;
+ UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
index a1ddc384e0d9b..945a4e06d6afe 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
@@ -154,3 +154,13 @@ namespace test_flag {
}
} //namespace test_flag
+
+struct HoldsStdSpan {
+ char* Ptr;
+ unsigned Size;
+ std::span<char> Span{Ptr, Size}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+
+ HoldsStdSpan(char* P, unsigned S)
+ : Span(P, S) // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ {}
+};
>From 862251bda93c854b6a0c3030c349a7863d43d366 Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Fri, 31 May 2024 13:15:23 -0400
Subject: [PATCH 2/2] applyGadgets once per function and avoid fixits in field
initializers
findGadgets() collects all gadgets into a single list, using a
single DeclUseTracker. Then applyGadgets() is called once with the
full list for a method (including for a ctor with initializers).
Handle ObjC interface variables as they return null from getParent().
Instead go through getContainingInterface() to get the DeclContext.
Disable fixits in field initializers as the machinery will not expect
a statement that is not a function body at this time.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 75 +++++++++++++-----------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 ++-
2 files changed, 48 insertions(+), 34 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index dbed6092cd5a0..5921481059092 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1386,14 +1386,18 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
};
/// Scan the function and return a list of gadgets found with provided kits.
-static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
-findGadgets(const Stmt *S, ASTContext &Ctx,
- const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
+static void findGadgets(const Stmt *S, ASTContext &Ctx,
+ const UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions, FixableGadgetList &FixableGadgets,
+ WarningGadgetList &WarningGadgets,
+ DeclUseTracker &Tracker) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
- FixableGadgetList FixableGadgets;
- WarningGadgetList WarningGadgets;
- DeclUseTracker Tracker;
+ GadgetFinderCallback(FixableGadgetList &FixableGadgets,
+ WarningGadgetList &WarningGadgets,
+ DeclUseTracker &Tracker)
+ : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets),
+ Tracker(Tracker) {}
void run(const MatchFinder::MatchResult &Result) override {
// In debug mode, assert that we've found exactly one gadget.
@@ -1434,10 +1438,14 @@ findGadgets(const Stmt *S, ASTContext &Ctx,
assert(numFound >= 1 && "Gadgets not found in match result!");
assert(numFound <= 1 && "Conflicting bind tags in gadgets!");
}
+
+ FixableGadgetList &FixableGadgets;
+ WarningGadgetList &WarningGadgets;
+ DeclUseTracker &Tracker;
};
MatchFinder M;
- GadgetFinderCallback CB;
+ GadgetFinderCallback CB{FixableGadgets, WarningGadgets, Tracker};
// clang-format off
M.addMatcher(
@@ -1484,8 +1492,6 @@ findGadgets(const Stmt *S, ASTContext &Ctx,
}
M.match(*S, Ctx);
- return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
- std::move(CB.Tracker)};
}
// Compares AST nodes by source locations.
@@ -2113,7 +2119,7 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
if (!IndexString)
return std::nullopt;
- SS << "&" << (*DreString).str() << ".data()"
+ SS << "&" << (*DreString).str() << ".data()"
<< "[" << (*IndexString).str() << "]";
}
return FixItList{
@@ -3312,19 +3318,19 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
SmallVector<Stmt *> Stmts;
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
- if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+ if (MD->getParent()->isLambda() && MD->getParent()->isLocalClass())
+ return;
+ }
+
for (FunctionDecl *FReDecl : FD->redecls()) {
if (FReDecl->isExternC()) {
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
EmitSuggestions = false;
break;
}
@@ -3337,26 +3343,29 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
Stmts.push_back(CI->getInit());
}
}
- }
-
- if (const auto *FD = dyn_cast<FieldDecl>(D)) {
+ } else if (const auto *FlD = dyn_cast<FieldDecl>(D)) {
// Visit in-class initializers for fields.
- if (!FD->hasInClassInitializer())
+ if (!FlD->hasInClassInitializer())
return;
-
- Stmts.push_back(FD->getInClassInitializer());
- }
-
- if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
+ Stmts.push_back(FlD->getInClassInitializer());
+ // In a FieldDecl there is no function body, there is only a single
+ // statement, and the suggestions machinery is not set up to handle that
+ // kind of structure yet and would give poor suggestions or likely even hit
+ // asserts.
+ EmitSuggestions = false;
+ } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
Stmts.push_back(D->getBody());
}
assert(!Stmts.empty());
+ FixableGadgetList FixableGadgets;
+ WarningGadgetList WarningGadgets;
+ DeclUseTracker Tracker;
for (Stmt *S : Stmts) {
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
- applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
- std::move(Tracker), Handler, EmitSuggestions);
+ findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets,
+ WarningGadgets, Tracker);
}
+ applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
+ std::move(Tracker), Handler, EmitSuggestions);
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index ceedeffd20763..c0f79ce53c5ee 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2472,7 +2472,12 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> {
: Callback(Callback) {}
bool VisitFieldDecl(FieldDecl *Node) {
- if (cast<DeclContext>(Node->getParent())->isDependentContext())
+ DeclContext *DeclCtx;
+ if (auto *ID = dyn_cast<ObjCIvarDecl>(Node))
+ DeclCtx = cast<DeclContext>(ID->getContainingInterface());
+ else
+ DeclCtx = cast<DeclContext>(Node->getParent());
+ if (DeclCtx->isDependentContext())
return true; // Not to analyze dependent decl
if (Node->hasInClassInitializer())
Callback(Node);
More information about the cfe-commits
mailing list