Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -35,6 +35,10 @@ /// variables, where `Var` is in, contains parameters. virtual VarGrpRef getGroupOfVar(const VarDecl *Var, bool *HasParm = nullptr) const; + + /// Returns the non-empty group of variables that include parameters of the + /// analyzing function, if such a group exists. An empty group, otherwise. + virtual VarGrpRef getGroupOfParms() const; }; /// The interface that lets the caller handle unsafe buffer usage analysis Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -13,6 +13,7 @@ #include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/iterator_range.h" #include #include #include @@ -1218,44 +1219,6 @@ return false; } -std::optional -PointerAssignmentGadget::getFixits(const Strategy &S) const { - const auto *LeftVD = cast(PtrLHS->getDecl()); - const auto *RightVD = cast(PtrRHS->getDecl()); - switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("unsupported strategies for FixableGadgets"); - } - return std::nullopt; -} - -std::optional -PointerInitGadget::getFixits(const Strategy &S) const { - const auto *LeftVD = PtrInitLHS; - const auto *RightVD = cast(PtrInitRHS->getDecl()); - switch (S.lookup(LeftVD)) { - case Strategy::Kind::Span: - if (S.lookup(RightVD) == Strategy::Kind::Span) - return FixItList{}; - return std::nullopt; - case Strategy::Kind::Wontfix: - return std::nullopt; - case Strategy::Kind::Iterator: - case Strategy::Kind::Array: - case Strategy::Kind::Vector: - llvm_unreachable("unsupported strategies for FixableGadgets"); - } - return std::nullopt; -} - std::optional ULCArraySubscriptGadget::getFixits(const Strategy &S) const { if (const auto *DRE = @@ -1530,6 +1493,59 @@ return SpanOpen + EltTyText.str() + '>'; } +std::optional +PointerAssignmentGadget::getFixits(const Strategy &S) const { + const auto *LeftVD = cast(PtrLHS->getDecl()); + const auto *RightVD = cast(PtrRHS->getDecl()); + switch (S.lookup(LeftVD)) { + case Strategy::Kind::Span: + if (S.lookup(RightVD) == Strategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case Strategy::Kind::Wontfix: { + if (S.lookup(RightVD) == Strategy::Kind::Span) { + const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in! + RightVD->getASTContext(); + if (auto EndLocOfRHS = + getPastLoc(PtrRHS, Ctx.getSourceManager(), Ctx.getLangOpts())) + return FixItList{FixItHint::CreateInsertion(*EndLocOfRHS, ".data()")}; + } + return std::nullopt; + } + case Strategy::Kind::Iterator: + case Strategy::Kind::Array: + case Strategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); + } + return std::nullopt; +} + +std::optional PointerInitGadget::getFixits(const Strategy &S) const { + const auto *LeftVD = PtrInitLHS; + const auto *RightVD = cast(PtrInitRHS->getDecl()); + switch (S.lookup(LeftVD)) { + case Strategy::Kind::Span: + if (S.lookup(RightVD) == Strategy::Kind::Span) + return FixItList{}; + return std::nullopt; + case Strategy::Kind::Wontfix: { + if (S.lookup(RightVD) == Strategy::Kind::Span) { + const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in! + RightVD->getASTContext(); + if (auto EndLocOfRHS = + getPastLoc(PtrInitRHS, Ctx.getSourceManager(), Ctx.getLangOpts())) + return FixItList{FixItHint::CreateInsertion(*EndLocOfRHS, ".data()")}; + } + return std::nullopt; + } + case Strategy::Kind::Iterator: + case Strategy::Kind::Array: + case Strategy::Kind::Vector: + llvm_unreachable("unsupported strategies for FixableGadgets"); + } + return std::nullopt; +} + std::optional DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { const VarDecl *VD = dyn_cast(BaseDeclRefExpr->getDecl()); @@ -1605,12 +1621,10 @@ CharSourceRange derefRange = clang::CharSourceRange::getCharRange( Op->getBeginLoc(), Op->getBeginLoc().getLocWithOffset(1)); // Inserts the [0] - std::optional EndOfOperand = - getEndCharLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts()); - if (EndOfOperand) { + if (auto LocPastOperand = + getPastLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts())) { return FixItList{{FixItHint::CreateRemoval(derefRange), - FixItHint::CreateInsertion( - (*EndOfOperand).getLocWithOffset(1), "[0]")}}; + FixItHint::CreateInsertion(*LocPastOperand, "[0]")}}; } break; } @@ -1948,19 +1962,9 @@ // [[clang::unsafe_buffer_usage]] void f(int *p) { // added def // return f(std::span(p, <# size #>)); // } -// -// The actual fix-its may contain more details, e.g., the attribute may be guard -// by a macro -// #if __has_cpp_attribute(clang::unsafe_buffer_usage) -// [[clang::unsafe_buffer_usage]] -// #endif -// -// `ParmsMask` is an array of size of `FD->getNumParams()` such -// that `ParmsMask[i]` is true iff the `i`-th parameter will be fixed with some -// strategy. static std::optional -createOverloadsForFixedParams(const std::vector &ParmsMask, const Strategy &S, - const FunctionDecl *FD, const ASTContext &Ctx, +createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD, + const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { // FIXME: need to make this conflict checking better: if (hasConflictingOverload(FD)) @@ -1970,21 +1974,36 @@ const LangOptions &LangOpts = Ctx.getLangOpts(); const unsigned NumParms = FD->getNumParams(); std::vector NewTysTexts(NumParms); + std::vector ParmsMask(NumParms, false); + bool AtLeastOneParmToFix = false; for (unsigned i = 0; i < NumParms; i++) { - if (!ParmsMask[i]) - continue; + const ParmVarDecl *PVD = FD->getParamDecl(i); + if (S.lookup(PVD) == Strategy::Kind::Wontfix) + continue; + if (S.lookup(PVD) != Strategy::Kind::Span) { + // Not supported, not suppose to happen: + DEBUG_NOTE_UNEXPECTED_ISSUE(PVD, + "unsupported strategy kind for parameter " + + PVD->getNameAsString()); + return std::nullopt; + } std::optional PteTyQuals = std::nullopt; std::optional PteTyText = - getPointeeTypeText(FD->getParamDecl(i), SM, LangOpts, &PteTyQuals); + getPointeeTypeText(PVD, SM, LangOpts, &PteTyQuals); if (!PteTyText) // something wrong in obtaining the text of the pointee type, give up return std::nullopt; // FIXME: whether we should create std::span type depends on the Strategy. NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals); + ParmsMask[i] = true; + AtLeastOneParmToFix = true; } + if (!AtLeastOneParmToFix) + // No need to create function overloads: + return {}; // FIXME Respect indentation of the original code. // A lambda that creates the text representation of a function declaration @@ -2287,28 +2306,17 @@ const VariableGroupsManager &VarGrpMgr, const FunctionDecl *FD, const Strategy &S, ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { FixItList FixItsSharedByParms{}; - std::vector ParmsNeedFixMask(FD->getNumParams(), false); - const VarDecl *FirstParmNeedsFix = nullptr; - - for (unsigned i = 0; i < FD->getNumParams(); i++) - if (FixItsForVariable.count(FD->getParamDecl(i))) { - ParmsNeedFixMask[i] = true; - FirstParmNeedsFix = FD->getParamDecl(i); - } - if (FirstParmNeedsFix) { - // In case at least one parameter needs to be fixed: - std::optional OverloadFixes = - createOverloadsForFixedParams(ParmsNeedFixMask, S, FD, Ctx, Handler); + std::optional OverloadFixes = + createOverloadsForFixedParams(S, FD, Ctx, Handler); - if (OverloadFixes) { - FixItsSharedByParms.append(*OverloadFixes); - } else { - // Something wrong in generating `OverloadFixes`, need to remove the - // whole group, where parameters are in, from `FixItsForVariable` (Note - // that all parameters should be in the same group): - for (auto *Member : VarGrpMgr.getGroupOfVar(FirstParmNeedsFix)) - FixItsForVariable.erase(Member); - } + if (OverloadFixes) { + FixItsSharedByParms.append(*OverloadFixes); + } else { + // Something wrong in generating `OverloadFixes`, need to remove the + // whole group, where parameters are in, from `FixItsForVariable` (Note + // that all parameters should be in the same group): + for (auto *Member : VarGrpMgr.getGroupOfParms()) + FixItsForVariable.erase(Member); } return FixItsSharedByParms; } @@ -2413,8 +2421,9 @@ return FinalFixItsForVariable; } +template static Strategy -getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { +getNaiveStrategy(llvm::iterator_range UnsafeVars) { Strategy S; for (const VarDecl *VD : UnsafeVars) { S.set(VD, Strategy::Kind::Span); @@ -2448,6 +2457,10 @@ return std::nullopt; return Groups[VarGrpMap.at(Var)]; } + + VarGrpRef getGroupOfParms() const override { + return GrpsUnionForParms.getArrayRef(); + } }; void clang::checkUnsafeBufferUsage(const Decl *D, @@ -2700,7 +2713,10 @@ ++I; } - Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); + // We assign strategies to variables on the graph. Other variables have the + // default "Won't fix" strategy. + Strategy NaiveStrategy = getNaiveStrategy( + llvm::make_range(VisitedVars.begin(), VisitedVars.end())); VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms); if (isa(D)) Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp @@ -2,11 +2,12 @@ // RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions \ // RUN: %s 2>&1 | FileCheck %s -// FIXME: what about possible diagnostic message non-determinism? - // CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]: void parmsNoFix(int *p, int *q) { int * a = new int[10]; + // CHECK: fix-it:{{.*}}:{[[@LINE+3]]:3-[[@LINE+3]]:12}:"std::span b" + // CHECK: fix-it:{{.*}}:{[[@LINE+2]]:13-[[@LINE+2]]:13}:"{" + // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:24-[[@LINE+1]]:24}:", 10}" int * b = new int[10]; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \ expected-note{{change type of 'b' to 'std::span' to preserve bounds information}} @@ -27,7 +28,7 @@ int * b; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \ expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a' to 'std::span' to propagate bounds information between them}} - b = a; + b = a; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] b[5] = 5; // expected-note{{used in buffer access here}} p[5] = 5; // expected-note{{used in buffer access here}} } @@ -78,11 +79,11 @@ expected-note{{change type of 'z' to 'std::span' to preserve bounds information, and change 'x', 'p', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}} int * y; - x = p; - y = x; + x = p; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] + y = x; // CHECK: fix-it:{{.*}}:{[[@LINE]]:8-[[@LINE]]:8}:".data()" // `x` needs to be fixed so does the pointer assigned to `x`, i.e.,`p` x[5] = 5; // expected-note{{used in buffer access here}} - z = r; + z = r; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] // `z` needs to be fixed so does the pointer assigned to `z`, i.e.,`r` z[5] = 5; // expected-note{{used in buffer access here}} // Since `p` and `r` are parameters need to be fixed together, @@ -109,10 +110,10 @@ // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" int * y; - p = x; - y = x; + p = x; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] + y = x; // CHECK: fix-it:{{.*}}:{[[@LINE]]:8-[[@LINE]]:8}:".data()" p[5] = 5; // expected-note{{used in buffer access here}} - r = z; + r = z; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] r[5] = 5; // expected-note{{used in buffer access here}} } // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void multiParmLocalAllFix2(int *p, int * r) {return multiParmLocalAllFix2(std::span(p, <# size #>), std::span(r, <# size #>));}\n" @@ -285,7 +286,7 @@ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span y" int * y; // expected-warning{{'y' is an unsafe pointer used for buffer access}} \ expected-note{{change type of 'y' to 'std::span' to preserve bounds information, and change 'x' to 'std::span' to propagate bounds information between them}} - y = x; + y = x; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]] tmp = y[5]; // expected-note{{used in buffer access here}} } // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp @@ -25,7 +25,8 @@ // FIXME: Suggest fixits for p, q, and r since span a valid fixit for r. void rhs_span3() { int *q = new int[6]; - int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' to 'std::span' to propagate bounds information between them}} p[5] = 10; // expected-note{{used in buffer access here}} int *r = q; } Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp @@ -65,7 +65,8 @@ void rhs_span2() { int *q = new int[6]; - int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' to 'std::span' to propagate bounds information between them}} p[5] = 10; // expected-note{{used in buffer access here}} int *r = q; } Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -36,9 +36,11 @@ void * voidPtrCall(void); char * charPtrCall(void); -void testArraySubscripts(int *p, int **pp) { -// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} -// expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}} +void testArraySubscripts(int *p, int **pp) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'pp' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'pp' to safe types to make function 'testArraySubscripts' bounds-safe}} \ + expected-note{{change type of 'pp' to 'std::span' to preserve bounds information, and change 'p' to safe types to make function 'testArraySubscripts' bounds-safe}} + foo(p[1], // expected-note{{used in buffer access here}} pp[1][1], // expected-note{{used in buffer access here}} // expected-warning@-1{{unsafe buffer access}}