[clang] bdf4f2b - [-Wunsafe-buffer-usage] Generate fix-it for local variable declarations
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 7 15:43:56 PST 2023
Author: Ziqing Luo
Date: 2023-02-07T15:40:19-08:00
New Revision: bdf4f2bea50e87f5b9273e3bbc9a7753bca3a6bb
URL: https://github.com/llvm/llvm-project/commit/bdf4f2bea50e87f5b9273e3bbc9a7753bca3a6bb
DIFF: https://github.com/llvm/llvm-project/commit/bdf4f2bea50e87f5b9273e3bbc9a7753bca3a6bb.diff
LOG: [-Wunsafe-buffer-usage] Generate fix-it for local variable declarations
Use clang fix-its to transform declarations of local variables, which
are used for buffer access , to be of std::span type.
We placed a few limitations to keep the solution simple:
- it only transforms local variable declarations (no parameter declaration);
- it only considers single level pointers, i.e., pointers of type T * regardless of whether T is again a pointer;
- it only transforms to std::span types (no std::array, or std::span::iterator, or ...);
- it can only transform a VarDecl that belongs to a DeclStmt whose has a single child.
One of the purposes of keeping this patch simple enough is to first
evaluate if fix-it is an appropriate approach to do the
transformation.
This commit was reverted by 622be09c815266632e204eaf1c7a35f050220459
for a compilation warning and now it is fixed.
Reviewed by: NoQ, jkorous
Differential revision: https://reviews.llvm.org/D139737
Added:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
Modified:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index e3f87cd0f3660..04c9fdea444f8 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -37,6 +37,15 @@ class UnsafeBufferUsageHandler {
/// Invoked when a fix is suggested against a variable.
virtual void handleFixableVariable(const VarDecl *Variable,
FixItList &&List) = 0;
+
+ /// Returns the text indicating that the user needs to provide input there:
+ virtual std::string
+ getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") {
+ std::string s = std::string("<# ");
+ s += HintTextToUser;
+ s += " #>";
+ return s;
+ }
};
// This function invokes the analysis and allows the caller to react to it
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 78889da32b3b4..75657d8d9a584 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -30,6 +30,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+FIXABLE_GADGET(ULCArraySubscript)
#undef FIXABLE_GADGET
#undef WARNING_GADGET
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index bc70dbf4a624d..9b9ec23a2f21f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11790,6 +11790,8 @@ def warn_unsafe_buffer_operation : Warning<
InGroup<UnsafeBufferUsage>, DefaultIgnore;
def note_unsafe_buffer_operation : Note<
"used%select{| in pointer arithmetic| in buffer access}0 here">;
+def note_unsafe_buffer_variable_fixit : Note<
+ "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information">;
def err_loongarch_builtin_requires_la32 : Error<
"this builtin requires target: loongarch32">;
} // end of sema component.
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 331479a80f6cc..c4f0a13a5d51e 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -9,6 +9,7 @@
#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
#include "llvm/ADT/SmallVector.h"
#include <memory>
#include <optional>
@@ -115,6 +116,19 @@ AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All);
return Visitor.findMatch(DynTypedNode::create(Node));
}
+
+AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
+ return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
+}
+
+// Returns a matcher that matches any expression 'e' such that `innerMatcher`
+// matches 'e' and 'e' is in an Unspecified Lvalue Context.
+static internal::Matcher<Stmt>
+isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
+ return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
+ castSubExpr(innerMatcher));
+ // FIXME: add assignmentTo context...
+}
} // namespace clang::ast_matchers
namespace {
@@ -282,7 +296,7 @@ class DecrementGadget : public WarningGadget {
/// 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 WarningGadget {
- static constexpr const char *const ArraySubscrTag = "arraySubscr";
+ static constexpr const char *const ArraySubscrTag = "ArraySubscript";
const ArraySubscriptExpr *ASE;
public:
@@ -393,6 +407,48 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
return {};
}
};
+
+
+// Represents expressions of the form `DRE[*]` in the Unspecified Lvalue
+// Context (see `isInUnspecifiedLvalueContext`).
+// Note here `[]` is the built-in subscript operator.
+class ULCArraySubscriptGadget : public FixableGadget {
+private:
+ static constexpr const char *const ULCArraySubscriptTag = "ArraySubscriptUnderULC";
+ const ArraySubscriptExpr *Node;
+
+public:
+ ULCArraySubscriptGadget(const MatchFinder::MatchResult &Result)
+ : FixableGadget(Kind::ULCArraySubscript),
+ Node(Result.Nodes.getNodeAs<ArraySubscriptExpr>(ULCArraySubscriptTag)) {
+ assert(Node != nullptr && "Expecting a non-null matching result");
+ }
+
+ static bool classof(const Gadget *G) {
+ return G->getKind() == Kind::ULCArraySubscript;
+ }
+
+ static Matcher matcher() {
+ auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType());
+ auto BaseIsArrayOrPtrDRE =
+ hasBase(ignoringParenImpCasts(declRefExpr(ArrayOrPtr)));
+ auto Target =
+ arraySubscriptExpr(BaseIsArrayOrPtrDRE).bind(ULCArraySubscriptTag);
+
+ return expr(isInUnspecifiedLvalueContext(Target));
+ }
+
+ virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+
+ virtual const Stmt *getBaseStmt() const override { return Node; }
+
+ virtual DeclUseList getClaimedVarUseSites() const override {
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
+ return {DRE};
+ }
+ return {};
+ }
+};
} // namespace
namespace {
@@ -546,26 +602,30 @@ static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker> findGadg
// clang-format off
M.addMatcher(
stmt(forEveryDescendant(
+ eachOf(
+ // A `FixableGadget` matcher and a `WarningGadget` matcher should not disable
+ // each other (they could if they were put in the same `anyOf` group).
+ // 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(anyOf(
- // Add Gadget::matcher() for every gadget in the registry.
-#define GADGET(x) \
+#define FIXABLE_GADGET(x) \
x ## Gadget::matcher().bind(#x),
#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
- // In parallel, match all DeclRefExprs so that to find out
- // whether there are any uncovered by gadgets.
- declRefExpr(anyOf(hasPointerType(), hasArrayType()),
- to(varDecl())).bind("any_dre"),
// Also match DeclStmts because we'll need them when fixing
// their underlying VarDecls that otherwise don't have
// any backreferences to DeclStmts.
declStmt().bind("any_ds")
- ))
- // FIXME: Idiomatically there should be a forCallable(equalsNode(D))
- // here, to make sure that the statement actually belongs to the
- // function and not to a nested function. However, forCallable uses
- // ParentMap which can't be used before the AST is fully constructed.
- // The original problem doesn't sound like it needs ParentMap though,
- // maybe there's a more direct solution?
+ )),
+ stmt(anyOf(
+ // Add Gadget::matcher() for every gadget in the registry.
+#define WARNING_GADGET(x) \
+ x ## Gadget::matcher().bind(#x),
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+ // In parallel, match all DeclRefExprs so that to find out
+ // whether there are any uncovered by gadgets.
+ declRefExpr(anyOf(hasPointerType(), hasArrayType()), to(varDecl())).bind("any_dre")
+ )))
)),
&CB
);
@@ -634,11 +694,242 @@ groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
return FixablesForUnsafeVars;
}
+std::optional<FixItList>
+ULCArraySubscriptGadget::getFixits(const Strategy &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: {
+ // If the index has a negative constant value, we give up as no valid
+ // fix-it can be generated:
+ const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in!
+ VD->getASTContext();
+ if (auto ConstVal = Node->getIdx()->getIntegerConstantExpr(Ctx)) {
+ if (ConstVal->isNegative())
+ return std::nullopt;
+ } else if (!Node->getIdx()->getType()->isUnsignedIntegerType())
+ return std::nullopt;
+ // 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:
+ llvm_unreachable("unsupported strategies for FixableGadgets");
+ }
+ }
+ return std::nullopt;
+}
+
+// Return the text representation of the given `APInt Val`:
+static std::string getAPIntText(APInt Val) {
+ SmallVector<char> Txt;
+ Val.toString(Txt, 10, true);
+ // APInt::toString does not add '\0' to the end of the string for us:
+ Txt.push_back('\0');
+ return Txt.data();
+}
+
+// Return the source location of the last character of the AST `Node`.
+template <typename NodeTy>
+static SourceLocation getEndCharLoc(const NodeTy *Node, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ return Lexer::getLocForEndOfToken(Node->getEndLoc(), 1, SM, LangOpts);
+}
+
+// Return the source location just past the last character of the AST `Node`.
+template <typename NodeTy>
+static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ return Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts);
+}
+
+// Return text representation of an `Expr`.
+static StringRef getExprText(const Expr *E, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ SourceLocation LastCharLoc = getPastLoc(E, SM, LangOpts);
+
+ return Lexer::getSourceText(
+ CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM,
+ LangOpts);
+}
+
+// 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.
+// In many cases, this function cannot figure out the actual extent `S`. It
+// then will use a place holder to replace `S` to ask users to fill `S` in. The
+// initializer shall be used to initialize a variable of type `std::span<T>`.
+//
+// FIXME: Support multi-level pointers
+//
+// Parameters:
+// `Init` a pointer to the initializer expression
+// `Ctx` a reference to the ASTContext
+static FixItList
+populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx,
+ const StringRef UserFillPlaceHolder) {
+ FixItList FixIts{};
+ const SourceManager &SM = Ctx.getSourceManager();
+ const LangOptions &LangOpts = Ctx.getLangOpts();
+ std::string ExtentText = UserFillPlaceHolder.data();
+ StringRef One = "1";
+
+ // Insert `{` before `Init`:
+ FixIts.push_back(FixItHint::CreateInsertion(Init->getBeginLoc(), "{"));
+ // Try to get the data extent. Break into
diff erent cases:
+ if (auto CxxNew = dyn_cast<CXXNewExpr>(Init->IgnoreImpCasts())) {
+ // In cases `Init` is `new T[n]` and there is no explicit cast over
+ // `Init`, we know that `Init` must evaluates to a pointer to `n` objects
+ // of `T`. So the extent is `n` unless `n` has side effects. Similar but
+ // simpler for the case where `Init` is `new T`.
+ if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) {
+ if (!Ext->HasSideEffects(Ctx))
+ ExtentText = getExprText(Ext, SM, LangOpts);
+ } else if (!CxxNew->isArray())
+ // Although the initializer is not allocating a buffer, the pointer
+ // variable could still be used in buffer access operations.
+ ExtentText = One;
+ } else if (const auto *CArrTy = Ctx.getAsConstantArrayType(
+ Init->IgnoreImpCasts()->getType())) {
+ // In cases `Init` is of an array type after stripping off implicit casts,
+ // the extent is the array size. Note that if the array size is not a
+ // constant, we cannot use it as the extent.
+ ExtentText = getAPIntText(CArrTy->getSize());
+ } else {
+ // In cases `Init` is of the form `&Var` after stripping of implicit
+ // casts, where `&` is the built-in operator, the extent is 1.
+ if (auto AddrOfExpr = dyn_cast<UnaryOperator>(Init->IgnoreImpCasts()))
+ if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf &&
+ isa_and_present<DeclRefExpr>(AddrOfExpr->getSubExpr()))
+ ExtentText = One;
+ // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, `std::addressof`, and explicit casting, etc.
+ // etc.
+ }
+
+ SmallString<32> StrBuffer{};
+ SourceLocation LocPassInit = getPastLoc(Init, SM, LangOpts);
+
+ StrBuffer.append(", ");
+ StrBuffer.append(ExtentText);
+ StrBuffer.append("}");
+ FixIts.push_back(FixItHint::CreateInsertion(LocPassInit, StrBuffer.str()));
+ return FixIts;
+}
+
+// For a `VarDecl` of the form `T * var (= Init)?`, this
+// function generates a fix-it for the declaration, which re-declares `var` to
+// be of `span<T>` type and transforms the initializer, if present, to a span
+// constructor---`span<T> var {Init, Extent}`, where `Extent` may need the user
+// to fill in.
+//
+// FIXME: support Multi-level pointers
+//
+// Parameters:
+// `D` a pointer the variable declaration node
+// `Ctx` a reference to the ASTContext
+// Returns:
+// the generated fix-it
+static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx,
+ const StringRef UserFillPlaceHolder) {
+ const QualType &SpanEltT = D->getType()->getPointeeType();
+ assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
+
+ FixItList FixIts{};
+ SourceLocation
+ ReplacementLastLoc; // the inclusive end location of the replacement
+ const SourceManager &SM = Ctx.getSourceManager();
+
+ if (const Expr *Init = D->getInit()) {
+ FixItList InitFixIts =
+ populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);
+
+ if (InitFixIts.empty())
+ return {}; // Something wrong with fixing initializer, give up
+ // The loc right before the initializer:
+ ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
+ FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
+ std::make_move_iterator(InitFixIts.end()));
+ } else
+ ReplacementLastLoc = getEndCharLoc(D, SM, Ctx.getLangOpts());
+
+ // Producing fix-it for variable declaration---make `D` to be of span type:
+ SmallString<32> Replacement;
+ raw_svector_ostream OS(Replacement);
+
+ OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
+ FixIts.push_back(FixItHint::CreateReplacement(
+ SourceRange{D->getBeginLoc(), ReplacementLastLoc}, OS.str()));
+ return FixIts;
+}
+
+static FixItList fixVariableWithSpan(const VarDecl *VD,
+ const DeclUseTracker &Tracker,
+ const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
+ const DeclStmt *DS = Tracker.lookupDecl(VD);
+ assert(DS && "Fixing non-local variables not implemented yet!");
+ if (!DS->isSingleDecl()) {
+ // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
+ return{};
+ }
+ // Currently DS is an unused variable but we'll need it when
+ // non-single decls are implemented, where the pointee type name
+ // and the '*' are spread around the place.
+ (void)DS;
+
+ // FIXME: handle cases where DS has multiple declarations
+ return fixVarDeclWithSpan(VD, Ctx, Handler.getUserFillPlaceHolder());
+}
+
+static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K,
+ const DeclUseTracker &Tracker,
+ const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
+ switch (K) {
+ case Strategy::Kind::Span: {
+ if (VD->getType()->isPointerType() && VD->isLocalVarDecl())
+ return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+ return {};
+ }
+ case Strategy::Kind::Iterator:
+ case Strategy::Kind::Array:
+ case Strategy::Kind::Vector:
+ llvm_unreachable("Strategy not implemented yet!");
+ case Strategy::Kind::Wontfix:
+ llvm_unreachable("Invalid strategy!");
+ }
+ llvm_unreachable("Unknown strategy!");
+}
+
+// Returns true iff there exists a `FixItHint` 'h' in `FixIts` such that the
+// `RemoveRange` of 'h' overlaps with a macro use.
+static bool overlapWithMacro(const FixItList &FixIts) {
+ // FIXME: For now we only check if the range (or the first token) is (part of)
+ // a macro expansion. Ideally, we want to check for all tokens in the range.
+ return llvm::any_of(FixIts, [](const FixItHint &Hint) {
+ auto BeginLoc = Hint.RemoveRange.getBegin();
+ if (BeginLoc.isMacroID())
+ // If the range (or the first token) is (part of) a macro expansion:
+ return true;
+ return false;
+ });
+}
+
static std::map<const VarDecl *, FixItList>
-getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S) {
+getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
+ const DeclUseTracker &Tracker, const ASTContext &Ctx,
+ UnsafeBufferUsageHandler &Handler) {
std::map<const VarDecl *, FixItList> FixItsForVariable;
for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
- // TODO fixVariable - fixit for the variable itself
+ FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), Tracker, Ctx, Handler);
+ // If we fail to produce Fix-It for the declaration we have to skip the
+ // variable entirely.
+ if (FixItsForVariable[VD].empty()) {
+ FixItsForVariable.erase(VD);
+ continue;
+ }
bool ImpossibleToFix = false;
llvm::SmallVector<FixItHint, 16> FixItsForVD;
for (const auto &F : Fixables) {
@@ -657,6 +948,10 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S) {
else
FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
FixItsForVD.begin(), FixItsForVD.end());
+ // Fix-it shall not overlap with macros or/and templates:
+ if (overlapWithMacro(FixItsForVariable[VD])) {
+ FixItsForVariable.erase(VD);
+ }
}
return FixItsForVariable;
}
@@ -702,7 +997,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
std::map<const VarDecl *, FixItList> FixItsForVariable =
- getFixIts(FixablesForUnsafeVars, NaiveStrategy);
+ getFixIts(FixablesForUnsafeVars, NaiveStrategy, Tracker,
+ D->getASTContext(), Handler);
// FIXME Detect overlapping FixIts.
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index b6dce0808705e..c1969a0f85d7f 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2200,13 +2200,18 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
// FIXME: rename to handleUnsafeVariable
void handleFixableVariable(const VarDecl *Variable,
FixItList &&Fixes) override {
- const auto &D =
- S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable);
- D << Variable;
- D << (Variable->getType()->isPointerType() ? 0 : 1);
- D << Variable->getSourceRange();
- for (const auto &F : Fixes)
- D << F;
+ S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
+ << Variable << (Variable->getType()->isPointerType() ? 0 : 1)
+ << Variable->getSourceRange();
+ if (!Fixes.empty()) {
+ unsigned FixItStrategy = 0; // For now we only has 'std::span' strategy
+ const auto &FD = S.Diag(Variable->getLocation(),
+ diag::note_unsafe_buffer_variable_fixit);
+
+ FD << Variable->getName() << FixItStrategy;
+ for (const auto &F : Fixes)
+ FD << F;
+ }
}
};
} // namespace
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
new file mode 100644
index 0000000000000..4f0e0fa31a431
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+ int tmp;
+ int *p = new int[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+ const int *q = new int[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span<const int> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
+ tmp = p[5];
+ tmp = q[5];
+
+ Int_ptr_t x = new int[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> x"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+ Int_ptr_t y = new int;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> y"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
+ Int_t * z = new int[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> z"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
+ Int_t * w = new Int_t[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> w"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
+
+ tmp = x[5];
+ tmp = y[5]; // y[5] will crash after being span
+ tmp = z[5];
+ tmp = w[5];
+}
+
+void local_array_subscript_auto() {
+ int tmp;
+ auto p = new int[10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+ tmp = p[5];
+}
+
+void local_array_subscript_variable_extent() {
+ int n = 10;
+ int tmp;
+ int *p = new int[n];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", n}"
+ // If the extent expression does not have a constant value, we cannot fill the extent for users...
+ int *q = new int[n++];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
+ tmp = p[5];
+ tmp = q[5];
+}
+
+
+void local_ptr_to_array() {
+ int tmp;
+ int n = 10;
+ int a[10];
+ int b[n]; // If the extent expression does not have a constant value, we cannot fill the extent for users...
+ int *p = a;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", 10}"
+ int *q = b;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
+ // No way to know if `n` is ever mutated since `int b[n];`, so no way to figure out the extent
+ tmp = p[5];
+ tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+ int var;
+ int * q = &var;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:", 1}"
+ // This expression involves unsafe buffer accesses, which will crash
+ // at runtime after applying the fix-it,
+ var = q[5];
+}
+
+void decl_without_init() {
+ int tmp;
+ int * p;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> p"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]
+ Int_ptr_t q;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int> q"
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]
+ tmp = p[5];
+ tmp = q[5];
+}
+
+// Explicit casts are required in the following cases. No way to
+// figure out span extent for them automatically.
+void explict_cast() {
+ int tmp;
+ int * p = (int*) new int[10][10];
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:35-[[@LINE-3]]:35}:", <# placeholder #>}"
+ tmp = p[5];
+
+ int a;
+ char * q = (char *)&a;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<char> q"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
+ tmp = (int) q[5];
+
+ void * r = &a;
+ char * s = (char *) r;
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:13}:"std::span<char> s"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:14-[[@LINE-2]]:14}:"{"
+ // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}"
+ tmp = (int) s[5];
+}
+
+void unsupported_multi_decl(int * x) {
+ int * p = x, * q = new int[10];
+ // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]
+ *p = q[5];
+}
+
+void unsupported_fixit_overlapping_macro(int * x) {
+ int tmp;
+ // In the case below, a tentative fix-it replaces `MY_INT * p =` with `std::span<MY_INT> p `.
+ // It overlaps with the use of the macro `MY_INT`. The fix-it is
+ // discarded then.
+#define MY_INT int
+ MY_INT * p = new int[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ tmp = p[5];
+
+#define MY_VAR(name) int * name
+ MY_VAR(q) = new int[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+ tmp = q[5];
+
+ // In cases where fix-its do not change the original code where
+ // macros are used, those fix-its will be emitted. For example,
+ // fixits are inserted before and after `new MY_INT[MY_TEN]` below.
+#define MY_TEN 10
+ int * g = new MY_INT[MY_TEN];
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> g"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:31-[[@LINE-3]]:31}:", MY_TEN}"
+ tmp = g[5];
+
+#undef MY_INT
+#undef MY_VAR
+#undef MY_TEN
+}
+
+void unsupported_subscript_negative(int i, unsigned j, unsigned long k) {
+ int tmp;
+ int * p = new int[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+
+ tmp = p[-1]; // If `p` is made a span, this `[]` operation is wrong,
+ // so no fix-it emitted.
+
+ int * q = new int[10];
+ // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+
+ tmp = q[5];
+ tmp = q[i]; // If `q` is made a span, this `[]` operation may be
+ // wrong as we do not know if `i` is non-negative, so
+ // no fix-it emitted.
+
+ int * r = new int[10];
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> r"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+
+ tmp = r[j] + r[k]; // both `j` and `k` are unsigned so they must be non-negative
+ tmp = r[(unsigned int)-1]; // a cast-to-unsigned-expression is also non-negative
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index a8ee6f58d140a..783c8ea865112 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -80,20 +80,24 @@ void testArraySubscripts(int *p, int **pp) {
void testArraySubscriptsWithAuto(int *p, int **pp) {
int a[10];
- auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+ auto ap1 = a; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
foo(ap1[1]); // expected-note{{used in buffer access here}}
- auto ap2 = p; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}}
+ auto ap2 = p; // expected-warning{{'ap2' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap2' to 'std::span' to preserve bounds information}}
foo(ap2[1]); // expected-note{{used in buffer access here}}
- auto ap3 = pp; // expected-warning{{'ap3' is an unsafe pointer used for buffer access}}
+ auto ap3 = pp; // expected-warning{{'ap3' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap3' to 'std::span' to preserve bounds information}}
foo(ap3[1][1]); // expected-note{{used in buffer access here}}
// expected-warning at -1{{unsafe buffer access}}
- auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}}
+ auto ap4 = *pp; // expected-warning{{'ap4' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap4' to 'std::span' to preserve bounds information}}
foo(ap4[1]); // expected-note{{used in buffer access here}}
}
@@ -355,7 +359,8 @@ void testMultiLineDeclStmt(int * p) {
auto
- ap1 = p; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}}
+ ap1 = p; // expected-warning{{'ap1' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'ap1' to 'std::span' to preserve bounds information}}
foo(ap1[1]); // expected-note{{used in buffer access here}}
}
More information about the cfe-commits
mailing list