[clang] 472a510 - Re-land "[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code"
via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 18 13:22:36 PDT 2023
Author: ziqingluo-90
Date: 2023-08-18T13:22:01-07:00
New Revision: 472a510bbc0325eb8e5920845e7c8d1a6a28a387
URL: https://github.com/llvm/llvm-project/commit/472a510bbc0325eb8e5920845e7c8d1a6a28a387
DIFF: https://github.com/llvm/llvm-project/commit/472a510bbc0325eb8e5920845e7c8d1a6a28a387.diff
LOG: Re-land "[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code"
This reverts commit ac9a76d7487b9af1ace626eb90064194cb12c53d.
Previously an abstract class has no pure virtual function. It causes build error on some bots.
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 13f28076c6f4d7..c865b2e8bdb379 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<const VarDecl *, std::vector<const VarDecl *>>;
+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 =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 8152eb53da71c2..4638f7422320b9 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<const VarDecl *, std::set<const FixableGadget *>> byVar;
+ 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;
};
static FixableGadgetSets
@@ -1382,7 +1386,7 @@ static std::optional<StringRef> 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<const VarDecl *, FixItList> 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<const VarDecl *, FixItList> 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<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) {
@@ -2408,7 +2425,6 @@ 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();
@@ -2451,7 +2467,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
UnsafeVars.push_back(VD);
// Fixpoint iteration for pointer assignments
- using DepMapTy = DenseMap<const VarDecl *, std::set<const VarDecl *>>;
+ using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
DepMapTy DependenciesMap{};
DepMapTy PtrAssignmentGraph{};
@@ -2460,7 +2476,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 = ImplPair.value();
+ std::pair<const VarDecl *, const VarDecl *> 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<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()) {
- std::vector<const VarDecl *> VarGroup{};
-
+ VarGrpTy &VarGroup = Groups.emplace_back();
std::queue<const VarDecl*> 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 07762997c9168c..addaca4db6d8b2 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<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) {}
@@ -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
More information about the cfe-commits
mailing list