diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -14,22 +14,22 @@ /// Unsafe gadgets correspond to unsafe code patterns that warrant /// an immediate warning. -#ifndef UNSAFE_GADGET -#define UNSAFE_GADGET(name) GADGET(name) +#ifndef WARNING_GADGET +#define WARNING_GADGET(name) GADGET(name) #endif /// Safe gadgets correspond to code patterns that aren't unsafe but need to be /// properly recognized in order to emit correct warnings and fixes over unsafe /// gadgets. -#ifndef SAFE_GADGET -#define SAFE_GADGET(name) GADGET(name) +#ifndef FIXABLE_GADGET +#define FIXABLE_GADGET(name) GADGET(name) #endif -UNSAFE_GADGET(Increment) -UNSAFE_GADGET(Decrement) -UNSAFE_GADGET(ArraySubscript) -UNSAFE_GADGET(PointerArithmetic) +WARNING_GADGET(Increment) +WARNING_GADGET(Decrement) +WARNING_GADGET(ArraySubscript) +WARNING_GADGET(PointerArithmetic) -#undef SAFE_GADGET -#undef UNSAFE_GADGET +#undef FIXABLE_GADGET +#undef WARNING_GADGET #undef GADGET 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 @@ -137,9 +137,9 @@ /// rigid AST structure that constitutes an operation on a pointer-type object. /// Discovery of a gadget in the code corresponds to claiming that we understand /// what this part of code is doing well enough to potentially improve it. -/// Gadgets can be unsafe (immediately deserving a warning) or safe (not -/// deserving a warning per se, but affecting our decision-making process -/// nonetheless). +/// Gadgets can be warning (immediately deserving a warning) or fixable (not +/// always deserving a warning per se, but requires our attention to identify +/// it warrants a fixit). class Gadget { public: enum class Kind { @@ -156,7 +156,7 @@ Kind getKind() const { return K; } - virtual bool isSafe() const = 0; + virtual bool isWarningGadget() const = 0; virtual const Stmt *getBaseStmt() const = 0; /// Returns the list of pointer-type variables on which this gadget performs @@ -164,53 +164,55 @@ /// of all DeclRefExprs in the gadget's AST! virtual DeclUseList getClaimedVarUseSites() const = 0; - /// Returns a fixit that would fix the current gadget according to - /// the current strategy. Returns None if the fix cannot be produced; - /// returns an empty list if no fixes are necessary. - virtual std::optional getFixits(const Strategy &) const { - return std::nullopt; - } - virtual ~Gadget() = default; private: Kind K; }; -using GadgetList = std::vector>; -/// Unsafe gadgets correspond to unsafe code patterns that warrants +/// Warning gadgets correspond to unsafe code patterns that warrants /// an immediate warning. -class UnsafeGadget : public Gadget { +class WarningGadget : public Gadget { public: - UnsafeGadget(Kind K) : Gadget(K) {} + WarningGadget(Kind K) : Gadget(K) {} - static bool classof(const Gadget *G) { return G->isSafe(); } - bool isSafe() const final { return false; } + static bool classof(const Gadget *G) { return G->isWarningGadget(); } + bool isWarningGadget() const final { return true; } }; -/// Safe gadgets correspond to code patterns that aren't unsafe but need to be -/// properly recognized in order to emit correct warnings and fixes over unsafe -/// gadgets. For example, if a raw pointer-type variable is replaced by -/// a safe C++ container, every use of such variable may need to be +/// 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 SafeGadget : public Gadget { +class FixableGadget : public Gadget { public: - SafeGadget(Kind K) : Gadget(K) {} + FixableGadget(Kind K) : Gadget(K) {} + + static bool classof(const Gadget *G) { return !G->isWarningGadget(); } + bool isWarningGadget() const final { return false; } + + /// Returns a fixit that would fix the current gadget according to + /// the current strategy. Returns None if the fix cannot be produced; + /// returns an empty list if no fixes are necessary. + virtual Optional getFixits(const Strategy &) const { + return None; + } - static bool classof(const Gadget *G) { return !G->isSafe(); } - bool isSafe() const final { return true; } }; +using FixableGadgetList = std::vector>; +using WarningGadgetList = std::vector>; + /// An increment of a pointer-type value is unsafe as it may run the pointer /// out of bounds. -class IncrementGadget : public UnsafeGadget { +class IncrementGadget : public WarningGadget { static constexpr const char *const OpTag = "op"; const UnaryOperator *Op; public: IncrementGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::Increment), + : WarningGadget(Kind::Increment), Op(Result.Nodes.getNodeAs(OpTag)) {} static bool classof(const Gadget *G) { @@ -239,13 +241,13 @@ /// A decrement of a pointer-type value is unsafe as it may run the pointer /// out of bounds. -class DecrementGadget : public UnsafeGadget { +class DecrementGadget : public WarningGadget { static constexpr const char *const OpTag = "op"; const UnaryOperator *Op; public: DecrementGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::Decrement), + : WarningGadget(Kind::Decrement), Op(Result.Nodes.getNodeAs(OpTag)) {} static bool classof(const Gadget *G) { @@ -273,13 +275,13 @@ /// Array subscript expressions on raw pointers as if they're arrays. Unsafe as /// it doesn't have any bounds checks for the array. -class ArraySubscriptGadget : public UnsafeGadget { +class ArraySubscriptGadget : public WarningGadget { static constexpr const char *const ArraySubscrTag = "arraySubscr"; const ArraySubscriptExpr *ASE; public: ArraySubscriptGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::ArraySubscript), + : WarningGadget(Kind::ArraySubscript), ASE(Result.Nodes.getNodeAs(ArraySubscrTag)) {} static bool classof(const Gadget *G) { @@ -310,7 +312,7 @@ /// \code /// ptr + n | n + ptr | ptr - n | ptr += n | ptr -= n /// \endcode -class PointerArithmeticGadget : public UnsafeGadget { +class PointerArithmeticGadget : public WarningGadget { static constexpr const char *const PointerArithmeticTag = "ptrAdd"; static constexpr const char *const PointerArithmeticPointerTag = "ptrAddPtr"; const BinaryOperator *PA; // pointer arithmetic expression @@ -318,7 +320,7 @@ public: PointerArithmeticGadget(const MatchFinder::MatchResult &Result) - : UnsafeGadget(Kind::PointerArithmetic), + : WarningGadget(Kind::PointerArithmetic), PA(Result.Nodes.getNodeAs(PointerArithmeticTag)), Ptr(Result.Nodes.getNodeAs(PointerArithmeticPointerTag)) {} @@ -452,10 +454,11 @@ } // namespace /// Scan the function and return a list of gadgets found with provided kits. -static std::pair findGadgets(const Decl *D) { +static std::tuple findGadgets(const Decl *D) { struct GadgetFinderCallback : MatchFinder::MatchCallback { - GadgetList Gadgets; + FixableGadgetList FixableGadgets; + WarningGadgetList WarningGadgets; DeclUseTracker Tracker; void run(const MatchFinder::MatchResult &Result) override { @@ -481,9 +484,15 @@ // Figure out which matcher we've found, and call the appropriate // subclass constructor. // FIXME: Can we do this more logarithmically? -#define GADGET(name) \ +#define FIXABLE_GADGET(name) \ if (Result.Nodes.getNodeAs(#name)) { \ - Gadgets.push_back(std::make_unique(Result)); \ + 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; \ } #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def" @@ -528,13 +537,13 @@ // Gadgets "claim" variables they're responsible for. Once this loop finishes, // the tracker will only track DREs that weren't claimed by any gadgets, // i.e. not understood by the analysis. - for (const auto &G : CB.Gadgets) { + for (const auto &G : CB.FixableGadgets) { for (const auto *DRE : G->getClaimedVarUseSites()) { CB.Tracker.claimUse(DRE); } } - return {std::move(CB.Gadgets), std::move(CB.Tracker)}; + return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; } void clang::checkUnsafeBufferUsage(const Decl *D, @@ -543,25 +552,37 @@ SmallSet WarnedDecls; - auto [Gadgets, Tracker] = findGadgets(D); + auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D); - DenseMap> Map; + DenseMap, + std::vector>> Map; // First, let's sort gadgets by variables. If some gadgets cover more than one // variable, they'll appear more than once in the map. - for (const auto &G : Gadgets) { + for (const auto &G : FixableGadgets) { + DeclUseList DREs = G->getClaimedVarUseSites(); + + // Populate the map. + for (const DeclRefExpr *DRE : DREs) { + if (const auto *VD = dyn_cast(DRE->getDecl())) { + Map[VD].first.push_back(G.get()); + } + } + } + + for (const auto &G : WarningGadgets) { DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites(); // Populate the map. bool Pushed = false; for (const DeclRefExpr *DRE : ClaimedVarUseSites) { if (const auto *VD = dyn_cast(DRE->getDecl())) { - Map[VD].push_back(G.get()); + Map[VD].second.push_back(G.get()); Pushed = true; } } - if (!Pushed && !G->isSafe()) { + if (!Pushed) { // We won't return to this gadget later. Emit the warning right away. Handler.handleUnsafeOperation(G->getBaseStmt()); continue; @@ -570,10 +591,14 @@ Strategy S; - for (const auto &[VD, VDGadgets] : Map) { + for (const auto &Item : Map) { + + const VarDecl *VD = Item.first; + const std::vector &VDFixableGadgets = Item.second.first; + const std::vector &VDWarningGadgets = Item.second.second; // If the variable has no unsafe gadgets, skip it entirely. - if (!any_of(VDGadgets, [](const Gadget *G) { return !G->isSafe(); })) + if (VDWarningGadgets.empty()) continue; std::optional Fixes; @@ -593,7 +618,7 @@ // to undo the previous fix first, and then if we can't produce the new // fix for both variables, revert to the old one. Fixes = FixItList{}; - for (const Gadget *G : VDGadgets) { + for (const FixableGadget *G : VDFixableGadgets) { std::optional F = G->getFixits(S); if (!F) { Fixes = std::nullopt; @@ -611,10 +636,8 @@ } else { // The strategy has failed. Emit the warning without the fixit. S.set(VD, Strategy::Kind::Wontfix); - for (const Gadget *G : VDGadgets) { - if (!G->isSafe()) { - Handler.handleUnsafeOperation(G->getBaseStmt()); - } + for (const WarningGadget *G : VDWarningGadgets) { + Handler.handleUnsafeOperation(G->getBaseStmt()); } } }