diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -9,8 +9,8 @@ #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/Preprocessor.h" #include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" #include "llvm/ADT/SmallVector.h" #include #include @@ -113,13 +113,14 @@ AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher, innerMatcher) { const DynTypedMatcher &DTM = static_cast(innerMatcher); - - MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); + + MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); return Visitor.findMatch(DynTypedNode::create(Node)); } // Matches a `Stmt` node iff the node is in a safe-buffer opt-out region -AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *, Handler) { +AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *, + Handler) { return !Handler->isSafeBufferOptOut(Node.getBeginLoc()); } @@ -130,7 +131,7 @@ // Returns a matcher that matches any expression 'e' such that `innerMatcher` // matches 'e' and 'e' is in an Unspecified Lvalue Context. static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) { -// clang-format off + // clang-format off return expr(anyOf( implicitCastExpr( @@ -331,7 +332,7 @@ anyOf(hasPointerType(), hasArrayType()))), unless(hasIndex(integerLiteral(equals(0))))) .bind(ArraySubscrTag)); - // clang-format on + // clang-format on } const ArraySubscriptExpr *getBaseStmt() const override { return ASE; } @@ -354,10 +355,10 @@ static constexpr const char *const PointerArithmeticTag = "ptrAdd"; static constexpr const char *const PointerArithmeticPointerTag = "ptrAddPtr"; const BinaryOperator *PA; // pointer arithmetic expression - const Expr * Ptr; // the pointer expression in `PA` + const Expr *Ptr; // the pointer expression in `PA` public: - PointerArithmeticGadget(const MatchFinder::MatchResult &Result) + PointerArithmeticGadget(const MatchFinder::MatchResult &Result) : WarningGadget(Kind::PointerArithmetic), PA(Result.Nodes.getNodeAs(PointerArithmeticTag)), Ptr(Result.Nodes.getNodeAs(PointerArithmeticPointerTag)) {} @@ -367,43 +368,42 @@ } static Matcher matcher() { - auto HasIntegerType = anyOf( - hasType(isInteger()), hasType(enumType())); - auto PtrAtRight = allOf(hasOperatorName("+"), - hasRHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), - hasLHS(HasIntegerType)); - auto PtrAtLeft = allOf( - anyOf(hasOperatorName("+"), hasOperatorName("-"), - hasOperatorName("+="), hasOperatorName("-=")), - hasLHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), - hasRHS(HasIntegerType)); - - return stmt(binaryOperator(anyOf(PtrAtLeft, PtrAtRight)).bind(PointerArithmeticTag)); + auto HasIntegerType = anyOf(hasType(isInteger()), hasType(enumType())); + auto PtrAtRight = + allOf(hasOperatorName("+"), + hasRHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), + hasLHS(HasIntegerType)); + auto PtrAtLeft = + allOf(anyOf(hasOperatorName("+"), hasOperatorName("-"), + hasOperatorName("+="), hasOperatorName("-=")), + hasLHS(expr(hasPointerType()).bind(PointerArithmeticPointerTag)), + hasRHS(HasIntegerType)); + + return stmt(binaryOperator(anyOf(PtrAtLeft, PtrAtRight)) + .bind(PointerArithmeticTag)); } const Stmt *getBaseStmt() const override { return PA; } DeclUseList getClaimedVarUseSites() const override { - if (const auto *DRE = - dyn_cast(Ptr->IgnoreParenImpCasts())) { + if (const auto *DRE = dyn_cast(Ptr->IgnoreParenImpCasts())) { return {DRE}; } return {}; } // FIXME: pointer adding zero should be fine - //FIXME: this gadge will need a fix-it + // FIXME: this gadge will need a fix-it }; - /// A call of a function or method that performs unchecked buffer operations /// over one of its pointer parameters. class UnsafeBufferUsageAttrGadget : public WarningGadget { - constexpr static const char *const OpTag = "call_expr"; - const CallExpr *Op; + constexpr static const char *const OpTag = "call_expr"; + const CallExpr *Op; public: - UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result) + UnsafeBufferUsageAttrGadget(const MatchFinder::MatchResult &Result) : WarningGadget(Kind::UnsafeBufferUsageAttr), Op(Result.Nodes.getNodeAs(OpTag)) {} @@ -413,22 +413,20 @@ static Matcher matcher() { return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))) - .bind(OpTag)); + .bind(OpTag)); } const Stmt *getBaseStmt() const override { return Op; } - DeclUseList getClaimedVarUseSites() const override { - return {}; - } + DeclUseList getClaimedVarUseSites() const override { return {}; } }; - // Represents expressions of the form `DRE[*]` in the Unspecified Lvalue // Context (see `isInUnspecifiedLvalueContext`). // Note here `[]` is the built-in subscript operator. class ULCArraySubscriptGadget : public FixableGadget { private: - static constexpr const char *const ULCArraySubscriptTag = "ArraySubscriptUnderULC"; + static constexpr const char *const ULCArraySubscriptTag = + "ArraySubscriptUnderULC"; const ArraySubscriptExpr *Node; public: @@ -457,7 +455,8 @@ virtual const Stmt *getBaseStmt() const override { return Node; } virtual DeclUseList getClaimedVarUseSites() const override { - if (const auto *DRE = dyn_cast(Node->getBase()->IgnoreImpCasts())) { + if (const auto *DRE = + dyn_cast(Node->getBase()->IgnoreImpCasts())) { return {DRE}; } return {}; @@ -530,11 +529,11 @@ class Strategy { public: enum class Kind { - Wontfix, // We don't plan to emit a fixit for this variable. - Span, // We recommend replacing the variable with std::span. - Iterator, // We recommend replacing the variable with std::span::iterator. - Array, // We recommend replacing the variable with std::array. - Vector // We recommend replacing the variable with std::vector. + Wontfix, // We don't plan to emit a fixit for this variable. + Span, // We recommend replacing the variable with std::span. + Iterator, // We recommend replacing the variable with std::span::iterator. + Array, // We recommend replacing the variable with std::array. + Vector // We recommend replacing the variable with std::vector. }; private: @@ -547,9 +546,7 @@ Strategy(const Strategy &) = delete; // Let's avoid copies. Strategy(Strategy &&) = default; - void set(const VarDecl *VD, Kind K) { - Map[VD] = K; - } + void set(const VarDecl *VD, Kind K) { Map[VD] = K; } Kind lookup(const VarDecl *VD) const { auto I = Map.find(VD); @@ -593,17 +590,17 @@ // Figure out which matcher we've found, and call the appropriate // subclass constructor. // FIXME: Can we do this more logarithmically? -#define FIXABLE_GADGET(name) \ - if (Result.Nodes.getNodeAs(#name)) { \ - FixableGadgets.push_back(std::make_unique(Result)); \ - NEXT; \ - } +#define FIXABLE_GADGET(name) \ + if (Result.Nodes.getNodeAs(#name)) { \ + FixableGadgets.push_back(std::make_unique(Result)); \ + NEXT; \ + } #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" -#define WARNING_GADGET(name) \ - if (Result.Nodes.getNodeAs(#name)) { \ - WarningGadgets.push_back(std::make_unique(Result)); \ - NEXT; \ - } +#define WARNING_GADGET(name) \ + if (Result.Nodes.getNodeAs(#name)) { \ + WarningGadgets.push_back(std::make_unique(Result)); \ + NEXT; \ + } #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" assert(numFound >= 1 && "Gadgets not found in match result!"); @@ -657,11 +654,24 @@ } } - return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; + return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), + std::move(CB.Tracker)}; } +// Compares AST nodes by source locations. +template struct CompareNode { + bool operator()(const NodeTy *N1, const NodeTy *N2) const { + return N1->getBeginLoc().getRawEncoding() < + N2->getBeginLoc().getRawEncoding(); + } +}; + struct WarningGadgetSets { - std::map>> byVar; + std::map>, + // To keep keys sorted by their locations in the map so that the + // order is deterministic: + CompareNode> + byVar; // These Gadgets are not related to pointer variables (e. g. temporaries). llvm::SmallVector, 16> noVar; }; @@ -709,8 +719,8 @@ return FixablesForUnsafeVars; } -bool clang::internal::anyConflict( - const SmallVectorImpl &FixIts, const SourceManager &SM) { +bool clang::internal::anyConflict(const SmallVectorImpl &FixIts, + const SourceManager &SM) { // A simple interval overlap detection algorithm. Sorts all ranges by their // begin location then finds the first overlap in one pass. std::vector All; // a copy of `FixIts` @@ -742,7 +752,8 @@ std::optional ULCArraySubscriptGadget::getFixits(const Strategy &S) const { - if (const auto *DRE = dyn_cast(Node->getBase()->IgnoreImpCasts())) + if (const auto *DRE = + dyn_cast(Node->getBase()->IgnoreImpCasts())) if (const auto *VD = dyn_cast(DRE->getDecl())) { switch (S.lookup(VD)) { case Strategy::Kind::Span: { @@ -793,7 +804,7 @@ // Return text representation of an `Expr`. static StringRef getExprText(const Expr *E, const SourceManager &SM, - const LangOptions &LangOpts) { + const LangOptions &LangOpts) { SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts); return Lexer::getSourceText( @@ -850,8 +861,8 @@ if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf && isa_and_present(AddrOfExpr->getSubExpr())) ExtentText = One; - // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, `std::addressof`, and explicit casting, etc. - // etc. + // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, `std::addressof`, + // and explicit casting, etc. etc. } SmallString<32> StrBuffer{}; @@ -918,7 +929,7 @@ assert(DS && "Fixing non-local variables not implemented yet!"); if (!DS->isSingleDecl()) { // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt` - return{}; + return {}; } // Currently DS is an unused variable but we'll need it when // non-single decls are implemented, where the pointee type name @@ -969,7 +980,8 @@ UnsafeBufferUsageHandler &Handler) { std::map FixItsForVariable; for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { - FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), Tracker, Ctx, Handler); + FixItsForVariable[VD] = + fixVariable(VD, S.lookup(VD), Tracker, Ctx, Handler); // If we fail to produce Fix-It for the declaration we have to skip the // variable entirely. if (FixItsForVariable[VD].empty()) { @@ -999,7 +1011,7 @@ // a fix-it conflicts with another one if (overlapWithMacro(FixItsForVariable[VD]) || clang::internal::anyConflict(FixItsForVariable[VD], - Ctx.getSourceManager())) { + Ctx.getSourceManager())) { FixItsForVariable.erase(VD); } } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp @@ -1,4 +1,3 @@ -// REQUIRES: !system-windows // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s typedef int * Int_ptr_t; typedef int Int_t;