Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -19,7 +19,22 @@ namespace clang { -using DefMapTy = llvm::DenseMap>; +using VarGrpTy = std::vector; +using VarGrpRef = ArrayRef; + +class VariableGroupsManager { +public: + VariableGroupsManager() = default; + virtual ~VariableGroupsManager() = default; + /// Returns the set of variables (including `Var`) that need to be fixed + /// together in one step. + /// + /// `Var` must be a variable that needs fix (so it must be in a group). + /// `HasParm` is an optional argument that will be set to true if the set of + /// variables, where `Var` is in, contains parameters. + virtual VarGrpRef getGroupOfVar(const VarDecl *Var, + bool *HasParm = nullptr) const; +}; /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. @@ -37,11 +52,16 @@ bool IsRelatedToDecl) = 0; /// Invoked when a fix is suggested against a variable. This function groups - /// all variables that must be fixed together (i.e their types must be changed to the - /// same target type to prevent type mismatches) into a single fixit. + /// all variables that must be fixed together (i.e their types must be changed + /// to the same target type to prevent type mismatches) into a single fixit. + /// + /// `D` is the declaration of the callable under analysis that owns `Variable` + /// and all the variables in `Group`. + /// `BriefMsg` is true if to NOT explain how variables are grouped together. virtual void handleUnsafeVariableGroup(const VarDecl *Variable, - const DefMapTy &VarGrpMap, - FixItList &&Fixes) = 0; + ArrayRef Group, + FixItList &&Fixes, const Decl *D, + bool BriefMsg = false) = 0; /// Returns a reference to the `Preprocessor`: virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0; Index: clang/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11841,6 +11841,9 @@ "used%select{| in pointer arithmetic| in buffer access}0 here">; def note_unsafe_buffer_variable_fixit_group : Note< "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information%select{|, and change %2 to '%select{std::span|std::array|std::span::iterator}1' to propagate bounds information between them}3">; +def note_unsafe_buffer_variable_fixit_together : Note< + "change type of %0 to '%select{std::span|std::array|std::span::iterator}1' to preserve bounds information" + "%select{|, and change %2 to safe types to make function '%4' bounds-safe}3">; def note_safe_buffer_usage_suggestions_disabled : Note< "pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions">; def err_loongarch_builtin_requires_la32 : Error< Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -12,7 +12,7 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" -#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/SetVector.h" #include #include #include @@ -1148,7 +1148,11 @@ } struct FixableGadgetSets { - std::map> byVar; + std::map, + // To keep keys sorted by their locations in the map so that the + // order is deterministic: + CompareNode> + byVar; }; static FixableGadgetSets @@ -1346,7 +1350,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) @@ -1354,6 +1358,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 @@ -1418,6 +1438,21 @@ 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()); @@ -1763,11 +1798,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`; @@ -1800,25 +1835,43 @@ // [[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, - const FunctionDecl *FD, const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { +createOverloadsForFixedParams(const std::vector &ParmsMask, const Strategy &S, + const FunctionDecl *FD, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { // FIXME: need to make this conflict checking better: if (hasConflictingOverload(FD)) return std::nullopt; const SourceManager &SM = Ctx.getSourceManager(); const LangOptions &LangOpts = Ctx.getLangOpts(); + const unsigned NumParms = FD->getNumParams(); + 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, &NewTysTexts, + &ParmsMask](const FunctionDecl *FD) -> std::optional { std::stringstream SS; SS << ";"; @@ -1838,13 +1891,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 @@ -1859,9 +1915,8 @@ // A lambda that creates the text representation of a function definition with // the original signature: const auto OldOverloadDefCreator = - [&Handler, &SM, - &LangOpts](const FunctionDecl *FD, unsigned ParmIdx, - StringRef NewTypeText) -> std::optional { + [&Handler, &SM, &LangOpts, &NewTysTexts, + &ParmsMask](const FunctionDecl *FD) -> std::optional { std::stringstream SS; SS << getEndOfLine().str(); @@ -1887,10 +1942,10 @@ 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() << ", " - << Handler.getUserFillPlaceHolder("size") << ")"; + SS << NewTysTexts[i] << "(" << Parm->getIdentifier()->getName().str() + << ", " << Handler.getUserFillPlaceHolder("size") << ")"; else SS << Parm->getIdentifier()->getName().str(); if (i != NumParms - 1) @@ -1912,8 +1967,7 @@ assert(FReDecl == FD && "inconsistent function definition"); // Inserts a definition with the old signature to the end of // `FReDecl`: - if (auto OldOverloadDef = - OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText)) + if (auto OldOverloadDef = OldOverloadDefCreator(FReDecl)) FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef)); else return {}; // give up @@ -1924,8 +1978,7 @@ FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeText())); } // Inserts a declaration with the new signature to the end of `FReDecl`: - if (auto NewOverloadDecl = - NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText)) + if (auto NewOverloadDecl = NewOverloadSignatureCreator(FReDecl)) FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl)); else return {}; @@ -1934,67 +1987,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, @@ -2049,7 +2077,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); @@ -2080,25 +2108,96 @@ }); } -static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars, - const Strategy &S, - const VarDecl * Var) { - for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) { - std::optional Fixits = F->getFixits(S); - if (!Fixits) { - return true; +// Returns true iff `VD` is a parameter of the declaration `D`: +static bool isParameterOf(const VarDecl *VD, const Decl *D) { + return isa(VD) && + VD->getDeclContext() == dyn_cast(D); +} + +// Erasing variables in `FixItsForVariable`, if such a variable has an unfixable +// group mate. A variable `v` is unfixable iff `FixItsForVariable` does not +// contain `v`. +static void eraseVarsForUnfixableGroupMates( + std::map &FixItsForVariable, + const VariableGroupsManager &VarGrpMgr) { + // Variables will be removed from `FixItsForVariable`: + SmallVector ToErase; + + for (auto [VD, Ignore] : FixItsForVariable) { + VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD); + if (llvm::any_of(Grp, + [&FixItsForVariable](const VarDecl *GrpMember) -> bool { + return FixItsForVariable.find(GrpMember) == + FixItsForVariable.end(); + })) { + // At least one group member cannot be fixed, so we have to erase the + // whole group: + for (const VarDecl *Member : Grp) + ToErase.push_back(Member); } } - return false; + for (auto *VarToErase : ToErase) + FixItsForVariable.erase(VarToErase); +} + +// Returns the fix-its that create bounds-safe function overloads for the +// function `D`, if `D`'s parameters will be changed to safe-types through +// fix-its in `FixItsForVariable`. +// +// NOTE: In case `D`'s parameters will be changed but bounds-safe function +// overloads cannot created, the whole group that contains the parameters will +// be erased from `FixItsForVariable`. +static FixItList createFunctionOverloadsForParms( + std::map &FixItsForVariable /* mutable */, + const VariableGroupsManager &VarGrpMgr, const Decl *D, const Strategy &S, + ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { + FixItList FixItsSharedByParms{}; + + if (auto *FD = dyn_cast( + D)) { // If `D` is not a `FunctionDecl`, no parameter will be fixed. + std::vector ParmsNeedFixMask(FD->getNumParams(), false); + const VarDecl *FirstParmNeedsFix = nullptr; + + for (unsigned i = 0; i < FD->getNumParams(); i++) + if (FixItsForVariable.find(FD->getParamDecl(i)) != + FixItsForVariable.end()) { + 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); + + 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); + } + } + } + return FixItsSharedByParms; } +// Constructs self-contained fix-its for each variable in `FixablesForAllVars`. static std::map getFixIts(FixableGadgetSets &FixablesForAllVars, 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 VariableGroupsManager &VarGrpMgr) { + // `FixItsForVariable` will map each variable to a set of fix-its directly + // associated to the variable itself. Fix-its of distict variables in + // `FixItsForVariable` are disjoint. std::map FixItsForVariable; + + // Populate `FixItsForVariable` with fix-its directly associated with each + // variable. Fix-its directly associated to a variable 'v' are the ones + // produced by the `FixableGadget`s whose claimed variable is 'v'. for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) { FixItsForVariable[VD] = fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler); @@ -2108,84 +2207,68 @@ 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 (ImpossibleToFix) { - FixItsForVariable.erase(VD); - continue; - } - - const auto VarGroupForVD = VarGrpMap.find(VD); - if (VarGroupForVD != VarGrpMap.end()) { - for (const VarDecl * V : VarGroupForVD->second) { - if (V == VD) { - continue; - } - if (impossibleToFixForVar(FixablesForAllVars, S, V)) { - ImpossibleToFix = true; - break; - } - } - if (ImpossibleToFix) { - FixItsForVariable.erase(VD); - for (const VarDecl * V : VarGroupForVD->second) { - FixItsForVariable.erase(V); - } + if (Fixits) { + FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), + Fixits->begin(), Fixits->end()); continue; } - } - FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), - FixItsForVD.begin(), FixItsForVD.end()); - - // 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; + break; } } - for (auto VD : FixItsForVariable) { - const auto VarGroupForVD = VarGrpMap.find(VD.first); - const Strategy::Kind ReplacementTypeForVD = S.lookup(VD.first); - if (VarGroupForVD != VarGrpMap.end()) { - 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); - } - } + // `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. + + // To further remove from `FixItsForVariable` variables whose group mates + // cannot be fixed... + eraseVarsForUnfixableGroupMates(FixItsForVariable, VarGrpMgr); + // Now `FixItsForVariable` gets further reduced: a variable is in + // `FixItsForVariable` iff it can be fixed and all its group mates can be + // fixed. + + // Fix-its of bounds-safe overloads of `D` are shared by parameters of `D`. + // That is, when fixing multiple parameters in one step, these fix-its will + // be applied only once (instead of being applied per parameter). + FixItList FixItsSharedByParms = createFunctionOverloadsForParms( + FixItsForVariable, VarGrpMgr, D, S, Ctx, Handler); + + // `FinalFixItsForVariable` will map each variable 'v' to the self-contained + // set of fix-its for 'v', including: + // 1) fix-its directly associated to every group mate of 'v' (including 'v' + // itself); and + // 2) `FixItsSharedByParms`, if parameters are in the same group as 'v'. + std::map FinalFixItsForVariable; + + for (auto [VD, Ignore] : FixItsForVariable) { + FixItList &FinalFixItsForVD = FinalFixItsForVariable[VD]; + bool AnyParm = false; + VarGrpRef Grp = VarGrpMgr.getGroupOfVar(VD, &AnyParm); + + for (auto *Member : Grp) { + FinalFixItsForVD.append(FixItsForVariable[Member]); } - } - return FixItsForVariable; + if (AnyParm) + FinalFixItsForVD.append(FixItsSharedByParms); + } + // Fix-its that will be applied in one step shall NOT: + // 1. overlap with macros or/and templates; or + // 2. conflicting each other. + // Otherwise, the fix-its will be dropped. + 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; @@ -2195,6 +2278,34 @@ return S; } +// Manages variable groups: +class VariableGroupsManagerImpl : public VariableGroupsManager { + const std::vector Groups; + const std::map &VarGrpMap; + const llvm::SetVector &GrpsUnionForParms; + +public: + VariableGroupsManagerImpl( + const std::vector &Groups, + const std::map &VarGrpMap, + const llvm::SetVector &GrpsUnionForParms) + : Groups(Groups), VarGrpMap(VarGrpMap), + GrpsUnionForParms(GrpsUnionForParms) {} + + VarGrpRef getGroupOfVar(const VarDecl *Var, bool *HasParm) const override { + if (GrpsUnionForParms.contains(Var)) { + if (HasParm) + *HasParm = true; + return GrpsUnionForParms.getArrayRef(); + } + if (HasParm) + *HasParm = false; + if (VarGrpMap.find(Var) == VarGrpMap.end()) + return std::nullopt; + return Groups[VarGrpMap.at(Var)]; + } +}; + void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) { @@ -2237,7 +2348,6 @@ FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets)); std::map FixItsForVariableGroup; - DefMapTy VariableGroupsMap{}; // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. for (auto it = FixablesForAllVars.byVar.cbegin(); @@ -2256,7 +2366,7 @@ UnsafeVars.push_back(VD); // Fixpoint iteration for pointer assignments - using DepMapTy = DenseMap>; + using DepMapTy = DenseMap>; DepMapTy DependenciesMap{}; DepMapTy PtrAssignmentGraph{}; @@ -2265,7 +2375,7 @@ std::optional> ImplPair = fixable->getStrategyImplications(); if (ImplPair) { - std::pair Impl = ImplPair.value(); + std::pair Impl = std::move(*ImplPair); PtrAssignmentGraph[Impl.first].insert(Impl.second); } } @@ -2310,14 +2420,24 @@ } } + // `Groups` stores the set of Connected Components in the graph. + std::vector Groups; + // `VarGrpMap` maps variables that need fix to the groups (indexes) that the + // variables belong to. Group indexes refer to the elements in `Groups`. + // `VarGrpMap` is complete in that every variable that needs fix is in it. + std::map VarGrpMap; + // The union group over the ones in "Groups" that contain parameters of `D`: + llvm::SetVector + GrpsUnionForParms; // these variables need to be fixed in one step + // Group Connected Components for Unsafe Vars // (Dependencies based on pointer assignments) std::set VisitedVars{}; for (const auto &[Var, ignore] : UnsafeOps.byVar) { if (VisitedVars.find(Var) == VisitedVars.end()) { - std::vector VarGroup{}; - + VarGrpTy &VarGroup = Groups.emplace_back(); std::queue Queue{}; + Queue.push(Var); while(!Queue.empty()) { const VarDecl* CurrentVar = Queue.front(); @@ -2331,21 +2451,26 @@ } } } - for (const VarDecl * V : VarGroup) { - if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) { - VariableGroupsMap[V] = VarGroup; - } + + bool HasParm = false; + unsigned GrpIdx = Groups.size() - 1; + + for (const VarDecl *V : VarGroup) { + VarGrpMap[V] = GrpIdx; + if (!HasParm && isParameterOf(V, D)) + HasParm = true; } + if (HasParm) + GrpsUnionForParms.insert(VarGroup.begin(), VarGroup.end()); } } Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); + VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms); FixItsForVariableGroup = getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D, - Tracker, Handler, VariableGroupsMap); - - // FIXME Detect overlapping FixIts. + Tracker, Handler, VarGrpMgr); for (const auto &G : UnsafeOps.noVar) { Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false); @@ -2353,10 +2478,16 @@ for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { auto FixItsIt = FixItsForVariableGroup.find(VD); - Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap, + bool BriefMsg = false; // Set to `true` if to NOT explain how variables are + // grouped together + // If `Group` contains parameters, make the diag message brief: + VarGrpRef Group = VarGrpMgr.getGroupOfVar(VD, &BriefMsg); + + Handler.handleUnsafeVariableGroup(VD, Group, FixItsIt != FixItsForVariableGroup.end() - ? std::move(FixItsIt->second) - : FixItList{}); + ? std::move(FixItsIt->second) + : FixItList{}, + D, BriefMsg); for (const auto &G : WarningGadgets) { Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true); } Index: clang/lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- clang/lib/Sema/AnalysisBasedWarnings.cpp +++ clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2163,6 +2163,41 @@ Sema &S; bool SuggestSuggestions; // Recommend -fsafe-buffer-usage-suggestions? + // Lists as a string the names of variables in `VarGroupForVD` except for `VD` + // itself: + std::string listVariableGroupAsString( + const VarDecl *VD, const ArrayRef &VarGroupForVD) const { + if (VarGroupForVD.size() <= 1) + return ""; + + std::vector VarNames; + auto PutInQuotes = [](StringRef S) -> std::string { + return "'" + S.str() + "'"; + }; + + for (auto *V : VarGroupForVD) { + if (V == VD) + continue; + VarNames.push_back(V->getName()); + } + if (VarNames.size() == 1) { + return PutInQuotes(VarNames[0]); + } + if (VarNames.size() == 2) { + return PutInQuotes(VarNames[0]) + " and " + PutInQuotes(VarNames[1]); + } + assert(VarGroupForVD.size() > 3); + const unsigned N = VarNames.size() - + 2; // need to print the last two names as "..., X, and Y" + std::string AllVars = ""; + + for (unsigned I = 0; I < N; ++I) + AllVars.append(PutInQuotes(VarNames[I]) + ", "); + AllVars.append(PutInQuotes(VarNames[N]) + ", and " + + PutInQuotes(VarNames[N + 1])); + return AllVars; + } + public: UnsafeBufferUsageReporter(Sema &S, bool SuggestSuggestions) : S(S), SuggestSuggestions(SuggestSuggestions) {} @@ -2218,61 +2253,33 @@ } } - void handleUnsafeVariableGroup(const VarDecl *Variable, - const DefMapTy &VarGrpMap, - FixItList &&Fixes) override { + void handleUnsafeVariableGroup(const VarDecl *Variable, const VarGrpRef Group, + FixItList &&Fixes, const Decl *D, + bool BriefMsg) override { assert(!SuggestSuggestions && "Unsafe buffer usage fixits displayed without suggestions!"); S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable) << Variable << (Variable->getType()->isPointerType() ? 0 : 1) << Variable->getSourceRange(); if (!Fixes.empty()) { - const auto VarGroupForVD = VarGrpMap.find(Variable)->second; unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy - const auto &FD = S.Diag(Variable->getLocation(), - diag::note_unsafe_buffer_variable_fixit_group); + const auto &FD = + S.Diag(Variable->getLocation(), + BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together + : diag::note_unsafe_buffer_variable_fixit_group); FD << Variable << FixItStrategy; - std::string AllVars = ""; - if (VarGroupForVD.size() > 1) { - if (VarGroupForVD.size() == 2) { - if (VarGroupForVD[0] == Variable) { - AllVars.append("'" + VarGroupForVD[1]->getName().str() + "'"); - } else { - AllVars.append("'" + VarGroupForVD[0]->getName().str() + "'"); - } - } else { - bool first = false; - if (VarGroupForVD.size() == 3) { - for (const VarDecl * V : VarGroupForVD) { - if (V == Variable) { - continue; - } - if (!first) { - first = true; - AllVars.append("'" + V->getName().str() + "'" + " and "); - } else { - AllVars.append("'" + V->getName().str() + "'"); - } - } - } else { - for (const VarDecl * V : VarGroupForVD) { - if (V == Variable) { - continue; - } - if (VarGroupForVD.back() != V) { - AllVars.append("'" + V->getName().str() + "'" + ", "); - } else { - AllVars.append("and '" + V->getName().str() + "'"); - } - } - } - } - FD << AllVars << 1; + if (Group.size() > 1) { + FD << listVariableGroupAsString(Variable, Group) + << /* print the group */ 1; } else { - FD << "" << 0; + FD << "" << /* no group mate to print */ 0; + } + if (BriefMsg) { + assert(isa(D) && + "Fix for non-function declarations is not supported."); + FD << cast(D)->getNameAsString(); } - for (const auto &F : Fixes) FD << F; } 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,306 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions -verify %s +// 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]; + 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}} + + a = p; + a = q; + b[5] = 5; // expected-note{{used in buffer access here}} +} + +// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:21-[[@LINE+2]]:27}:"std::span p" +// CHECK: fix-it:{{.*}}:{[[@LINE+14]]:2-[[@LINE+14]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsSingleton(int *p) {return parmsSingleton(std::span(p, <# size #>));}\n" +void parmsSingleton(int *p) { //expected-warning{{'p' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'p' to 'std::span' to preserve bounds information}} + // CHECK: fix-it:{{.*}}:{[[@LINE+3]]:3-[[@LINE+3]]:12}:"std::span a" + // CHECK: fix-it:{{.*}}:{[[@LINE+2]]:13-[[@LINE+2]]:13}:"{" + // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:24-[[@LINE+1]]:24}:", 10}" + int * a = new int[10]; + // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span b" + 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[5] = 5; // expected-note{{used in buffer access here}} + p[5] = 5; // expected-note{{used in buffer access here}} +} + + +// Parameters other than `r` all will be fixed +// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:24-[[@LINE+3]]:30}:"std::span p" +// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:32-[[@LINE+2]]:39}:"std::span q" +// CHECK: 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 safe types to make function 'multiParmAllFix' bounds-safe}} \ + expected-note{{change type of 'q' to 'std::span' to preserve bounds information, and change 'p' and 'a' to safe types to make function 'multiParmAllFix' bounds-safe}} \ + expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p' and 'q' to safe types to make function 'multiParmAllFix' bounds-safe}} + 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: 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: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" +// CHECK: 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: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span p" + // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span r" + // CHECK: 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', 'z', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}} + // CHECK: 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', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}} + 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: 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: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span p" +// CHECK: 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 'x', 'r', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}} \ + 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 safe types to make function 'multiParmLocalAllFix2' bounds-safe}} + int * x = new int[10]; + // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span x" + // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" + int * z = new int[10]; + // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span z" + // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: 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: 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 safe types to make function 'noneParmFix_control' bounds-safe}} \ + 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 safe types to make function 'noneParmFix_control' bounds-safe}} \ + 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 safe types to make function 'noneParmFix_control' bounds-safe}} + 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \ + 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} \ + 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 safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} + 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', and 'r' to safe types to make function 'noneParmOrLocalFix_control' bounds-safe}} + 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} \ + 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} \ + 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} + 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix2_control' bounds-safe}} + + 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 'x', 'q', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} \ + 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', 'x', 'r', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} \ + 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', 'q', 'l', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} + 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', 'x', 'q', 'r', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} + + 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', 'b', and 'a' to safe types to make function 'noneParmOrLocalFix3_control' bounds-safe}} + 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: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span x" + // CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}" + // 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; + tmp = y[5]; // expected-note{{used in buffer access here}} +} +// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + +// Test that other parameters of (lambdas and blocks) do not interfere +// the grouping of variables of the function. +// CHECK: fix-it:{{.*}}:{[[@LINE+3]]:30-[[@LINE+3]]:37}:"std::span p" +// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:39-[[@LINE+2]]:46}:"std::span q" +// CHECK: fix-it:{{.*}}:{[[@LINE+20]]:2-[[@LINE+20]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid parmsFromLambdaAndBlock(int * p, int * q) {return parmsFromLambdaAndBlock(std::span(p, <# size #>), std::span(q, <# size #>));}\n" +void parmsFromLambdaAndBlock(int * p, int * q) { + // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span a" + int * a; // expected-warning{{'a' is an unsafe pointer used for buffer access}} \ + expected-note{{change type of 'a' to 'std::span' to preserve bounds information, and change 'p', 'b', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}} + // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span b" + 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', 'p', and 'q' to safe types to make function 'parmsFromLambdaAndBlock' bounds-safe}} + auto Lamb = [](int * x) -> void { // expected-warning{{'x' is an unsafe pointer used for buffer access}} + x[5] = 5; // expected-note{{used in buffer access here}} + }; + + void (^Blk)(int*) = ^(int * y) { // expected-warning{{'y' is an unsafe pointer used for buffer access}} + y[5] = 5; // expected-note{{used in buffer access here}} + }; + + a = p; + b = q; + a[5] = 5; // expected-note{{used in buffer access here}} + b[5] = 5; // expected-note{{used in buffer access here}} +} 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}}