[clang] 50d4a1f - [-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets
via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 6 11:45:30 PST 2023
Author: MalavikaSamak
Date: 2023-01-06T11:45:23-08:00
New Revision: 50d4a1f70e111cd41b1a94d95fd06b5691aa2643
URL: https://github.com/llvm/llvm-project/commit/50d4a1f70e111cd41b1a94d95fd06b5691aa2643
DIFF: https://github.com/llvm/llvm-project/commit/50d4a1f70e111cd41b1a94d95fd06b5691aa2643.diff
LOG: [-Wunsafe-buffer-usage] Safe-buffers re-architecture to introduce Fixable gadgets
Re-architecture of safe-buffers gadgets to re-classify them as warning and fixable
gadgets. The warning gadgets identify unsafe operations on buffer variables and
emit suitable warnings. While the fixable gadgets consider all operations on
variables identified by the warning gadgets and emit necessary fixits.
Differential Revision: https://reviews.llvm.org/D140062?id=486625
Added:
Modified:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/lib/Analysis/UnsafeBufferUsage.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index f24e925966094..d10d95e5b1ba7 100644
--- 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
index 80a54c3ad38b7..d5423d81ee309 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -137,9 +137,9 @@ namespace {
/// 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 @@ class Gadget {
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 @@ class Gadget {
/// 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 @@ class IncrementGadget : public UnsafeGadget {
/// 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 @@ class DecrementGadget : public UnsafeGadget {
/// 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 @@ class ArraySubscriptGadget : public UnsafeGadget {
/// \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 @@ class PointerArithmeticGadget : public UnsafeGadget {
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 @@ class Strategy {
} // 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 @@ static std::pair<GadgetList, DeclUseTracker> findGadgets(const Decl *D) {
// 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 @@ static std::pair<GadgetList, DeclUseTracker> findGadgets(const Decl *D) {
// 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 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
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 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
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 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
// 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 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
} 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());
}
}
}
More information about the cfe-commits
mailing list