This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG50d4a1f70e11: [-Wunsafe-buffer-usage] Safe-buffers
re-architecture to introduce Fixable… (authored by malavikasamak).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Changed prior to commit:
https://reviews.llvm.org/D140062?vs=486625&id=486952#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140062/new/
https://reviews.llvm.org/D140062
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/lib/Analysis/UnsafeBufferUsage.cpp
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ 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<FixItList> getFixits(const Strategy &) const {
- return std::nullopt;
- }
-
virtual ~Gadget() = default;
private:
Kind K;
};
-using GadgetList = std::vector<std::unique_ptr<Gadget>>;
-/// 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<FixItList> 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<std::unique_ptr<FixableGadget>>;
+using WarningGadgetList = std::vector<std::unique_ptr<WarningGadget>>;
+
/// 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<UnaryOperator>(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<UnaryOperator>(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<ArraySubscriptExpr>(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<BinaryOperator>(PointerArithmeticTag)),
Ptr(Result.Nodes.getNodeAs<Expr>(PointerArithmeticPointerTag)) {}
@@ -452,10 +454,11 @@
} // namespace
/// Scan the function and return a list of gadgets found with provided kits.
-static std::pair<GadgetList, DeclUseTracker> findGadgets(const Decl *D) {
+static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker> 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<Stmt>(#name)) { \
- Gadgets.push_back(std::make_unique<name ## Gadget>(Result)); \
+ FixableGadgets.push_back(std::make_unique<name ## Gadget>(Result)); \
+ NEXT; \
+ }
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+#define WARNING_GADGET(name) \
+ if (Result.Nodes.getNodeAs<Stmt>(#name)) { \
+ WarningGadgets.push_back(std::make_unique<name ## Gadget>(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<const VarDecl *, 8> WarnedDecls;
- auto [Gadgets, Tracker] = findGadgets(D);
+ auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D);
- DenseMap<const VarDecl *, std::vector<const Gadget *>> Map;
+ DenseMap<const VarDecl *, std::pair<std::vector<const FixableGadget *>,
+ std::vector<const WarningGadget *>>> 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<VarDecl>(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<VarDecl>(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<const FixableGadget *> &VDFixableGadgets = Item.second.first;
+ const std::vector<const WarningGadget *> &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<FixItList> 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<FixItList> 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());
}
}
}
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
