[clang] ac9a76d - Revert "[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code"
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 17 16:42:44 PDT 2023
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
More information about the cfe-commits
mailing list