diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 13f28076c6f4d..c865b2e8bdb37 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -20,7 +20,19 @@ 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). + virtual VarGrpRef getGroupOfVar(const VarDecl *Var) const =0; +}; /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. @@ -53,7 +65,7 @@ class UnsafeBufferUsageHandler { /// 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. virtual void handleUnsafeVariableGroup(const VarDecl *Variable, - const DefMapTy &VarGrpMap, + const VariableGroupsManager &VarGrpMgr, FixItList &&Fixes) = 0; #ifndef NDEBUG diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 8152eb53da71c..4638f7422320b 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1176,7 +1176,11 @@ groupWarningGadgetsByVar(const WarningGadgetList &AllUnsafeOperations) { } 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 @@ -1382,7 +1386,7 @@ static std::optional getRangeText(SourceRange SR, 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) @@ -2225,7 +2229,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, ASTContext &Ctx, /* The function decl under analysis */ const Decl *D, const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, - const DefMapTy &VarGrpMap) { + const VariableGroupsManager &VarGrpMgr) { std::map FixItsForVariable; for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) { FixItsForVariable[VD] = @@ -2261,9 +2265,10 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, continue; } - const auto VarGroupForVD = VarGrpMap.find(VD); - if (VarGroupForVD != VarGrpMap.end()) { - for (const VarDecl * V : VarGroupForVD->second) { + + { + const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD); + for (const VarDecl * V : VarGroupForVD) { if (V == VD) { continue; } @@ -2275,7 +2280,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, if (ImpossibleToFix) { FixItsForVariable.erase(VD); - for (const VarDecl * V : VarGroupForVD->second) { + for (const VarDecl * V : VarGroupForVD) { FixItsForVariable.erase(V); } continue; @@ -2293,30 +2298,24 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, } } - 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; - } + // The map that maps each variable `v` to fix-its for the whole group where + // `v` is in: + std::map FinalFixItsForVariable{ + FixItsForVariable}; - FixItList GroupFix; - if (FixItsForVariable.find(Var) == FixItsForVariable.end()) { - GroupFix = fixVariable(Var, ReplacementTypeForVD, D, Tracker, - Var->getASTContext(), Handler); - } else { - GroupFix = FixItsForVariable[Var]; - } + for (auto &[Var, Ignore] : FixItsForVariable) { + const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Var); - for (auto Fix : GroupFix) { - FixItsForVariable[VD.first].push_back(Fix); - } - } + for (const VarDecl *GrpMate : VarGroupForVD) { + if (Var == GrpMate) + continue; + if (FixItsForVariable.count(GrpMate)) + FinalFixItsForVariable[Var].insert(FinalFixItsForVariable[Var].end(), + FixItsForVariable[GrpMate].begin(), + FixItsForVariable[GrpMate].end()); } } - return FixItsForVariable; + return FinalFixItsForVariable; } @@ -2329,6 +2328,24 @@ getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { return S; } +// Manages variable groups: +class VariableGroupsManagerImpl : public VariableGroupsManager { + const std::vector Groups; + const std::map &VarGrpMap; + +public: + VariableGroupsManagerImpl( + const std::vector &Groups, + const std::map &VarGrpMap) + : Groups(Groups), VarGrpMap(VarGrpMap) {} + + VarGrpRef getGroupOfVar(const VarDecl *Var) const override { + auto I = VarGrpMap.find(Var); + assert(I != VarGrpMap.end()); + return Groups[I->second]; + } +}; + void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) { @@ -2408,7 +2425,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D, 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(); @@ -2451,7 +2467,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeVars.push_back(VD); // Fixpoint iteration for pointer assignments - using DepMapTy = DenseMap>; + using DepMapTy = DenseMap>; DepMapTy DependenciesMap{}; DepMapTy PtrAssignmentGraph{}; @@ -2460,7 +2476,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, std::optional> ImplPair = fixable->getStrategyImplications(); if (ImplPair) { - std::pair Impl = ImplPair.value(); + std::pair Impl = std::move(*ImplPair); PtrAssignmentGraph[Impl.first].insert(Impl.second); } } @@ -2505,14 +2521,21 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } + // `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; + // 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(); @@ -2526,10 +2549,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } } - for (const VarDecl * V : VarGroup) { - if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) { - VariableGroupsMap[V] = VarGroup; - } + unsigned GrpIdx = Groups.size() - 1; + + for (const VarDecl *V : VarGroup) { + VarGrpMap[V] = GrpIdx; } } } @@ -2563,12 +2586,11 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); + VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap); 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); @@ -2576,7 +2598,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { auto FixItsIt = FixItsForVariableGroup.find(VD); - Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap, + Handler.handleUnsafeVariableGroup(VD, VarGrpMgr, FixItsIt != FixItsForVariableGroup.end() ? std::move(FixItsIt->second) : FixItList{}); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 07762997c9168..addaca4db6d8b 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2175,6 +2175,41 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { 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) {} @@ -2231,62 +2266,25 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { } void handleUnsafeVariableGroup(const VarDecl *Variable, - const DefMapTy &VarGrpMap, - FixItList &&Fixes) override { + const VariableGroupsManager &VarGrpMgr, + FixItList &&Fixes) 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; + const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable); 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); 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; - } else { - FD << "" << 0; - } - - for (const auto &F : Fixes) + FD << listVariableGroupAsString(Variable, VarGroupForVD) + << (VarGroupForVD.size() > 1); + for (const auto &F : Fixes) { FD << F; + } } #ifndef NDEBUG