llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: None (jkorous-apple) <details> <summary>Changes</summary> --- Patch is 39.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80084.diff 7 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+44-3) - (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+218-138) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+14-2) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp (+24) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp (+5-7) - (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp (+167) - (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp (+5) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index aca1ad998822c..2c9ebf3fb42d0 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -42,6 +42,43 @@ class VariableGroupsManager { virtual VarGrpRef getGroupOfParms() const =0; }; +// FixitStrategy is a map from variables to the way we plan to emit fixes for +// these variables. It is figured out gradually by trying different fixes +// for different variables depending on gadgets in which these variables +// participate. +class FixitStrategy { +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. + }; + +private: + using MapTy = llvm::DenseMap<const VarDecl *, Kind>; + + MapTy Map; + +public: + FixitStrategy() = default; + FixitStrategy(const FixitStrategy &) = delete; // Let's avoid copies. + FixitStrategy &operator=(const FixitStrategy &) = delete; + FixitStrategy(FixitStrategy &&) = default; + FixitStrategy &operator=(FixitStrategy &&) = default; + + void set(const VarDecl *VD, Kind K) { Map[VD] = K; } + + Kind lookup(const VarDecl *VD) const { + auto I = Map.find(VD); + if (I == Map.end()) + return Kind::Wontfix; + + return I->second; + } +}; + /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. class UnsafeBufferUsageHandler { @@ -58,6 +95,8 @@ class UnsafeBufferUsageHandler { #endif public: + enum class TargetType { Span, Array }; + UnsafeBufferUsageHandler() = default; virtual ~UnsafeBufferUsageHandler() = default; @@ -75,9 +114,11 @@ class UnsafeBufferUsageHandler { /// /// `D` is the declaration of the callable under analysis that owns `Variable` /// and all of its group mates. - virtual void handleUnsafeVariableGroup(const VarDecl *Variable, - const VariableGroupsManager &VarGrpMgr, - FixItList &&Fixes, const Decl *D) = 0; + virtual void + handleUnsafeVariableGroup(const VarDecl *Variable, + const VariableGroupsManager &VarGrpMgr, + FixItList &&Fixes, const Decl *D, + const FixitStrategy &VarTargetTypes) = 0; #ifndef NDEBUG public: diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 823cd2a7b9969..18d0ca7a17d3c 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -12,10 +12,13 @@ #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/CharInfo.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" #include <memory> #include <optional> #include <queue> @@ -407,9 +410,6 @@ using DeclUseList = SmallVector<const DeclRefExpr *, 1>; // Convenience typedef. using FixItList = SmallVector<FixItHint, 4>; - -// Defined below. -class Strategy; } // namespace namespace { @@ -486,7 +486,7 @@ class FixableGadget : public Gadget { /// Returns a fixit that would fix the current gadget according to /// the current strategy. Returns std::nullopt if the fix cannot be produced; /// returns an empty list if no fixes are necessary. - virtual std::optional<FixItList> getFixits(const Strategy &) const { + virtual std::optional<FixItList> getFixits(const FixitStrategy &) const { return std::nullopt; } @@ -737,7 +737,8 @@ class PointerInitGadget : public FixableGadget { return stmt(PtrInitStmt); } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { // FIXME: This needs to be the entire DeclStmt, assuming that this method @@ -789,7 +790,8 @@ class PointerAssignmentGadget : public FixableGadget { return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr)); } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { // FIXME: This should be the binary operator, assuming that this method @@ -892,7 +894,8 @@ class ULCArraySubscriptGadget : public FixableGadget { return expr(isInUnspecifiedLvalueContext(Target)); } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -932,7 +935,8 @@ class UPCStandalonePointerGadget : public FixableGadget { return stmt(isInUnspecifiedPointerContext(target)); } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -976,7 +980,8 @@ class PointerDereferenceGadget : public FixableGadget { virtual const Stmt *getBaseStmt() const final { return Op; } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; }; // Represents expressions of the form `&DRE[any]` in the Unspecified Pointer @@ -1009,7 +1014,8 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget { .bind(UPCAddressofArraySubscriptTag))))); } - virtual std::optional<FixItList> getFixits(const Strategy &) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1088,46 +1094,6 @@ class DeclUseTracker { }; } // namespace -namespace { -// Strategy is a map from variables to the way we plan to emit fixes for -// these variables. It is figured out gradually by trying different fixes -// for different variables depending on gadgets in which these variables -// participate. -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. - }; - -private: - using MapTy = llvm::DenseMap<const VarDecl *, Kind>; - - MapTy Map; - -public: - Strategy() = default; - Strategy(const Strategy &) = delete; // Let's avoid copies. - Strategy &operator=(const Strategy &) = delete; - Strategy(Strategy &&) = default; - Strategy &operator=(Strategy &&) = default; - - void set(const VarDecl *VD, Kind K) { Map[VD] = K; } - - Kind lookup(const VarDecl *VD) const { - auto I = Map.find(VD); - if (I == Map.end()) - return Kind::Wontfix; - - return I->second; - } -}; -} // namespace - - // Representing a pointer type expression of the form `++Ptr` in an Unspecified // Pointer Context (UPC): class UPCPreIncrementGadget : public FixableGadget { @@ -1159,7 +1125,8 @@ class UPCPreIncrementGadget : public FixableGadget { ).bind(UPCPreIncrementTag))))); } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1204,7 +1171,8 @@ class UUCAddAssignGadget : public FixableGadget { // clang-format on } - virtual std::optional<FixItList> getFixits(const Strategy &S) const override; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &S) const override; virtual const Stmt *getBaseStmt() const override { return Node; } @@ -1254,7 +1222,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget { // clang-format on } - virtual std::optional<FixItList> getFixits(const Strategy &s) const final; + virtual std::optional<FixItList> + getFixits(const FixitStrategy &s) const final; // TODO remove this method from FixableGadget interface virtual const Stmt *getBaseStmt() const final { return nullptr; } @@ -1464,38 +1433,40 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts, } std::optional<FixItList> -PointerAssignmentGadget::getFixits(const Strategy &S) const { +PointerAssignmentGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl()); const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl()); switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("unsupported strategies for FixableGadgets"); + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case FixitStrategy::Kind::Wontfix: + return std::nullopt; + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; } std::optional<FixItList> -PointerInitGadget::getFixits(const Strategy &S) const { +PointerInitGadget::getFixits(const FixitStrategy &S) const { const auto *LeftVD = PtrInitLHS; const auto *RightVD = cast<VarDecl>(PtrInitRHS->getDecl()); switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Span: + if (S.lookup(RightVD) == FixitStrategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case FixitStrategy::Kind::Wontfix: + return std::nullopt; + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; @@ -1512,12 +1483,12 @@ static bool isNonNegativeIntegerExpr(const Expr *Expr, const VarDecl *VD, } std::optional<FixItList> -ULCArraySubscriptGadget::getFixits(const Strategy &S) const { +ULCArraySubscriptGadget::getFixits(const FixitStrategy &S) const { if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) { switch (S.lookup(VD)) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { // If the index has a negative constant value, we give up as no valid // fix-it can be generated: @@ -1528,10 +1499,11 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const { // no-op is a good fix-it, otherwise return FixItList{}; } - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Array: + return FixItList{}; + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } } @@ -1542,17 +1514,18 @@ static std::optional<FixItList> // forward declaration fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node); std::optional<FixItList> -UPCAddressofArraySubscriptGadget::getFixits(const Strategy &S) const { +UPCAddressofArraySubscriptGadget::getFixits(const FixitStrategy &S) const { auto DREs = getClaimedVarUseSites(); const auto *VD = cast<VarDecl>(DREs.front()->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: + case FixitStrategy::Kind::Span: return fixUPCAddressofArraySubscriptWithSpan(Node); - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } return std::nullopt; // something went wrong, no fix-it @@ -1803,10 +1776,10 @@ getSpanTypeText(StringRef EltTyText, } std::optional<FixItList> -DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { +DerefSimplePtrArithFixableGadget::getFixits(const FixitStrategy &s) const { const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl()); - if (VD && s.lookup(VD) == Strategy::Kind::Span) { + if (VD && s.lookup(VD) == FixitStrategy::Kind::Span) { ASTContext &Ctx = VD->getASTContext(); // std::span can't represent elements before its begin() if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx)) @@ -1866,10 +1839,10 @@ DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { } std::optional<FixItList> -PointerDereferenceGadget::getFixits(const Strategy &S) const { +PointerDereferenceGadget::getFixits(const FixitStrategy &S) const { const VarDecl *VD = cast<VarDecl>(BaseDeclRefExpr->getDecl()); switch (S.lookup(VD)) { - case Strategy::Kind::Span: { + case FixitStrategy::Kind::Span: { ASTContext &Ctx = VD->getASTContext(); SourceManager &SM = Ctx.getSourceManager(); // Required changes: *(ptr); => (ptr[0]); and *ptr; => ptr[0] @@ -1884,11 +1857,12 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const { } break; } - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("Strategy not implemented yet!"); - case Strategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: + llvm_unreachable("FixitStrategy not implemented yet!"); + case FixitStrategy::Kind::Wontfix: llvm_unreachable("Invalid strategy!"); } @@ -1897,27 +1871,27 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const { // Generates fix-its replacing an expression of the form UPC(DRE) with // `DRE.data()` -std::optional<FixItList> UPCStandalonePointerGadget::getFixits(const Strategy &S) - const { +std::optional<FixItList> +UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const { const auto VD = cast<VarDecl>(Node->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<SourceLocation> EndOfOperand = - getPastLoc(Node, SM, Ctx.getLangOpts()); - - if (EndOfOperand) - return FixItList{{FixItHint::CreateInsertion( - *EndOfOperand, ".data()")}}; - // FIXME: Points inside a macro expansion. - break; + case FixitStrategy::Kind::Span: { + ASTContext &Ctx = VD->getASTContext(); + SourceManager &SM = Ctx.getSourceManager(); + // Inserts the .data() after the DRE + std::optional<SourceLocation> EndOfOperand = + getPastLoc(Node, SM, Ctx.getLangOpts()); + + if (EndOfOperand) + return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}}; + // FIXME: Points inside a macro expansion. + break; } - case Strategy::Kind::Wontfix: - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: + case FixitStrategy::Kind::Wontfix: + case FixitStrategy::Kind::Iterator: + case FixitStrategy::Kind::Array: + return std::nullopt; + case FixitStrategy::Kind::Vector: llvm_unreachable("unsupported strategies for FixableGadgets"); } @@ -1962,14 +1936,14 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) { } std::optional<FixItList> -UUCAddAssignGadget::getFixits(const Strategy &S) const { +UUCAddAssignGadget::getFixits(const FixitStrategy &S) const { DeclUseList DREs = getClaimedVarUseSites(); if (DREs.size() != 1) return std::nullopt; // In cases of `Ptr += n` where `Ptr` is not a DRE, we // give up if (const VarDecl *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) { - if (S.lookup(VD) == Strategy::Kind::Span) { + if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; const Stmt *AddAssignNode = getBaseStmt(); @@ -2003,14 +1977,15 @@ UUCAddAssignGadget::getFixits(const Strategy &S) const { return std::nullopt; // Not in the cases that we can handle for now, give up. } -std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) const { +std::optional<FixItList> +UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const { DeclUseList DREs = getClaimedVarUseSites(); if (DREs.size() != 1) return std::nullopt; // In cases of `++Ptr` where `Ptr` is not a DRE, we // give up if (const VarDecl *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) { - if (S.lookup(VD) == Strategy::Kind::Span) { + if (S.lookup(VD) == FixitStrategy::Kind::Span) { FixItList Fixes; std::stringstream SS; const Stmt *PreIncNode = getBaseStmt(); @@ -2261,7 +2236,7 @@ static bool hasConflictingOverload(const FunctionDecl *FD) { // } // static std::optional<FixItList> -createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, +createOverloadsForFixedParams(const FixitStrategy &S, const FunctionDecl *FD, const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { // FIXME: need to make this conflict checking better: @@ -2278,9 +2253,9 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, for (unsigned i = 0; i < NumParms; i++) { const ParmVarDecl *PVD = FD->getParamDecl(i); - if (S.lookup(PVD) == Strategy::Kind::Wontfix) + if (S.lookup(PVD) == FixitStrategy::Kind::Wontfix) continue; - if (S.lookup(PVD) != Strategy::Kind::Span) + if (S.lookup(PVD) != FixitStrategy::Kind::Span) // Not supported, not suppose to happen: return std::nullopt; @@ -2291,7 +2266,8 @@ createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, if (!PteTyText) // something wrong in obtaining the text of the pointee type, give up return std::nullopt; - // FIXME: whether we should create std::span type depends on the Strategy. + // FIXME: whether we should create std::span type depends on the + // FixitStrategy. NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals); ParmsMask[i] = true; AtLeastOneParmToFix = true; @@ -2495,10 +2471,104 @@ static FixItList fixVariableWithSpan(const VarDecl *VD, return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler); } +static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + FixItList FixIts{}; + + if (auto CAT = dyn_cast<clang::ConstantArrayType>(D-... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/80084 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
