Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize the code
Browse files Browse the repository at this point in the history 
Slightly refactor and optimize the code in preparation for
implementing grouping parameters for a single fix-it.

Reviewed by: NoQ (Artem Dergachev), t-rasmud (Rashmi Mudduluru)

Differential revision: https://reviews.llvm.org/D156474
  • Loading branch information
@ziqingluo-90
ziqingluo-90 committed Aug 17, 2023
1 parent d37642b commit 8437847
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 86 deletions.
16 changes: 14 additions & 2 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

/// The interface that lets the caller handle unsafe buffer usage analysis
/// results by overriding this class's handle... methods.
Expand Down Expand Up @@ -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
Expand Down
102 changes: 62 additions & 40 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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] =
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand All @@ -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;
}


Expand All @@ -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) {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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{};

Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
}
}
Expand Down Expand Up @@ -2563,20 +2586,19 @@ 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);
}

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{});
Expand Down
86 changes: 42 additions & 44 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8437847

Please sign in to comment.