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) - {} -}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
