Skip to content

Commit

Permalink
Re-land "[-Wunsafe-buffer-usage][NFC] Slightly refactor and optimize …
Browse files Browse the repository at this point in the history 
…the code"

This reverts commit ac9a76d.

Previously an abstract class has no pure virtual function.  It causes build error on some bots.
  • Loading branch information
@ziqingluo-90
ziqingluo-90 committed Aug 18, 2023
1 parent 07bb667 commit 472a510
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 =0;
};

/// 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 472a510

Please sign in to comment.