diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -37,6 +37,15 @@ /// 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 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -30,6 +30,7 @@ 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 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11790,6 +11790,8 @@ InGroup, 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 --- 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 #include @@ -115,6 +116,19 @@ MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); return Visitor.findMatch(DynTypedNode::create(Node)); } + +AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher, 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 +isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) { + return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue), + castSubExpr(innerMatcher)); + // FIXME: add assignmentTo context... +} } // namespace clang::ast_matchers namespace { @@ -282,7 +296,7 @@ /// 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 @@ 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(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 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(Node->getBase()->IgnoreImpCasts())) { + return {DRE}; + } + return {}; + } +}; } // namespace namespace { @@ -546,26 +602,30 @@ // 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,241 @@ return FixablesForUnsafeVars; } +std::optional +ULCArraySubscriptGadget::getFixits(const Strategy &S) const { + if (const auto *DRE = dyn_cast(Node->getBase()->IgnoreImpCasts())) + if (const auto *VD = dyn_cast(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 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 +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 +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`. +// +// 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 different cases: + if (auto CxxNew = dyn_cast(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(Init->IgnoreImpCasts())) + if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf && + isa_and_present(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` type and transforms the initializer, if present, to a span +// constructor---`span 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!"); + } +} + +// 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 -getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S) { +getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, + const DeclUseTracker &Tracker, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { std::map 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 FixItsForVD; for (const auto &F : Fixables) { @@ -657,6 +947,10 @@ 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 +996,8 @@ Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); std::map 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 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2200,13 +2200,18 @@ // 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 --- /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 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 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 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 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 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 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 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 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 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 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 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 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 p" + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]] + Int_ptr_t q; + // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span 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 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 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 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 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 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 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 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -80,20 +80,24 @@ 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}} + 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@-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 @@ 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}} }