Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1286,7 +1286,7 @@ const SourceManager &SM, const LangOptions &LangOpts) { bool Invalid = false; - CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd()); + CharSourceRange CSR = CharSourceRange::getCharRange(SR); StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid); if (!Invalid) @@ -1294,6 +1294,22 @@ return std::nullopt; } +// Returns the `SourceRange` of `D`. The reason why this function exists is +// that `D->getSourceRange()` may return a range where the end location is the +// starting location of the last token. The end location of the source range +// returned by this function is the last location of the last token. +static SourceRange getSourceRangeToTokenEnd(const Decl *D, + const SourceManager &SM, + LangOptions LangOpts) { + SourceLocation Begin = D->getBeginLoc(); + SourceLocation + End = // `D->getEndLoc` should always return the starting location of the + // last token, so we should get the end of the token + Lexer::getLocForEndOfToken(D->getEndLoc(), 0, SM, LangOpts); + + return SourceRange(Begin, End); +} + // Returns the text of the pointee type of `T` from a `VarDecl` of a pointer // type. The text is obtained through from `TypeLoc`s. Since `TypeLoc` does not // have source ranges of qualifiers ( The `QualifiedTypeLoc` looks hacky too me @@ -1358,6 +1374,20 @@ return getRangeText(NameRange, SM, LangOpts); } +// Returns the text representing a `std::span` type where the element type is +// represented by `EltTyText`. Note the optional parameter `Qualifiers`: +// one needs to pass qualifiers explicitly if the element type needs to be +// qualified. +static std::string +getSpanTypeText(StringRef EltTyText, + std::optional Quals = std::nullopt) { + const char *const SpanOpen = "std::span<"; + + if (Quals) + return SpanOpen + EltTyText.str() + ' ' + Quals->getAsString() + '>'; + return SpanOpen + EltTyText.str() + '>'; +} + std::optional DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { const VarDecl *VD = dyn_cast(BaseDeclRefExpr->getDecl()); @@ -1703,11 +1733,11 @@ return SS.str(); } -// For a `FunDecl`, one of whose `ParmVarDecl`s is being changed to have a new -// type, this function produces fix-its to make the change self-contained. Let -// 'F' be the entity defined by the original `FunDecl` and "NewF" be the entity -// defined by the `FunDecl` after the change to the parameter. Fix-its produced -// by this function are +// For a `FunctionDecl`, whose `ParmVarDecl`s are being changed to have new +// types, this function produces fix-its to make the change self-contained. Let +// 'F' be the entity defined by the original `FunctionDecl` and "NewF" be the +// entity defined by the `FunctionDecl` after the change to the parameters. +// Fix-its produced by this function are // 1. Add the `[[clang::unsafe_buffer_usage]]` attribute to each declaration // of 'F'; // 2. Create a declaration of "NewF" next to each declaration of `F`; @@ -1740,25 +1770,45 @@ // [[clang::unsafe_buffer_usage]] // #endif // -// `NewTypeText` is the string representation of the new type, to which the -// parameter indexed by `ParmIdx` is being changed. +// `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(unsigned ParmIdx, StringRef NewTyText, +createOverloadsForFixedParams(ArrayRef ParmsMask, const Strategy &S, const FunctionDecl *FD, const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { + const unsigned NumParms = FD->getNumParams(); + assert(ParmsMask.size() == NumParms && "Incorrect parameter mask size"); // FIXME: need to make this conflict checking better: if (hasConflictingOverload(FD)) return std::nullopt; const SourceManager &SM = Ctx.getSourceManager(); const LangOptions &LangOpts = Ctx.getLangOpts(); + std::vector NewTysTexts(NumParms); + + for (unsigned i = 0; i < NumParms; i++) { + if (!ParmsMask[i]) + continue; + + std::optional PteTyQuals = std::nullopt; + std::optional PteTyText = + getPointeeTypeText(FD->getParamDecl(i), 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); + } // FIXME Respect indentation of the original code. // A lambda that creates the text representation of a function declaration - // with the new type signature: + // with the new type signatures: const auto NewOverloadSignatureCreator = - [&SM, &LangOpts](const FunctionDecl *FD, unsigned ParmIdx, - StringRef NewTypeText) -> std::optional { + [&SM, &LangOpts]( + const FunctionDecl *FD, ArrayRef ParmsMask, + ArrayRef NewTysTexts) -> std::optional { std::stringstream SS; SS << ";"; @@ -1778,13 +1828,16 @@ if (Parm->isImplicit()) continue; - if (i == ParmIdx) { - SS << NewTypeText.str(); + if (ParmsMask[i]) { + // This `i`-th parameter will be fixed with `NewTysTexts[i]` being its + // new type: + SS << NewTysTexts[i]; // print parameter name if provided: - if (IdentifierInfo * II = Parm->getIdentifier()) - SS << " " << II->getName().str(); - } else if (auto ParmTypeText = - getRangeText(Parm->getSourceRange(), SM, LangOpts)) { + if (auto II = Parm->getIdentifier()) + SS << ' ' << II->getName().str(); + } else if (auto ParmTypeText = getRangeText( + getSourceRangeToTokenEnd(Parm, SM, LangOpts), + SM, LangOpts)) { // print the whole `Parm` without modification: SS << ParmTypeText->str(); } else @@ -1800,8 +1853,8 @@ // the original signature: const auto OldOverloadDefCreator = [&Handler, &SM, - &LangOpts](const FunctionDecl *FD, unsigned ParmIdx, - StringRef NewTypeText) -> std::optional { + &LangOpts](const FunctionDecl *FD, ArrayRef ParmsMask, + ArrayRef NewTysTexts) -> std::optional { std::stringstream SS; SS << getEndOfLine().str(); @@ -1827,9 +1880,9 @@ continue; assert(Parm->getIdentifier() && "A parameter of a function definition has no name"); - if (i == ParmIdx) + if (ParmsMask[i]) // This is our spanified paramter! - SS << NewTypeText.str() << "(" << Parm->getIdentifier()->getName().str() << ", " + SS << NewTysTexts[i] << "(" << Parm->getIdentifier()->getName().str() << ", " << Handler.getUserFillPlaceHolder("size") << ")"; else SS << Parm->getIdentifier()->getName().str(); @@ -1853,7 +1906,7 @@ // Inserts a definition with the old signature to the end of // `FReDecl`: if (auto OldOverloadDef = - OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText)) + OldOverloadDefCreator(FReDecl, ParmsMask, NewTysTexts)) FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef)); else return {}; // give up @@ -1865,7 +1918,7 @@ } // Inserts a declaration with the new signature to the end of `FReDecl`: if (auto NewOverloadDecl = - NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText)) + NewOverloadSignatureCreator(FReDecl, ParmsMask, NewTysTexts)) FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl)); else return {}; @@ -1874,67 +1927,42 @@ return FixIts; } -// To fix a `ParmVarDecl` to be of `std::span` type. In addition, creating a -// new overload of the function so that the change is self-contained (see -// `createOverloadsForFixedParams`). -static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { +// To fix a `ParmVarDecl` to be of `std::span` type. +static FixItList fixParamWithSpan(const ParmVarDecl *PVD, + const ASTContext &Ctx) { + // FIXME: can this code be shared with local variable fix? + assert(PVD->getType()->isPointerType() && + "Unexpected non-pointer type parameter for fix"); if (PVD->hasDefaultArg()) // FIXME: generate fix-its for default values: return {}; - assert(PVD->getType()->isPointerType()); - auto *FD = dyn_cast(PVD->getDeclContext()); - - if (!FD) - return {}; std::optional PteTyQualifiers = std::nullopt; std::optional PteTyText = getPointeeTypeText( PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers); if (!PteTyText) - return {}; + return {}; // somehow failed to obtain pointee type text std::optional PVDNameText = PVD->getIdentifier()->getName(); if (!PVDNameText) return {}; - std::string SpanOpen = "std::span<"; - std::string SpanClose = ">"; - std::string SpanTyText; std::stringstream SS; - SS << SpanOpen << *PteTyText; - // Append qualifiers to span element type: if (PteTyQualifiers) - SS << " " << PteTyQualifiers->getAsString(); - SS << SpanClose; - // Save the Span type text: - SpanTyText = SS.str(); + // Append qualifiers if they exist: + SS << getSpanTypeText(*PteTyText, PteTyQualifiers); + else + SS << getSpanTypeText(*PteTyText); // Append qualifiers to the type of the parameter: if (PVD->getType().hasQualifiers()) - SS << " " << PVD->getType().getQualifiers().getAsString(); + SS << ' ' << PVD->getType().getQualifiers().getAsString(); // Append parameter's name: - SS << " " << PVDNameText->str(); - - FixItList Fixes; - unsigned ParmIdx = 0; - - Fixes.push_back( - FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str())); - for (auto *ParmIter : FD->parameters()) { - if (PVD == ParmIter) - break; - ParmIdx++; - } - if (ParmIdx < FD->getNumParams()) - if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText, - FD, Ctx, Handler)) { - Fixes.append(*OverloadFix); - return Fixes; - } - return {}; + SS << ' ' << PVDNameText->str(); + // Add replacement fix-it: + return {FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str())}; } static FixItList fixVariableWithSpan(const VarDecl *VD, @@ -1989,7 +2017,7 @@ case Strategy::Kind::Span: { if (VD->getType()->isPointerType()) { if (const auto *PVD = dyn_cast(VD)) - return fixParamWithSpan(PVD, Ctx, Handler); + return fixParamWithSpan(PVD, Ctx); if (VD->isLocalVarDecl()) return fixVariableWithSpan(VD, Tracker, Ctx, Handler); @@ -2020,25 +2048,32 @@ }); } -static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForUnsafeVars, - const Strategy &S, - const VarDecl * Var) { - for (const auto &F : FixablesForUnsafeVars.byVar.find(Var)->second) { - std::optional Fixits = F->getFixits(S); - if (!Fixits) { - return true; - } - } - return false; +// Returns true if `VD` is a parameter declaration of `D` (a declaration of a +// function, method, block, or lambda): +static bool isParameterOf(const VarDecl *VD, const Decl *D) { + return isa(VD) && + VD->getDeclContext() == dyn_cast(D); } static std::map getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, - ASTContext &Ctx, + ASTContext &Ctx, /* The function decl under analysis */ const Decl *D, - const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, - const DefMapTy &VarGrpMap) { + const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, + const DefMapTy &VarGrpMap) { + // `FixItsForVariable` will map each variable to fix-its directly associated + // to the variable itself. `FixItsForVariable[x]` should be disjoint with + // `FixItsForVariable[y]` if `x` and `y` are two distinct variables: std::map FixItsForVariable; + // Fix-its that are shared by parameters of `D` when parameters are fixed + // together: + FixItList FixItsSharedByParms; + // `FinalFixItsForVariable` will map each variable 'v' to the self-contained + // set of fix-its for 'v', including: + // 1) The union over `FixItsForVariable` on the variable group of 'v'; + // 2) `FixItsSharedByParms`, if parameters are in the same group as 'v'; + std::map FinalFixItsForVariable; + for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler); @@ -2048,84 +2083,122 @@ FixItsForVariable.erase(VD); continue; } - bool ImpossibleToFix = false; - llvm::SmallVector FixItsForVD; for (const auto &F : Fixables) { std::optional Fixits = F->getFixits(S); - if (!Fixits) { - ImpossibleToFix = true; - break; - } else { - const FixItList CorrectFixes = Fixits.value(); - FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(), - CorrectFixes.end()); + if (Fixits) { + FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), + Fixits->begin(), Fixits->end()); + continue; } - } - - if (ImpossibleToFix) { FixItsForVariable.erase(VD); - continue; + break; } - - const auto VarGroupForVD = VarGrpMap.find(VD); - if (VarGroupForVD != VarGrpMap.end()) { - for (const VarDecl * V : VarGroupForVD->second) { - if (V == VD) { - continue; - } - if (impossibleToFixForVar(FixablesForUnsafeVars, S, V)) { - ImpossibleToFix = true; - break; - } - } + } + // `FixItsForVariable` now contains only variables that can be + // fixed. A variable can be fixed if its' declaration and all Fixables + // associated to it can all be fixed. + { + // For each variable 'v' in `FixItsForVariable`, if a group member of 'v' + // CANNOT be fixed, we remove from `FixItsForVariable` the whole group: + SmallVector ToErase; - if (ImpossibleToFix) { - FixItsForVariable.erase(VD); - for (const VarDecl * V : VarGroupForVD->second) { - FixItsForVariable.erase(V); - } + for (auto VD : FixItsForVariable) { + const auto VDGroupIter = VarGrpMap.find(VD.first); + + if (VDGroupIter == VarGrpMap.end()) continue; + if (llvm::any_of(VDGroupIter->getSecond(), + [&FixItsForVariable](const VarDecl *GrpMember) -> bool { + return FixItsForVariable.find(GrpMember) == + FixItsForVariable.end(); + })) { + // At least a group member cannot be fixed, so we have to erase the + // whole group: + ToErase.insert(ToErase.end(), VDGroupIter->getSecond().begin(), + VDGroupIter->getSecond().end()); + } + } + for (auto VarToErase : ToErase) + FixItsForVariable.erase(VarToErase); + } + // Now `FixItsForVariable` gets further reduced: a variable is in + // `FixItsForVariable` iff it can be fixed and all its group members can be + // fixed. + + // Fix-its to parameter declarations of `D` are already in `FixItsForVariable` + // for parameters needing fix. Thus the type signature of `D` will be changed. + // To make the fix-its self-contained, we need to create an overload of `D` + // with its' original signature. The overload fix-it will be added to + // `FixItsSharedByParms`. + if (isa( + D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed. + const FunctionDecl *FD = cast(D); + const unsigned NumParms = FD->getNumParams(); + SmallVector ParmsMask(NumParms); + const ParmVarDecl *FirstParmNeedsFix = nullptr; + + for (unsigned i = 0; i < NumParms; i++) { + if (FixItsForVariable.find(FD->getParamDecl(i)) == + FixItsForVariable.end()) { + ParmsMask[i] = false; + } else { + ParmsMask[i] = true; + if (!FirstParmNeedsFix) + FirstParmNeedsFix = FD->getParamDecl(i); } } - FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), - FixItsForVD.begin(), FixItsForVD.end()); + if (FirstParmNeedsFix) { + // In case at least one parameter needs to be fixed: + std::optional OverloadFixes = + createOverloadsForFixedParams(ParmsMask, S, FD, Ctx, Handler); - // Fix-it shall not overlap with macros or/and templates: - if (overlapWithMacro(FixItsForVariable[VD]) || - clang::internal::anyConflict(FixItsForVariable[VD], - Ctx.getSourceManager())) { - FixItsForVariable.erase(VD); - continue; + 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 VarToErase : VarGrpMap.at(FirstParmNeedsFix)) + FixItsForVariable.erase(VarToErase); + } } } - + // To populate `FinalFixItsForVariable`: for (auto VD : FixItsForVariable) { + FinalFixItsForVariable[VD.first].append(VD.second); + + // Adding group members' fix-its to each variable's complete fix-it list: const auto VarGroupForVD = VarGrpMap.find(VD.first); - const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first); + bool GroupIncludesParms = isParameterOf(VD.first, D); + if (VarGroupForVD != VarGrpMap.end()) { - for (const VarDecl * Var : VarGroupForVD->second) { - if (Var == VD.first) { + for (const VarDecl *Var : VarGroupForVD->second) { + if (Var == VD.first) continue; - } - - FixItList GroupFix; - if (FixItsForVariable.find(Var) == FixItsForVariable.end()) { - GroupFix = fixVariable(Var, ReplacementTypeForVD, D, - Tracker, Var->getASTContext(), Handler); - } else { - GroupFix = FixItsForVariable[Var]; - } - - for (auto Fix : GroupFix) { - FixItsForVariable[VD.first].push_back(Fix); - } + assert(FixItsForVariable.find(Var) != FixItsForVariable.end() && + "It is unexpected that a group member cannot be fixed"); + FinalFixItsForVariable[VD.first].append(FixItsForVariable[Var]); + // Test if `Var` is a parameter of `D`: + if (!GroupIncludesParms && isParameterOf(Var, D)) + GroupIncludesParms = true; } } - } - return FixItsForVariable; + if (GroupIncludesParms) + FinalFixItsForVariable[VD.first].append(FixItsSharedByParms); + } + // Fix-its of one `VarDecl` shall not + // 1. overlap with macros or/and templates; or + // 2. conflicting each other; + for (auto Iter = FinalFixItsForVariable.begin(); + Iter != FinalFixItsForVariable.end();) + if (overlapWithMacro(Iter->second) || + clang::internal::anyConflict(Iter->second, Ctx.getSourceManager())) { + Iter = FinalFixItsForVariable.erase(Iter); + } else + Iter++; + return FinalFixItsForVariable; } - static Strategy getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { Strategy S; @@ -2186,19 +2259,48 @@ // Fixpoint iteration for pointer assignments using DepMapTy = DenseMap>; DepMapTy DependenciesMap{}; + // A pointer variable `y` is in `PtrImplicationGraph[x]` if fixing pointer + // variable `x` implicates fixing `y`: DepMapTy PtrAssignmentGraph{}; - - for (auto it : FixablesForAllVars.byVar) { - for (const FixableGadget *fixable : it.second) { - std::optional> ImplPair = - fixable->getStrategyImplications(); - if (ImplPair) { - std::pair Impl = ImplPair.value(); - PtrAssignmentGraph[Impl.first].insert(Impl.second); + + { + // The set of parameters associated to UnsafeOps or involved in strategy + // implications of Fixables: + SmallSet ParmsNeedFix; + + for (auto it : FixablesForAllVars.byVar) { + for (const FixableGadget *fixable : it.second) { + std::optional> ImplPair = + fixable->getStrategyImplications(); + if (ImplPair) { + std::pair Impl = ImplPair.value(); + PtrAssignmentGraph[Impl.first].insert(Impl.second); + if (isParameterOf(Impl.first, D)) + ParmsNeedFix.insert(Impl.first); + if (isParameterOf(Impl.second, D)) + ParmsNeedFix.insert(Impl.second); + } + } + } + for (auto UnsafeVar : UnsafeOps.byVar) + if (isParameterOf(UnsafeVar.first, D)) + ParmsNeedFix.insert(UnsafeVar.first); + // To force parameters in `ParmsNeedFix` to be in a ring in + // `PtrAssignmentGraph` so that + // 1) they are guaranteed to be in the same group; + // 2) no such parameter will be missed regardless of the starting node in + // search of connected components. + if (!ParmsNeedFix.empty()) { + auto First = ParmsNeedFix.begin(), Last = First; + + for (auto I = ++First; I != ParmsNeedFix.end(); ++I) { + PtrAssignmentGraph[*Last].insert(*I); + Last = I; } + if (First != Last) + PtrAssignmentGraph[*First].insert(*Last); } } - /* The following code does a BFS traversal of the `PtrAssignmentGraph` considering all unsafe vars as starting nodes and constructs an undirected Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp @@ -0,0 +1,251 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions -verify %s +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions \ +// RUN: %s 2>&1 | FileCheck %s + +// FIXME: what about possible diagnostic message non-determinism? + +// Parameters other than `r` all will be fixed +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span p" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span q" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:41-[[@LINE+1]]:48}:"std::span a" +void * multiParmAllFix(int *p, int **q, int a[], int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'a' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' and 'a' to 'std::span' to propagate bounds information between them}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to 'std::span' to propagate bounds information between them}} \ + expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to 'std::span' to propagate bounds information between them}} + int tmp; + + tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = a[5]; // expected-note{{used in buffer access here}} + if (++q) {} // expected-note{{used in pointer arithmetic here}} + return r; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid * multiParmAllFix(int *p, int **q, int a[], int * r) {return multiParmAllFix(std::span(p, <# size #>), std::span(q, <# size #>), std::span(a, <# size #>), r);}\n" + +void * multiParmAllFix(int *p, int **q, int a[], int * r); +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:58-[[@LINE-2]]:58}:";\nvoid * multiParmAllFix(std::span p, std::span q, std::span a, int * r)" + +// Fixing local variables implicates fixing parameters +void multiParmLocalAllFix(int *p, int * r) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span p" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span r" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span x" + int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'r', and 'z' to 'std::span' to propagate bounds information between them}} + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span z" + int * z; // expected-warning{{'z' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'z' to 'std::span' to preserve bounds information, and change 'x', 'p', 'r', to 'std::span' to propagate bounds information between them}} + int * y; + + x = p; + y = x; + // `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` 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, + // fixing `x` involves fixing all `p`, `r`, and `z`. Similar for + // fixing `z`. +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid multiParmLocalAllFix(int *p, int * r) {return multiParmLocalAllFix(std::span(p, <# size #>), std::span(r, <# size #>));}\n" + + +// Fixing parameters implicates fixing local variables +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span p" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE+1]]:37-[[@LINE+1]]:44}:"std::span r" +void multiParmLocalAllFix2(int *p, int * r) { // 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 'r', 'x', and 'z' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'x', and 'z' to 'std::span' to propagate bounds information between them}} + int * x = new int[10]; + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span x" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" + int * z = new int[10]; + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span z" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" + int * y; + + p = x; + y = x; + p[5] = 5; // expected-note{{used in buffer access here}} + r = z; + r[5] = 5; // expected-note{{used in buffer access here}} +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid multiParmLocalAllFix2(int *p, int * r) {return multiParmLocalAllFix2(std::span(p, <# size #>), std::span(r, <# size #>));}\n" + + +// No fix emitted for any of the parameter since parameter `r` cannot be fixed +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]] +void noneParmFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + r++; // expected-note{{used in pointer arithmetic here}} + tmp = r[5]; // expected-note{{used in buffer access here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + +// To show what if the `r` in `noneParmFix` can be fixed: +void noneParmFix_control(int * p, int * q, int * r) { // 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' and 'r' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'r' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p' and 'q' to 'std::span' to propagate bounds information between them}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + if (++r) {} // expected-note{{used in pointer arithmetic here}} + tmp = r[5]; // expected-note{{used in buffer access here}} +} + + +// No fix emitted for any of the parameter since local variable `l` cannot be fixed. +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]] +void noneParmOrLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + // `l` and `r` must be fixed together while all parameters must be fixed together as well: + int * l; l = r; // expected-warning{{'l' is an unsafe pointer used for buffer access}} + + l++; // expected-note{{used in pointer arithmetic here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + +// To show what if the `l` can be fixed in `noneParmOrLocalFix`: +void noneParmOrLocalFix_control(int * p, int * q, int * r) {// \ + 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', 'r', and 'l' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', and 'l' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', and 'l' to 'std::span' to propagate bounds information between them}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', to 'std::span' to propagate bounds information between them}} + l = r; + if (++l){}; // expected-note{{used in pointer arithmetic here}} +} + + +// No fix emitted for any of the parameter since local variable `l` cannot be fixed. +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]] +void noneParmOrLocalFix2(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + + int * a; a = r; + int * b; b = a; + int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}} + + // `a, b, l` and parameters must be fixed together but `l` can't be fixed: + l++; // expected-note{{used in pointer arithmetic here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + +// To show what if the `l` can be fixed in `noneParmOrLocalFix2`: +void noneParmOrLocalFix2_control(int * p, int * q, int * r) { // \ + 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', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + + int * a; a = r; + int * b; b = a; + int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'a', and 'b' to 'std::span' to propagate bounds information between them}} + + l = b; + if(++l){} // expected-note{{used in pointer arithmetic here}} +} + +// No fix emitted for any of the parameter since local variable `l` cannot be fixed +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]] +void noneParmOrLocalFix3(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + + int * a; a = r; + int * b; b = a; + int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}} + + l++; // expected-note{{used in pointer arithmetic here}} + + int * x; x = p; // expected-warning{{'x' is an unsafe pointer used for buffer access}} + tmp = x[5]; // expected-note{{used in buffer access here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + +void noneParmOrLocalFix3_control(int * p, int * q, int * r) { // \ + 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', 'r', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p', 'r', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'q', 'x', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + + int * a; a = r; + int * b; b = a; + int * l; // expected-warning{{'l' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'l' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'x', 'a', and 'b' to 'std::span' to propagate bounds information between them}} + + l = b; + if (++l){}; // expected-note{{used in pointer arithmetic here}} + + int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'q', 'r', 'l', 'a', and 'b' to 'std::span' to propagate bounds information between them}} + x = p; + tmp = x[5]; // expected-note{{used in buffer access here}} +} + + +// No fix emitted for any of the parameter but some local variables will get fixed +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]] +void noneParmSomeLocalFix(int * p, int * q, int * r) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-warning{{'q' is an unsafe pointer used for buffer access}} \ + expected-warning{{'r' is an unsafe pointer used for buffer access}} + int tmp = p[5]; // expected-note{{used in buffer access here}} + tmp = q[5]; // expected-note{{used in buffer access here}} + tmp = r[5]; // expected-note{{used in buffer access here}} + + int * a; a = r; + int * b; b = a; + int * l; l = b; // expected-warning{{'l' is an unsafe pointer used for buffer access}} + + l++; // expected-note{{used in pointer arithmetic here}} + + //`x` and `y` can be fixed + int * x = new int[10]; + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span x" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" + // CHECK-DAG: 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; + tmp = y[5]; // expected-note{{used in buffer access here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp =================================================================== --- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp @@ -29,8 +29,7 @@ int tmp; tmp = p[5] + q[5]; } -// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span(p, <# size #>), q);}\n" -// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:2-[[@LINE-2]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(p, std::span(q, <# size #>));}\n" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span(p, <# size #>), std::span(q, <# size #>));}\n" void ptrToConst(const int * x) { // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:17-[[@LINE-1]]:30}:"std::span x" @@ -100,22 +99,30 @@ // namned } NAMED_S; + // FIXME: `decltype(ANON_S)` represents an unnamed type but it can // be referred as "`decltype(ANON_S)`", so the analysis should // fix-it. - void decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r, - decltype(NAMED_S) ** rr) { - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:26-[[@LINE-2]]:41}:"std::span p" - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:65-[[@LINE-3]]:86}:"std::span r" - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:26-[[@LINE-3]]:49}:"std::span rr" + // As parameter `q` cannot be fixed, fixes to parameters are all being given up. + void decltypeSpecifierAnon(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r, + decltype(NAMED_S) ** rr) { + // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] if (++p) {} if (++q) {} if (++r) {} if (++rr) {} } - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span(p, <# size #>), q, r, rr);}\n - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, std::span(r, <# size #>), rr);}\n" - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:4-[[@LINE-3]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, r, std::span(rr, <# size #>));}\n" + // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + + void decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:26-[[@LINE-1]]:41}:"std::span p" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:43-[[@LINE-2]]:64}:"std::span r" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:66-[[@LINE-3]]:89}:"std::span rr" + if (++p) {} + if (++r) {} + if (++rr) {} + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(NAMED_S) * r, decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span(p, <# size #>), std::span(r, <# size #>), std::span(rr, <# size #>));}\n #define MACRO_TYPE(T) long T @@ -125,8 +132,7 @@ int tmp = p[5]; tmp = q[5]; } - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span(p, <# size #>), q);}\n" - // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(p, std::span(q, <# size #>));}\n" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span(p, <# size #>), std::span(q, <# size #>));}\n" } // The followings test various declarators: 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,7 +36,7 @@ void * voidPtrCall(void); char * charPtrCall(void); -void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of 'pp' to 'std::span' to preserve bounds information}} +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}} foo(p[1], // expected-note{{used in buffer access here}} @@ -109,7 +109,6 @@ sizeof(decltype(p[1]))); // no-warning } -// expected-note@+1{{change type of 'a' to 'std::span' to preserve bounds information}} void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) { // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} // expected-warning@-2{{'q' is an unsafe pointer used for buffer access}}