Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -30,8 +30,10 @@ WARNING_GADGET(ArraySubscript) WARNING_GADGET(PointerArithmetic) WARNING_GADGET(UnsafeBufferUsageAttr) -FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context -FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context +FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context +FIXABLE_GADGET( + UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context +FIXABLE_GADGET(PointerCtxAccess) #undef FIXABLE_GADGET #undef WARNING_GADGET Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -21,6 +21,13 @@ using namespace ast_matchers; namespace clang::ast_matchers { +// Because we're dealing with raw pointers, let's define what we mean by that. +static auto hasPointerType() { + return hasType(hasCanonicalType(pointerType())); +} + +static auto hasArrayType() { return hasType(hasCanonicalType(arrayType())); } + // A `RecursiveASTVisitor` that traverses all descendants of a given node "n" // except for those belonging to a different callable of "n". class MatchDescendantVisitor @@ -114,17 +121,15 @@ // Because we're dealing with raw pointers, let's define what we mean by that. static auto hasPointerType() { - return hasType(hasCanonicalType(pointerType())); + return hasType(hasCanonicalType(pointerType())); } -static auto hasArrayType() { - return hasType(hasCanonicalType(arrayType())); -} +static auto hasArrayType() { return hasType(hasCanonicalType(arrayType())); } 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)); } @@ -150,19 +155,29 @@ // attribute), or // 2. the operand of a cast operation; or // ... - auto CallArgMatcher = - callExpr(hasAnyArgument(allOf( - hasPointerType() /* array also decays to pointer type*/, - InnerMatcher)), - unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))); + auto CallArgMatcher = callExpr( + forEachArgumentWithParam( + InnerMatcher, hasPointerType() /* array also decays to pointer type*/ + ), + unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))); auto CastOperandMatcher = explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral), castSubExpr(allOf(hasPointerType(), InnerMatcher))); - return stmt(anyOf(CallArgMatcher, CastOperandMatcher)); + auto CompOperandMatcher = + binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="), + eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)), + hasRHS(allOf(hasPointerType(), InnerMatcher)))); + + // Not to be UPC for now + // auto ReturnExprMatcher = + // returnStmt(has(implicitCastExpr(has(InnerMatcher)))); + + return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher)); // FIXME: any more cases? (UPC excludes the RHS of an assignment. For now we // don't have to check that.) } + } // namespace clang::ast_matchers namespace { @@ -216,7 +231,6 @@ Kind K; }; - /// Warning gadgets correspond to unsafe code patterns that warrants /// an immediate warning. class WarningGadget : public Gadget { @@ -227,10 +241,10 @@ bool isWarningGadget() const final { return true; } }; -/// Fixable gadgets correspond to code patterns that aren't always unsafe but need to be -/// properly recognized in order to emit fixes. For example, if a raw pointer-type -/// variable is replaced by a safe C++ container, every use of such variable must be -/// carefully considered and possibly updated. +/// Fixable gadgets correspond to code patterns that aren't always unsafe but +/// need to be properly recognized in order to emit fixes. For example, if a raw +/// pointer-type variable is replaced by a safe C++ container, every use of such +/// variable must be carefully considered and possibly updated. class FixableGadget : public Gadget { public: FixableGadget(Kind K) : Gadget(K) {} @@ -265,10 +279,10 @@ } static Matcher matcher() { - return stmt(unaryOperator( - hasOperatorName("++"), - hasUnaryOperand(ignoringParenImpCasts(hasPointerType())) - ).bind(OpTag)); + return stmt( + unaryOperator(hasOperatorName("++"), + hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))) + .bind(OpTag)); } const UnaryOperator *getBaseStmt() const override { return Op; } @@ -300,10 +314,10 @@ } static Matcher matcher() { - return stmt(unaryOperator( - hasOperatorName("--"), - hasUnaryOperand(ignoringParenImpCasts(hasPointerType())) - ).bind(OpTag)); + return stmt( + unaryOperator(hasOperatorName("--"), + hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))) + .bind(OpTag)); } const UnaryOperator *getBaseStmt() const override { return Op; } @@ -336,13 +350,13 @@ static Matcher matcher() { // FIXME: What if the index is integer literal 0? Should this be // a safe gadget in this case? - // clang-format off - return stmt(arraySubscriptExpr( - hasBase(ignoringParenImpCasts( - anyOf(hasPointerType(), hasArrayType()))), - unless(hasIndex(integerLiteral(equals(0))))) - .bind(ArraySubscrTag)); - // clang-format on + // clang-format off +return stmt(arraySubscriptExpr( + hasBase(ignoringParenImpCasts( + anyOf(hasPointerType(), hasArrayType()))), + unless(hasIndex(integerLiteral(equals(0))))) + .bind(ArraySubscrTag)); + // clang-format on } const ArraySubscriptExpr *getBaseStmt() const override { return ASE; } @@ -365,10 +379,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)) {} @@ -378,32 +392,32 @@ } 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 }; // Represents expressions of the form `DRE[*]` in the Unspecified Lvalue @@ -411,7 +425,8 @@ // 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: @@ -440,22 +455,59 @@ 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 {}; } }; +// Fixable gadget to handle the expressions DRE in the unspecified pointer +// context. +class PointerCtxAccessGadget : public FixableGadget { +private: + static constexpr const char *const DeclRefExprTag = "PtrAccess"; + const DeclRefExpr *Node; + +public: + PointerCtxAccessGadget(const MatchFinder::MatchResult &Result) + : FixableGadget(Kind::PointerCtxAccess), + Node(Result.Nodes.getNodeAs(DeclRefExprTag)) { + assert(Node != nullptr && "Expecting a non-null matching result"); + } + + static bool classof(const Gadget *G) { + return G->getKind() == Kind::PointerCtxAccess; + } + + static Matcher matcher() { + auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType()); + auto target = expr( + ignoringParenImpCasts(declRefExpr(ArrayOrPtr).bind(DeclRefExprTag))); + return stmt(isInUnspecifiedPointerContext(target)); + } + + virtual std::optional getFixits(const Strategy &S) const override; + + virtual const Stmt *getBaseStmt() const override { return Node; } + + virtual DeclUseList getClaimedVarUseSites() const override { + if (const auto *DRE = dyn_cast(Node)) { + return {DRE}; + } + return {}; + } +}; /// 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)) {} @@ -465,13 +517,11 @@ 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[any]` in the Unspecified Pointer @@ -584,11 +634,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: @@ -601,9 +651,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); @@ -616,7 +664,8 @@ } // namespace /// Scan the function and return a list of gadgets found with provided kits. -static std::tuple findGadgets(const Decl *D) { +static std::tuple +findGadgets(const Decl *D) { struct GadgetFinderCallback : MatchFinder::MatchCallback { FixableGadgetList FixableGadgets; @@ -646,17 +695,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!"); @@ -710,7 +759,8 @@ } } - 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)}; } struct WarningGadgetSets { @@ -764,7 +814,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: @@ -841,9 +892,9 @@ // Return the source location of the last character of `Node`. template -static std::optional getEndCharLoc(const NodeTy *Node, - const SourceManager &SM, - const LangOptions &LangOpts) { +static std::optional +getEndCharLoc(const NodeTy *Node, const SourceManager &SM, + const LangOptions &LangOpts) { std::optional LastToken = Lexer::findNextToken( Node->getEndLoc().getLocWithOffset(-1), SM, LangOpts); @@ -878,274 +929,301 @@ } } return {}; // something went wrong. no fix-it -} -// For a non-null initializer `Init` of `T *` type, this function returns -// `FixItHint`s producing a list initializer `{Init, S}` as a part of a fix-it -// to output stream. -// In many cases, this function cannot figure out the actual extent `S`. It -// then will use a place holder to replace `S` to ask users to fill `S` in. The -// initializer shall be used to initialize a variable of type `std::span`. -// -// FIXME: Support multi-level pointers -// -// Parameters: -// `Init` a pointer to the initializer expression -// `Ctx` a reference to the ASTContext -static FixItList -populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx, - const StringRef UserFillPlaceHolder) { - FixItList FixIts{}; - const SourceManager &SM = Ctx.getSourceManager(); - const LangOptions &LangOpts = Ctx.getLangOpts(); - std::string ExtentText = UserFillPlaceHolder.data(); - StringRef One = "1"; - - // Insert `{` before `Init`: - FixIts.push_back(FixItHint::CreateInsertion(Init->getBeginLoc(), "{")); - // Try to get the data extent. Break into different cases: - if (auto CxxNew = dyn_cast(Init->IgnoreImpCasts())) { - // In cases `Init` is `new T[n]` and there is no explicit cast over - // `Init`, we know that `Init` must evaluates to a pointer to `n` objects - // of `T`. So the extent is `n` unless `n` has side effects. Similar but - // simpler for the case where `Init` is `new T`. - if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) { - if (!Ext->HasSideEffects(Ctx)) - ExtentText = - getExprTextOr(Ext, SM, LangOpts, UserFillPlaceHolder); - } else if (!CxxNew->isArray()) - // Although the initializer is not allocating a buffer, the pointer - // variable could still be used in buffer access operations. - ExtentText = One; - } else if (const auto *CArrTy = Ctx.getAsConstantArrayType( - Init->IgnoreImpCasts()->getType())) { - // In cases `Init` is of an array type after stripping off implicit casts, - // the extent is the array size. Note that if the array size is not a - // constant, we cannot use it as the extent. - ExtentText = getAPIntText(CArrTy->getSize()); - } else { - // In cases `Init` is of the form `&Var` after stripping of implicit - // casts, where `&` is the built-in operator, the extent is 1. - if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts())) - 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. + // Generates fix-its replacing an expression of the form UPC(DRE) with + // `DRE.data()` + std::optional PointerCtxAccessGadget::getFixits(const Strategy &S) + const { + if (const auto *DRE = dyn_cast(Node)) + if (const auto *VD = dyn_cast(DRE->getDecl())) { + switch (S.lookup(VD)) { + case Strategy::Kind::Span: { + ASTContext &Ctx = VD->getASTContext(); + SourceManager &SM = Ctx.getSourceManager(); + // Inserts the .data() after the DRE + std::optional endOfOperand = + getEndCharLoc(DRE, SM, Ctx.getLangOpts()); + + return FixItList{{FixItHint::CreateInsertion( + endOfOperand.value().getLocWithOffset(1), ".data()")}}; + } + case Strategy::Kind::Wontfix: + case Strategy::Kind::Iterator: + case Strategy::Kind::Array: + case Strategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); + } + } + return std::nullopt; } - SmallString<32> StrBuffer{}; - std::optional LocPassInit = getEndCharLoc(Init, SM, LangOpts); + // For a non-null initializer `Init` of `T *` type, this function returns + // `FixItHint`s producing a list initializer `{Init, S}` as a part of a + // fix-it to output stream. In many cases, this function cannot figure out the + // actual extent `S`. It then will use a place holder to replace `S` to ask + // users to fill `S` in. The initializer shall be used to initialize a + // variable of type `std::span`. + // + // FIXME: Support multi-level pointers + // + // Parameters: + // `Init` a pointer to the initializer expression + // `Ctx` a reference to the ASTContext + static FixItList populateInitializerFixItWithSpan( + const Expr *Init, const ASTContext &Ctx, + const StringRef UserFillPlaceHolder) { + FixItList FixIts{}; + const SourceManager &SM = Ctx.getSourceManager(); + const LangOptions &LangOpts = Ctx.getLangOpts(); + std::string ExtentText = UserFillPlaceHolder.data(); + StringRef One = "1"; + + // Insert `{` before `Init`: + FixIts.push_back(FixItHint::CreateInsertion(Init->getBeginLoc(), "{")); + // Try to get the data extent. Break into different cases: + if (auto CxxNew = dyn_cast(Init->IgnoreImpCasts())) { + // In cases `Init` is `new T[n]` and there is no explicit cast over + // `Init`, we know that `Init` must evaluates to a pointer to `n` objects + // of `T`. So the extent is `n` unless `n` has side effects. Similar but + // simpler for the case where `Init` is `new T`. + if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) { + if (!Ext->HasSideEffects(Ctx)) + ExtentText = getExprTextOr(Ext, SM, LangOpts, UserFillPlaceHolder); + } else if (!CxxNew->isArray()) + // Although the initializer is not allocating a buffer, the pointer + // variable could still be used in buffer access operations. + ExtentText = One; + } else if (const auto *CArrTy = Ctx.getAsConstantArrayType( + Init->IgnoreImpCasts()->getType())) { + // In cases `Init` is of an array type after stripping off implicit casts, + // the extent is the array size. Note that if the array size is not a + // constant, we cannot use it as the extent. + ExtentText = getAPIntText(CArrTy->getSize()); + } else { + // In cases `Init` is of the form `&Var` after stripping of implicit + // casts, where `&` is the built-in operator, the extent is 1. + if (auto AddrOfExpr = dyn_cast(Init->IgnoreImpCasts())) + 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. + } - if (!LocPassInit) - return {}; - // One past the last character of `Init`: - LocPassInit = LocPassInit->getLocWithOffset(1); - StrBuffer.append(", "); - StrBuffer.append(ExtentText); - StrBuffer.append("}"); - FixIts.push_back( - FixItHint::CreateInsertion(*LocPassInit, StrBuffer.str())); - return FixIts; -} + SmallString<32> StrBuffer{}; + std::optional LocPassInit = + getEndCharLoc(Init, SM, LangOpts); + + if (!LocPassInit) + return {}; + // One past the last character of `Init`: + LocPassInit = LocPassInit->getLocWithOffset(1); + StrBuffer.append(", "); + StrBuffer.append(ExtentText); + StrBuffer.append("}"); + FixIts.push_back(FixItHint::CreateInsertion(*LocPassInit, StrBuffer.str())); + return FixIts; + } -// For a `VarDecl` of the form `T * var (= Init)?`, this -// function generates a fix-it for the declaration, which re-declares `var` to -// be of `span` type and transforms the initializer, if present, to a span -// constructor---`span var {Init, Extent}`, where `Extent` may need the user -// to fill in. -// -// FIXME: support Multi-level pointers -// -// Parameters: -// `D` a pointer the variable declaration node -// `Ctx` a reference to the ASTContext -// Returns: -// the generated fix-it -static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx, - const StringRef UserFillPlaceHolder) { - const QualType &SpanEltT = D->getType()->getPointeeType(); - assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!"); - - FixItList FixIts{}; - SourceLocation - ReplacementLastLoc; // the inclusive end location of the replacement - const SourceManager &SM = Ctx.getSourceManager(); - - if (const Expr *Init = D->getInit()) { - FixItList InitFixIts = - populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder); - - if (InitFixIts.empty()) - return {}; // Something wrong with fixing initializer, give up - // The loc right before the initializer: - ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1); - FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()), - std::make_move_iterator(InitFixIts.end())); - } else if (auto DeclEndCharLoc = getEndCharLoc(D, SM, Ctx.getLangOpts())) - // The loc right before the loc right passed `D`: - ReplacementLastLoc = *DeclEndCharLoc; - else - return {}; // Something wrong, give up - - // Producing fix-it for variable declaration---make `D` to be of span type: - SmallString<32> Replacement; - raw_svector_ostream OS(Replacement); - - OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName(); - FixIts.push_back(FixItHint::CreateReplacement( - SourceRange{D->getBeginLoc(), ReplacementLastLoc}, OS.str())); - return FixIts; -} + // For a `VarDecl` of the form `T * var (= Init)?`, this + // function generates a fix-it for the declaration, which re-declares `var` to + // be of `span` type and transforms the initializer, if present, to a span + // constructor---`span var {Init, Extent}`, where `Extent` may need the + // user to fill in. + // + // FIXME: support Multi-level pointers + // + // Parameters: + // `D` a pointer the variable declaration node + // `Ctx` a reference to the ASTContext + // Returns: + // the generated fix-it + static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx, + const StringRef UserFillPlaceHolder) { + const QualType &SpanEltT = D->getType()->getPointeeType(); + assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!"); + + FixItList FixIts{}; + SourceLocation + ReplacementLastLoc; // the inclusive end location of the replacement + const SourceManager &SM = Ctx.getSourceManager(); + + if (const Expr *Init = D->getInit()) { + FixItList InitFixIts = + populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder); + + if (InitFixIts.empty()) + return {}; // Something wrong with fixing initializer, give up + // The loc right before the initializer: + ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1); + FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()), + std::make_move_iterator(InitFixIts.end())); + } else if (auto DeclEndCharLoc = getEndCharLoc(D, SM, Ctx.getLangOpts())) + // The loc right before the loc right passed `D`: + ReplacementLastLoc = *DeclEndCharLoc; + else + return {}; // Something wrong, give up -static FixItList fixVariableWithSpan(const VarDecl *VD, - const DeclUseTracker &Tracker, - const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { - const DeclStmt *DS = Tracker.lookupDecl(VD); - assert(DS && "Fixing non-local variables not implemented yet!"); - if (!DS->isSingleDecl()) { - // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt` - return{}; + // Producing fix-it for variable declaration---make `D` to be of span type: + SmallString<32> Replacement; + raw_svector_ostream OS(Replacement); + + OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName(); + FixIts.push_back(FixItHint::CreateReplacement( + SourceRange{D->getBeginLoc(), ReplacementLastLoc}, OS.str())); + return FixIts; } - // Currently DS is an unused variable but we'll need it when - // non-single decls are implemented, where the pointee type name - // and the '*' are spread around the place. - (void)DS; - // FIXME: handle cases where DS has multiple declarations - return fixVarDeclWithSpan(VD, Ctx, Handler.getUserFillPlaceHolder()); -} + static FixItList fixVariableWithSpan( + const VarDecl *VD, const DeclUseTracker &Tracker, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + const DeclStmt *DS = Tracker.lookupDecl(VD); + assert(DS && "Fixing non-local variables not implemented yet!"); + if (!DS->isSingleDecl()) { + // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt` + return {}; + } + // Currently DS is an unused variable but we'll need it when + // non-single decls are implemented, where the pointee type name + // and the '*' are spread around the place. + (void)DS; -static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K, - const DeclUseTracker &Tracker, - const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { - switch (K) { - case Strategy::Kind::Span: { - if (VD->getType()->isPointerType() && VD->isLocalVarDecl()) - return fixVariableWithSpan(VD, Tracker, Ctx, Handler); - return {}; + // FIXME: handle cases where DS has multiple declarations + return fixVarDeclWithSpan(VD, Ctx, Handler.getUserFillPlaceHolder()); } - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("Strategy not implemented yet!"); - case Strategy::Kind::Wontfix: - llvm_unreachable("Invalid strategy!"); + + static FixItList fixVariable( + const VarDecl *VD, Strategy::Kind K, const DeclUseTracker &Tracker, + const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { + switch (K) { + case Strategy::Kind::Span: { + if (VD->getType()->isPointerType() && VD->isLocalVarDecl()) + return fixVariableWithSpan(VD, Tracker, Ctx, Handler); + return {}; + } + case Strategy::Kind::Iterator: + case Strategy::Kind::Array: + case Strategy::Kind::Vector: + llvm_unreachable("Strategy not implemented yet!"); + case Strategy::Kind::Wontfix: + llvm_unreachable("Invalid strategy!"); + } } -} -// Returns true iff there exists a `FixItHint` 'h' in `FixIts` such that the -// `RemoveRange` of 'h' overlaps with a macro use. -static bool overlapWithMacro(const FixItList &FixIts) { - // FIXME: For now we only check if the range (or the first token) is (part of) - // a macro expansion. Ideally, we want to check for all tokens in the range. - return llvm::any_of(FixIts, [](const FixItHint &Hint) { - auto BeginLoc = Hint.RemoveRange.getBegin(); - if (BeginLoc.isMacroID()) - // If the range (or the first token) is (part of) a macro expansion: - return true; - return false; - }); -} + // Returns true iff there exists a `FixItHint` 'h' in `FixIts` such that the + // `RemoveRange` of 'h' overlaps with a macro use. + static bool overlapWithMacro(const FixItList &FixIts) { + // FIXME: For now we only check if the range (or the first token) is (part + // of) a macro expansion. Ideally, we want to check for all tokens in the + // range. + return llvm::any_of(FixIts, [](const FixItHint &Hint) { + auto BeginLoc = Hint.RemoveRange.getBegin(); + if (BeginLoc.isMacroID()) + // If the range (or the first token) is (part of) a macro expansion: + return true; + return false; + }); + } -static std::map -getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, - const DeclUseTracker &Tracker, const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { - std::map FixItsForVariable; - for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { - 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()) { - FixItsForVariable.erase(VD); - continue; - } - bool ImpossibleToFix = false; - llvm::SmallVector FixItsForVD; - for (const auto &F : Fixables) { - std::optional Fixits = F->getFixits(S); - if (!Fixits) { - ImpossibleToFix = true; - break; - } else { - const FixItList CorrectFixes = Fixits.value(); - FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(), - CorrectFixes.end()); + static std::map getFixIts( + FixableGadgetSets & FixablesForUnsafeVars, const Strategy &S, + const DeclUseTracker &Tracker, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + std::map FixItsForVariable; + for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { + 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()) { + FixItsForVariable.erase(VD); + continue; + } + bool ImpossibleToFix = false; + llvm::SmallVector FixItsForVD; + for (const auto &F : Fixables) { + std::optional Fixits = F->getFixits(S); + if (!Fixits) { + ImpossibleToFix = true; + break; + } else { + const FixItList CorrectFixes = Fixits.value(); + FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(), + CorrectFixes.end()); + } + } + if (ImpossibleToFix) + FixItsForVariable.erase(VD); + else + FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), + FixItsForVD.begin(), FixItsForVD.end()); + // Fix-it shall not overlap with macros or/and templates: + if (overlapWithMacro(FixItsForVariable[VD])) { + FixItsForVariable.erase(VD); } } - if (ImpossibleToFix) - FixItsForVariable.erase(VD); - else - FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), - FixItsForVD.begin(), FixItsForVD.end()); - // Fix-it shall not overlap with macros or/and templates: - if (overlapWithMacro(FixItsForVariable[VD])) { - FixItsForVariable.erase(VD); - } + return FixItsForVariable; } - return FixItsForVariable; -} -static Strategy -getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { - Strategy S; - for (const VarDecl *VD : UnsafeVars) { - S.set(VD, Strategy::Kind::Span); + static Strategy getNaiveStrategy( + const llvm::SmallVectorImpl &UnsafeVars) { + Strategy S; + for (const VarDecl *VD : UnsafeVars) { + S.set(VD, Strategy::Kind::Span); + } + return S; } - return S; -} -void clang::checkUnsafeBufferUsage(const Decl *D, - UnsafeBufferUsageHandler &Handler) { - assert(D && D->getBody()); + void clang::checkUnsafeBufferUsage(const Decl *D, + UnsafeBufferUsageHandler &Handler) { + assert(D && D->getBody()); - WarningGadgetSets UnsafeOps; - FixableGadgetSets FixablesForUnsafeVars; - DeclUseTracker Tracker; + WarningGadgetSets UnsafeOps; + FixableGadgetSets FixablesForUnsafeVars; + DeclUseTracker Tracker; - { - auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D); - UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); - FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets)); - Tracker = std::move(TrackerRes); - } + { + auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D); + UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); + FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets)); + Tracker = std::move(TrackerRes); + } - // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. - for (auto it = FixablesForUnsafeVars.byVar.cbegin(); - it != FixablesForUnsafeVars.byVar.cend();) { - // FIXME: Support ParmVarDecl as well. - if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) { - it = FixablesForUnsafeVars.byVar.erase(it); - } else { - ++it; + // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. + for (auto it = FixablesForUnsafeVars.byVar.cbegin(); + it != FixablesForUnsafeVars.byVar.cend();) { + // FIXME: Support ParmVarDecl as well. + if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) { + it = FixablesForUnsafeVars.byVar.erase(it); + } else { + ++it; + } } - } - llvm::SmallVector UnsafeVars; - for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar) - UnsafeVars.push_back(VD); + llvm::SmallVector UnsafeVars; + for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar) + UnsafeVars.push_back(VD); - Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); - std::map FixItsForVariable = - getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker, - D->getASTContext(), Handler); + Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); + std::map FixItsForVariable = + getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker, + D->getASTContext(), Handler); - // FIXME Detect overlapping FixIts. + // FIXME Detect overlapping FixIts. - for (const auto &G : UnsafeOps.noVar) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false); - } + for (const auto &G : UnsafeOps.noVar) { + Handler.handleUnsafeOperation(G->getBaseStmt(), + /*IsRelatedToDecl=*/false); + } - for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { - auto FixItsIt = FixItsForVariable.find(VD); - Handler.handleFixableVariable(VD, FixItsIt != FixItsForVariable.end() - ? std::move(FixItsIt->second) - : FixItList{}); - for (const auto &G : WarningGadgets) { - Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true); + for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { + auto FixItsIt = FixItsForVariable.find(VD); + Handler.handleFixableVariable(VD, FixItsIt != FixItsForVariable.end() + ? std::move(FixItsIt->second) + : FixItList{}); + for (const auto &G : WarningGadgets) { + Handler.handleUnsafeOperation(G->getBaseStmt(), + /*IsRelatedToDecl=*/true); + } } } -} Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp @@ -0,0 +1,91 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +void foo(int* v) { +} + +void m1(int* v1, int* v2, int) { + +} + +void condition_check_nullptr() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + if(p != nullptr) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" +} + +void condition_check() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + auto q = new int[10]; + + int tmp = p[5]; + if(p == q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(q != p){} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:12}:".data()" + + if(p < q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p <= q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p > q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if(p >= q) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + if( p == q && p != nullptr) {} + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:".data()" +} + +void unsafe_method_invocation_single_param() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + foo(p); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()" + +} + +void safe_method_invocation_single_param() { + auto p = new int[10]; + foo(p); +} + +void unsafe_method_invocation_double_param() { + auto p = new int[10]; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}" + + int tmp = p[5]; + m1(p, p, 10); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:".data()" + + auto q = new int[10]; + + m1(p, q, 4); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()" + + m1(q, p, 9); + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()" + + m1(q, q, 8); +} +