ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: NoQ, jkorous, t-rasmud, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Refactor the `getFixIts` function for better readability.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156762
Files:
clang/lib/Analysis/UnsafeBufferUsage.cpp
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2142,92 +2142,85 @@
});
}
-static bool impossibleToFixForVar(const FixableGadgetSets &FixablesForAllVars,
- const Strategy &S,
- const VarDecl * Var) {
- for (const auto &F : FixablesForAllVars.byVar.find(Var)->second) {
- std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
- return true;
+// 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<const VarDecl *, FixItList> &FixItsForVariable,
+ const VariableGroupsManager &VarGrpMgr) {
+ // Variables will be removed from `FixItsForVariable`:
+ SmallVector<const VarDecl *, 8> 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);
}
static std::map<const VarDecl *, FixItList>
getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
ASTContext &Ctx,
- /* The function decl under analysis */ const Decl *D,
+ /* The function decl under analysis */ const FunctionDecl *FD,
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<const VarDecl *, FixItList> 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);
+ fixVariable(VD, S.lookup(VD), FD, Tracker, Ctx, Handler);
// If we fail to produce Fix-It for the declaration we have to skip the
// variable entirely.
if (FixItsForVariable[VD].empty()) {
FixItsForVariable.erase(VD);
continue;
}
- bool ImpossibleToFix = false;
- llvm::SmallVector<FixItHint, 16> FixItsForVD;
for (const auto &F : Fixables) {
std::optional<FixItList> Fixits = F->getFixits(S);
- if (!Fixits) {
-#ifndef NDEBUG
- Handler.addDebugNoteForVar(
- VD, F->getBaseStmt()->getBeginLoc(),
- ("gadget '" + F->getDebugName() + "' refused to produce a fix")
- .str());
-#endif
- 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 = VarGrpMgr.getGroupOfVar(VD);
- for (const VarDecl * V : VarGroupForVD) {
- if (V == VD) {
- continue;
- }
- if (impossibleToFixForVar(FixablesForAllVars, S, V)) {
- ImpossibleToFix = true;
- break;
- }
- }
-
- if (ImpossibleToFix) {
- FixItsForVariable.erase(VD);
- for (const VarDecl * V : VarGroupForVD) {
- 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())) {
+#ifndef NDEBUG
+ Handler.addDebugNoteForVar(
+ VD, F->getBaseStmt()->getBeginLoc(),
+ ("gadget '" + F->getDebugName() + "' refused to produce a fix")
+ .str());
+#endif
FixItsForVariable.erase(VD);
- continue;
+ break;
}
}
+ // `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.
+
// The map that maps each variable `v` to fix-its for the whole group where
// `v` is in:
std::map<const VarDecl *, FixItList> FinalFixItsForVariable{
@@ -2245,10 +2238,20 @@
FixItsForVariable[GrpMate].end());
}
}
+ // 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<const VarDecl *> &UnsafeVars) {
Strategy S;
@@ -2287,6 +2290,7 @@
// We do not want to visit a Lambda expression defined inside a method independently.
// Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
return;
@@ -2519,9 +2523,14 @@
Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap);
- FixItsForVariableGroup =
- getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D,
- Tracker, Handler, VarGrpMgr);
+ if (isa<FunctionDecl>(D))
+ // The only case where `D` is not a `FunctionDecl` is when `D` is a
+ // `BlockDecl`. Let's NOT try to fix variables in blocks for now. Becuase
+ // those variables could be declared implicitly (captured variables) or in
+ // enclosing scopes.
+ FixItsForVariableGroup =
+ getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(),
+ cast<FunctionDecl>(D), Tracker, Handler, VarGrpMgr);
for (const auto &G : UnsafeOps.noVar) {
Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
