[clang] d5c4283 - Revert "[-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `&DRE.data()[any]`"
David Spickett via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 5 01:07:51 PDT 2023
Author: David Spickett
Date: 2023-04-05T08:07:19Z
New Revision: d5c428356f6ee107a97977eb9ef1aa4d5fa0c378
URL: https://github.com/llvm/llvm-project/commit/d5c428356f6ee107a97977eb9ef1aa4d5fa0c378
DIFF: https://github.com/llvm/llvm-project/commit/d5c428356f6ee107a97977eb9ef1aa4d5fa0c378.diff
LOG: Revert "[-Wunsafe-buffer-usage] Fix-Its transforming `&DRE[any]` to `&DRE.data()[any]`"
This reverts commit 87b5807d3802b932c06d83c4287014872aa2caab.
The test case is failing on Windows https://lab.llvm.org/buildbot/#/builders/65/builds/8950.
Added:
Modified:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/lib/Analysis/UnsafeBufferUsage.cpp
Removed:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index f78cf2c57689c..8957ab664ed93 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -43,7 +43,7 @@ class UnsafeBufferUsageHandler {
/// Returns the text indicating that the user needs to provide input there:
virtual std::string
- getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const {
+ getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") {
std::string s = std::string("<# ");
s += HintTextToUser;
s += " #>";
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index f0c99a767071f..a8485682c1d1f 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -30,10 +30,9 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
-FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context
+FIXABLE_GADGET(ULCArraySubscript)
FIXABLE_GADGET(DerefSimplePtrArithFixable)
FIXABLE_GADGET(PointerDereference)
-FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context
#undef FIXABLE_GADGET
#undef WARNING_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 1ef276ab90444..4a8358af68ec5 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -15,7 +15,6 @@
#include "llvm/ADT/SmallVector.h"
#include <memory>
#include <optional>
-#include <sstream>
using namespace llvm;
using namespace clang;
@@ -113,15 +112,6 @@ class MatchDescendantVisitor
bool Matches;
};
-// 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()));
-}
-
AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
const DynTypedMatcher &DTM = static_cast<DynTypedMatcher>(innerMatcher);
@@ -155,30 +145,6 @@ static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
));
// clang-format off
}
-
-
-// Returns a matcher that matches any expression `e` such that `InnerMatcher`
-// matches `e` and `e` is in an Unspecified Pointer Context (UPC).
-static internal::Matcher<Stmt>
-isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
- // A UPC can be
- // 1. an argument of a function call (except the callee has [[unsafe_...]]
- // 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 CastOperandMatcher =
- explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral),
- castSubExpr(allOf(hasPointerType(), InnerMatcher)));
-
- return stmt(anyOf(CallArgMatcher, CastOperandMatcher));
- // 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 {
@@ -193,6 +159,15 @@ using FixItList = SmallVector<FixItHint, 4>;
class Strategy;
} // namespace
+// 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()));
+}
+
namespace {
/// Gadget is an individual operation in the code that may be of interest to
/// this analysis. Each (non-abstract) subclass corresponds to a specific
@@ -527,46 +502,6 @@ class PointerDereferenceGadget : public FixableGadget {
virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
};
-// Represents expressions of the form `&DRE[any]` in the Unspecified Pointer
-// Context (see `isInUnspecifiedPointerContext`).
-// Note here `[]` is the built-in subscript operator.
-class UPCAddressofArraySubscriptGadget : public FixableGadget {
-private:
- static constexpr const char *const UPCAddressofArraySubscriptTag =
- "AddressofArraySubscriptUnderUPC";
- const UnaryOperator *Node; // the `&DRE[any]` node
-
-public:
- UPCAddressofArraySubscriptGadget(const MatchFinder::MatchResult &Result)
- : FixableGadget(Kind::ULCArraySubscript),
- Node(Result.Nodes.getNodeAs<UnaryOperator>(
- UPCAddressofArraySubscriptTag)) {
- assert(Node != nullptr && "Expecting a non-null matching result");
- }
-
- static bool classof(const Gadget *G) {
- return G->getKind() == Kind::UPCAddressofArraySubscript;
- }
-
- static Matcher matcher() {
- return expr(isInUnspecifiedPointerContext(expr(ignoringImpCasts(
- unaryOperator(hasOperatorName("&"),
- hasUnaryOperand(arraySubscriptExpr(
- hasBase(ignoringParenImpCasts(declRefExpr())))))
- .bind(UPCAddressofArraySubscriptTag)))));
- }
-
- virtual std::optional<FixItList> getFixits(const Strategy &) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
-
- virtual DeclUseList getClaimedVarUseSites() const override {
- const auto *ArraySubst = cast<ArraySubscriptExpr>(Node->getSubExpr());
- const auto *DRE =
- cast<DeclRefExpr>(ArraySubst->getBase()->IgnoreImpCasts());
- return {DRE};
- }
-};
} // namespace
namespace {
@@ -775,7 +710,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler) {
// We also should make sure no two `FixableGadget` (resp. `WarningGadget`) matchers
// match for the same node, so that we can group them
// in one `anyOf` group (for better performance via short-circuiting).
- stmt(eachOf(
+ stmt(anyOf(
#define FIXABLE_GADGET(x) \
x ## Gadget::matcher().bind(#x),
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
@@ -934,26 +869,6 @@ ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
return std::nullopt;
}
-static std::optional<FixItList> // forward declaration
-fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node);
-
-std::optional<FixItList>
-UPCAddressofArraySubscriptGadget::getFixits(const Strategy &S) const {
- auto DREs = getClaimedVarUseSites();
- const auto *VD = cast<VarDecl>(DREs.front()->getDecl());
-
- switch (S.lookup(VD)) {
- case Strategy::Kind::Span:
- return fixUPCAddressofArraySubscriptWithSpan(Node);
- 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; // something went wrong, no fix-it
-}
-
// Return the text representation of the given `APInt Val`:
static std::string getAPIntText(APInt Val) {
SmallVector<char> Txt;
@@ -1070,35 +985,6 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
return std::nullopt;
}
-// Generates fix-its replacing an expression of the form `&DRE[e]` with
-// `&DRE.data()[e]`:
-static std::optional<FixItList>
-fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
- const auto *ArraySub = cast<ArraySubscriptExpr>(Node->getSubExpr());
- const auto *DRE = cast<DeclRefExpr>(ArraySub->getBase()->IgnoreImpCasts());
- // FIXME: this `getASTContext` call is costly, we should pass the
- // ASTContext in:
- const ASTContext &Ctx = DRE->getDecl()->getASTContext();
- const Expr *Idx = ArraySub->getIdx();
- const SourceManager &SM = Ctx.getSourceManager();
- const LangOptions &LangOpts = Ctx.getLangOpts();
- std::stringstream SS;
- bool IdxIsLitZero = false;
-
- if (auto ICE = Idx->getIntegerConstantExpr(Ctx))
- if ((*ICE).isZero())
- IdxIsLitZero = true;
- if (IdxIsLitZero) {
- // If the index is literal zero, we produce the most concise fix-it:
- SS << getExprText(DRE, SM, LangOpts).str() << ".data()";
- } else {
- SS << "&" << getExprText(DRE, SM, LangOpts).str() << ".data()"
- << "[" << getExprText(Idx, SM, LangOpts).str() << "]";
- }
- return FixItList{
- FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())};
-}
-
// 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.
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
deleted file mode 100644
index db72124741e3c..0000000000000
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
+++ /dev/null
@@ -1,83 +0,0 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
-
-int f(unsigned long, void *);
-
-[[clang::unsafe_buffer_usage]]
-int unsafe_f(unsigned long, void *);
-
-void address_to_integer(int x) {
- int * p = new int[10];
- unsigned long n = (unsigned long) &p[5];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"&p.data()[5]"
- unsigned long m = (unsigned long) &p[x];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"&p.data()[x]"
-}
-
-void call_argument(int x) {
- int * p = new int[10];
-
- f((unsigned long) &p[5], &p[x]);
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:21-[[@LINE-1]]:26}:"&p.data()[5]"
- // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:28-[[@LINE-2]]:33}:"&p.data()[x]"
-}
-
-void ignore_unsafe_calls(int x) {
- // Cannot fix `&p[x]` for now as it is an argument of an unsafe
- // call. So no fix for variable `p`.
- int * p = new int[10];
- // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
- unsafe_f((unsigned long) &p[5],
- // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
- &p[x]);
-
- int * q = new int[10];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
- // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
- unsafe_f((unsigned long) &q[5],
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:28-[[@LINE-1]]:33}:"&q.data()[5]"
- (void*)0);
-}
-
-void odd_subscript_form() {
- int * p = new int[10];
- unsigned long n = (unsigned long) &5[p];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:42}:"&p.data()[5]"
-}
-
-void index_is_zero() {
- int * p = new int[10];
- int n = p[5];
-
- f((unsigned long)&p[0],
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:20-[[@LINE-1]]:25}:"p.data()"
- &p[0]);
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:10}:"p.data()"
-}
-
-// CHECK-NOT: fix-it
-// To test multiple function declarations, each of which carries
-//
diff erent incomplete informations:
-[[clang::unsafe_buffer_usage]]
-void unsafe_g(void*);
-
-void unsafe_g(void*);
-
-void multiple_unsafe_fundecls() {
- int * p = new int[10];
-
- unsafe_g(&p[5]);
-}
-
-void unsafe_h(void*);
-
-[[clang::unsafe_buffer_usage]]
-void unsafe_h(void*);
-
-void unsafe_h(void* p) { ((char*)p)[10]; }
-
-void multiple_unsafe_fundecls2() {
- int * p = new int[10];
-
- unsafe_h(&p[5]);
-}
More information about the cfe-commits
mailing list