[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
Tue Jun 11 12:16:35 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/4] 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 c0e634d6070c4f7b8704d6b5e0fa3efca64f9db6 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/4] 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 | 73 +++++++++++++-----------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 ++-
2 files changed, 47 insertions(+), 33 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index dbed6092cd5a0..bfbf84a03b2bf 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.
@@ -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);
>From aeb62902086671bfd215d7e7ae468ca8c376e55c Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Mon, 10 Jun 2024 12:45:51 -0400
Subject: [PATCH 3/4] Add comment on AggregateUnsafeMembers
The aggregate test case generates an AST without any ctor and
thus without any ctor initializers, inlcuding no CXXDefaultInitExpr.
---
clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp | 2 ++
1 file changed, 2 insertions(+)
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 a0efecefe1c89..8ba198a7e262c 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -142,6 +142,8 @@ struct SubclassUnsafeMembers : public UnsafeMembers {
{}
};
+// Without an explicit constructor, there is no CXXDefaultInitExpr to anchor on
+// in matchers.
struct AggregateUnsafeMembers {
UnsafeMembers f1;
UnsafeMembers f2{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
>From 7977a45fa60862140e908f3803507c00248cebe6 Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Tue, 11 Jun 2024 15:15:09 -0400
Subject: [PATCH 4/4] Avoid visiting field initializers in aggregate records
In aggregates, the field initializer may be never invoked in which
case we do not need to visit and warn on it.
FIXMEs are left in place for aggregates where the field initializer
is used, but there's currently no warning.
---
clang/lib/Sema/AnalysisBasedWarnings.cpp | 24 ++++-
...warn-unsafe-buffer-usage-function-attr.cpp | 95 +++++++++++++++----
2 files changed, 95 insertions(+), 24 deletions(-)
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index c0f79ce53c5ee..5cb6eebeee478 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2473,10 +2473,28 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> {
bool VisitFieldDecl(FieldDecl *Node) {
DeclContext *DeclCtx;
- if (auto *ID = dyn_cast<ObjCIvarDecl>(Node))
+ if (auto *ID = dyn_cast<ObjCIvarDecl>(Node)) {
DeclCtx = cast<DeclContext>(ID->getContainingInterface());
- else
- DeclCtx = cast<DeclContext>(Node->getParent());
+ } else {
+ RecordDecl *RD = Node->getParent();
+ if (auto *CCRD = dyn_cast<CXXRecordDecl>(RD); CCRD->isAggregate()) {
+ // Aggregates have implicitly generated constructors which are not
+ // traversed by our AST visitors at this time. The field initializers in
+ // an aggregate may never be used if the callers always explicitly
+ // initialize them, in which case we do not need to warn on unsafe
+ // buffer usage in the initializer.
+ //
+ // FIXME: We should go through the implicit aggregate initialization
+ // (either an implicit CXXConstructExpr for value-init or an implicit
+ // ListInitExpr for aggregate-init) to determine if a field's default
+ // initializer (CXXDefaultInitExpr) is actually used. If it is, then we
+ // should visit it, while retaining a reference to the caller for
+ // showing the path to the use of the default initializer in the
+ // warning.
+ return true;
+ }
+ DeclCtx = cast<DeclContext>(RD);
+ }
if (DeclCtx->isDependentContext())
return true; // Not to analyze dependent decl
if (Node->hasInClassInitializer())
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 8ba198a7e262c..b9e517a35de7d 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -142,13 +142,6 @@ struct SubclassUnsafeMembers : public UnsafeMembers {
{}
};
-// Without an explicit constructor, there is no CXXDefaultInitExpr to anchor on
-// in matchers.
-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}}
@@ -166,22 +159,10 @@ void testClassMembers() {
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};
}
+// Not an aggregate, so its constructor is not implicit code and will be
+// visited/checked for warnings.
struct NotCalledHoldsUnsafeMembers {
NotCalledHoldsUnsafeMembers()
: FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}}
@@ -192,3 +173,75 @@ struct NotCalledHoldsUnsafeMembers {
UnsafeMembers FromCtor2;
UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
};
+
+// An aggregate, so its constructor is implicit code. Since it's not called, it
+// is never generated.
+struct AggregateUnused {
+ UnsafeMembers f1;
+ // While this field would trigger the warning during initialization, since
+ // it's unused, there's no code generated that does the initialization, so
+ // no warning.
+ UnsafeMembers f2{3};
+};
+
+struct AggregateExplicitlyInitializedSafe {
+ UnsafeMembers f1;
+ // The warning is not fired as the field is always explicltly initialized
+ // elsewhere. This initializer is never used.
+ UnsafeMembers f2{3};
+};
+
+void testAggregateExplicitlyInitializedSafe() {
+ AggregateExplicitlyInitializedSafe A{
+ .f2 = UnsafeMembers(), // A safe constructor.
+ };
+}
+
+struct AggregateExplicitlyInitializedUnsafe {
+ UnsafeMembers f1;
+ // The warning is not fired as the field is always explicltly initialized
+ // elsewhere. This initializer is never used.
+ UnsafeMembers f2{3};
+};
+
+void testAggregateExplicitlyInitializedUnsafe() {
+ AggregateExplicitlyInitializedUnsafe A{
+ .f2 = UnsafeMembers(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ };
+}
+
+struct AggregateViaAggregateInit {
+ UnsafeMembers f1;
+ // FIXME: A construction of this class does initialize the field through
+ // this initializer, so it should warn. Ideally it should also point to
+ // where the site of the construction is in testAggregateViaAggregateInit().
+ UnsafeMembers f2{3};
+};
+
+void testAggregateViaAggregateInit() {
+ AggregateViaAggregateInit A{};
+};
+
+struct AggregateViaValueInit {
+ UnsafeMembers f1;
+ // FIXME: A construction of this class does initialize the field through
+ // this initializer, so it should warn. Ideally it should also point to
+ // where the site of the construction is in testAggregateViaValueInit().
+ UnsafeMembers f2{3};
+};
+
+void testAggregateViaValueInit() {
+ auto A = AggregateViaValueInit();
+};
+
+struct AggregateViaDefaultInit {
+ UnsafeMembers f1;
+ // FIXME: A construction of this class does initialize the field through
+ // this initializer, so it should warn. Ideally it should also point to
+ // where the site of the construction is in testAggregateViaValueInit().
+ UnsafeMembers f2{3};
+};
+
+void testAggregateViaDefaultInit() {
+ AggregateViaDefaultInit A;
+};
More information about the cfe-commits
mailing list