[clang] b05d37d - Revert "Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (#91991)"
Zequan Wu via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 14 13:45:02 PST 2024
Author: Zequan Wu
Date: 2024-11-14T13:43:59-08:00
New Revision: b05d37d0d25e5f3ef181e11eb2a61dd816ae72e1
URL: https://github.com/llvm/llvm-project/commit/b05d37d0d25e5f3ef181e11eb2a61dd816ae72e1
DIFF: https://github.com/llvm/llvm-project/commit/b05d37d0d25e5f3ef181e11eb2a61dd816ae72e1.diff
LOG: Revert "Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (#91991)"
This reverts commit a518ed2d815c16010a6262edd0414a5f60a63a39 because it causes regression. See https://github.com/llvm/llvm-project/pull/91991#issuecomment-2477456171 for detail.
Added:
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 881756b9b5f9b4..81e9b6822a3882 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -171,12 +171,6 @@ class MatchDescendantVisitor
return VisitorBase::TraverseCXXTypeidExpr(Node);
}
- bool TraverseCXXDefaultInitExpr(CXXDefaultInitExpr *Node) {
- if (!TraverseStmt(Node->getExpr()))
- return false;
- return VisitorBase::TraverseCXXDefaultInitExpr(Node);
- }
-
bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
if (!Node)
return true;
@@ -1998,18 +1992,14 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
};
/// Scan the function and return a list of gadgets found with provided kits.
-static void findGadgets(const Stmt *S, ASTContext &Ctx,
- const UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions, FixableGadgetList &FixableGadgets,
- WarningGadgetList &WarningGadgets,
- DeclUseTracker &Tracker) {
+static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
+findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
- GadgetFinderCallback(FixableGadgetList &FixableGadgets,
- WarningGadgetList &WarningGadgets,
- DeclUseTracker &Tracker)
- : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets),
- Tracker(Tracker) {}
+ FixableGadgetList FixableGadgets;
+ WarningGadgetList WarningGadgets;
+ DeclUseTracker Tracker;
void run(const MatchFinder::MatchResult &Result) override {
// In debug mode, assert that we've found exactly one gadget.
@@ -2050,14 +2040,10 @@ static void 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{FixableGadgets, WarningGadgets, Tracker};
+ GadgetFinderCallback CB;
// clang-format off
M.addMatcher(
@@ -2102,7 +2088,9 @@ static void findGadgets(const Stmt *S, ASTContext &Ctx,
// clang-format on
}
- M.match(*S, Ctx);
+ M.match(*D->getBody(), D->getASTContext());
+ return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
+ std::move(CB.Tracker)};
}
// Compares AST nodes by source locations.
@@ -3646,9 +3634,39 @@ class VariableGroupsManagerImpl : public VariableGroupsManager {
}
};
-void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
- WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
- UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
+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);
+
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation and consider it done. No need to deal
@@ -3692,10 +3710,8 @@ void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
if (WarningGadgets.empty())
return;
- WarningGadgetSets UnsafeOps =
- groupWarningGadgetsByVar(std::move(WarningGadgets));
- FixableGadgetSets FixablesForAllVars =
- groupFixablesByVar(std::move(FixableGadgets));
+ UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
+ FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
@@ -3916,56 +3932,3 @@ void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
}
}
}
-
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D);
-
- SmallVector<Stmt *> Stmts;
-
- 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;
- }
- }
-
- Stmts.push_back(FD->getBody());
-
- if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
- for (const CXXCtorInitializer *CI : ID->inits()) {
- Stmts.push_back(CI->getInit());
- }
- }
- } 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) {
- 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/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
index 724d444638b57e..bfc34b55c1f667 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -111,37 +111,6 @@ 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 2{{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}}
- {}
-};
-
// https://github.com/llvm/llvm-project/issues/80482
void testClassMembers() {
UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
@@ -153,95 +122,4 @@ 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}}
-}
-
-// 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}}
- FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}}
- {}
-
- UnsafeMembers FromCtor;
- 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;
-};
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 30b6d4ba9fb904..638c76c58c0250 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
@@ -174,23 +174,3 @@ namespace test_flag {
}
} //namespace test_flag
-
-struct HoldsStdSpanAndInitializedInCtor {
- char* Ptr;
- unsigned Size;
- std::span<char> Span{Ptr, Size}; // no-warning (this code is unreachable)
-
- HoldsStdSpanAndInitializedInCtor(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}}
- {}
-};
-
-struct HoldsStdSpanAndNotInitializedInCtor {
- 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}}
-
- HoldsStdSpanAndNotInitializedInCtor(char* P, unsigned S)
- : Ptr(P), Size(S)
- {}
-};
More information about the cfe-commits
mailing list