Author: Ziqing Luo Date: 2023-08-17T16:42:30-07:00 New Revision: ac9a76d7487b9af1ace626eb90064194cb12c53d
URL: https://github.com/llvm/llvm-project/commit/ac9a76d7487b9af1ace626eb90064194cb12c53d DIFF: https://github.com/llvm/llvm-project/commit/ac9a76d7487b9af1ace626eb90064194cb12c53d.diff LOG: Revert "[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code" This reverts commit 843784764ab58e35f8aa2da97f07dc5e810f4bcb. There is a build failure caused by this commit. Added: Modified: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h clang/lib/Analysis/UnsafeBufferUsage.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 3bb106f6ab83e1..13f28076c6f4d7 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -20,19 +20,7 @@ namespace clang { -using VarGrpTy = std::vector<const VarDecl *>; -using VarGrpRef = ArrayRef<const VarDecl *>; - -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; -}; +using DefMapTy = llvm::DenseMap<const VarDecl *, std::vector<const VarDecl *>>; /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. @@ -65,7 +53,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 VariableGroupsManager &VarGrpMgr, + const DefMapTy &VarGrpMap, FixItList &&Fixes) = 0; #ifndef NDEBUG diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index f2e0190a9221cb..2c36d78e60c89b 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1176,11 +1176,7 @@ groupWarningGadgetsByVar(const WarningGadgetList &AllUnsafeOperations) { } struct FixableGadgetSets { - std::map<const VarDecl *, std::set<const FixableGadget *>, - // To keep keys sorted by their locations in the map so that the - // order is deterministic: - CompareNode<VarDecl>> - byVar; + std::map<const VarDecl *, std::set<const FixableGadget *>> byVar; }; static FixableGadgetSets @@ -1386,7 +1382,7 @@ static std::optional<StringRef> getRangeText(SourceRange SR, const SourceManager &SM, const LangOptions &LangOpts) { bool Invalid = false; - CharSourceRange CSR = CharSourceRange::getCharRange(SR); + CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd()); StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid); if (!Invalid) @@ -2229,7 +2225,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, ASTContext &Ctx, /* The function decl under analysis */ const Decl *D, const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, - const VariableGroupsManager &VarGrpMgr) { + const DefMapTy &VarGrpMap) { std::map<const VarDecl *, FixItList> FixItsForVariable; for (const auto &[VD, Fixables] : FixablesForAllVars.byVar) { FixItsForVariable[VD] = @@ -2265,10 +2261,9 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, continue; } - - { - const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(VD); - for (const VarDecl * V : VarGroupForVD) { + const auto VarGroupForVD = VarGrpMap.find(VD); + if (VarGroupForVD != VarGrpMap.end()) { + for (const VarDecl * V : VarGroupForVD->second) { if (V == VD) { continue; } @@ -2280,7 +2275,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, if (ImpossibleToFix) { FixItsForVariable.erase(VD); - for (const VarDecl * V : VarGroupForVD) { + for (const VarDecl * V : VarGroupForVD->second) { FixItsForVariable.erase(V); } continue; @@ -2298,24 +2293,30 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S, } } - // The map that maps each variable `v` to fix-its for the whole group where - // `v` is in: - std::map<const VarDecl *, FixItList> FinalFixItsForVariable{ - FixItsForVariable}; + 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; + } - for (auto &[Var, Ignore] : FixItsForVariable) { - const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Var); + FixItList GroupFix; + if (FixItsForVariable.find(Var) == FixItsForVariable.end()) { + GroupFix = fixVariable(Var, ReplacementTypeForVD, D, Tracker, + Var->getASTContext(), Handler); + } else { + GroupFix = FixItsForVariable[Var]; + } - 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()); + for (auto Fix : GroupFix) { + FixItsForVariable[VD.first].push_back(Fix); + } + } } } - return FinalFixItsForVariable; + return FixItsForVariable; } @@ -2328,24 +2329,6 @@ getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) { return S; } -// Manages variable groups: -class VariableGroupsManagerImpl : public VariableGroupsManager { - const std::vector<VarGrpTy> Groups; - const std::map<const VarDecl *, unsigned> &VarGrpMap; - -public: - VariableGroupsManagerImpl( - const std::vector<VarGrpTy> &Groups, - const std::map<const VarDecl *, unsigned> &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) { @@ -2425,6 +2408,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets)); std::map<const VarDecl *, FixItList> FixItsForVariableGroup; + DefMapTy VariableGroupsMap{}; // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. for (auto it = FixablesForAllVars.byVar.cbegin(); @@ -2467,7 +2451,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeVars.push_back(VD); // Fixpoint iteration for pointer assignments - using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>; + using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>; DepMapTy DependenciesMap{}; DepMapTy PtrAssignmentGraph{}; @@ -2476,7 +2460,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, std::optional<std::pair<const VarDecl *, const VarDecl *>> ImplPair = fixable->getStrategyImplications(); if (ImplPair) { - std::pair<const VarDecl *, const VarDecl *> Impl = std::move(*ImplPair); + std::pair<const VarDecl *, const VarDecl *> Impl = ImplPair.value(); PtrAssignmentGraph[Impl.first].insert(Impl.second); } } @@ -2521,21 +2505,14 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } - // `Groups` stores the set of Connected Components in the graph. - std::vector<VarGrpTy> 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<const VarDecl *, unsigned> VarGrpMap; - // Group Connected Components for Unsafe Vars // (Dependencies based on pointer assignments) std::set<const VarDecl *> VisitedVars{}; for (const auto &[Var, ignore] : UnsafeOps.byVar) { if (VisitedVars.find(Var) == VisitedVars.end()) { - VarGrpTy &VarGroup = Groups.emplace_back(); - std::queue<const VarDecl*> Queue{}; + std::vector<const VarDecl *> VarGroup{}; + std::queue<const VarDecl*> Queue{}; Queue.push(Var); while(!Queue.empty()) { const VarDecl* CurrentVar = Queue.front(); @@ -2549,10 +2526,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } } } - unsigned GrpIdx = Groups.size() - 1; - - for (const VarDecl *V : VarGroup) { - VarGrpMap[V] = GrpIdx; + for (const VarDecl * V : VarGroup) { + if (UnsafeOps.byVar.find(V) != UnsafeOps.byVar.end()) { + VariableGroupsMap[V] = VarGroup; + } } } } @@ -2586,11 +2563,12 @@ void clang::checkUnsafeBufferUsage(const Decl *D, } Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); - VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap); FixItsForVariableGroup = getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D, - Tracker, Handler, VarGrpMgr); + Tracker, Handler, VariableGroupsMap); + + // FIXME Detect overlapping FixIts. for (const auto &G : UnsafeOps.noVar) { Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false); @@ -2598,7 +2576,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D, for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { auto FixItsIt = FixItsForVariableGroup.find(VD); - Handler.handleUnsafeVariableGroup(VD, VarGrpMgr, + Handler.handleUnsafeVariableGroup(VD, VariableGroupsMap, FixItsIt != FixItsForVariableGroup.end() ? std::move(FixItsIt->second) : FixItList{}); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index addaca4db6d8b2..07762997c9168c 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2175,41 +2175,6 @@ 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<const VarDecl *> &VarGroupForVD) const { - if (VarGroupForVD.size() <= 1) - return ""; - - std::vector<StringRef> 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) {} @@ -2266,25 +2231,62 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { } void handleUnsafeVariableGroup(const VarDecl *Variable, - const VariableGroupsManager &VarGrpMgr, - FixItList &&Fixes) override { + const DefMapTy &VarGrpMap, + 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 = VarGrpMgr.getGroupOfVar(Variable); + 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); FD << Variable << FixItStrategy; - FD << listVariableGroupAsString(Variable, VarGroupForVD) - << (VarGroupForVD.size() > 1); - for (const auto &F : Fixes) { - FD << F; + 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 << F; } #ifndef NDEBUG _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
