Skip to content

Commit c56c349

Browse files
zygoloidzygoloid
authored
[clang-tidy] Switch misc-confusable-identifiers check to a faster algorithm. (#130369)
Optimizations: - Only build the skeleton for each identifier once, rather than once for each declaration of that identifier. - Only compute the contexts in which identifiers are declared for identifiers that have the same skeleton as another identifier in the translation unit. - Only compare pairs of declarations that are declared in related contexts, rather than comparing all pairs of declarations with the same skeleton. Also simplify by removing the caching of enclosing `DeclContext` sets, because with the above changes we don't even compute the enclosing `DeclContext` sets in common cases. Instead, we terminate the traversal to enclosing `DeclContext`s immediately if we've already found another declaration in that context with the same identifier. (This optimization is not currently applied to the `forallBases` traversal, but could be applied there too if needed.) This also fixes two bugs that together caused the check to fail to find some of the issues it was looking for: - The old check skipped comparisons of declarations from different contexts unless both declarations were type template parameters. This caused the checker to not warn on some instances of the CVE it is intended to detect. - The old check skipped comparisons of declarations in all base classes other than the first one found by the traversal. This appears to be an oversight, incorrectly returning `false` rather than `true` from the `forallBases` callback, which terminates traversal. This also fixes an issue where the check would have false positives for template parameters and function parameters in some cases, because those parameters sometimes have a parent `DeclContext` that is the parent of the parameterized entity, or sometimes is the translation unit. In either case, this would cause warnings about declarations that are never visible together in any scope. This decreases the runtime of this check, especially in the common case where there are few or no skeletons with two or more different identifiers. Running this check over LLVM, clang, and clang-tidy, the wall time for the check as reported by clang-tidy's internal profiler is reduced from 5202.86s to 3900.90s.
1 parent a061171 commit c56c349

File tree

3 files changed

+205
-114
lines changed

3 files changed

+205
-114
lines changed

‎clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp

Lines changed: 139 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
//===--- ConfusableIdentifierCheck.cpp -
2-
// clang-tidy--------------------------===//
1+
//===--- ConfusableIdentifierCheck.cpp - clang-tidy -----------------------===//
32
//
43
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
54
// See https://llvm.org/LICENSE.txt for license information.
@@ -9,6 +8,7 @@
98

109
#include "ConfusableIdentifierCheck.h"
1110

11+
#include "clang/ASTMatchers/ASTMatchers.h"
1212
#include "clang/Lex/Preprocessor.h"
1313
#include "llvm/ADT/SmallString.h"
1414
#include "llvm/Support/ConvertUTF.h"
@@ -88,134 +88,177 @@ static llvm::SmallString<64U> skeleton(StringRef Name) {
8888
return Skeleton;
8989
}
9090

91-
static bool mayShadowImpl(const DeclContext *DC0, const DeclContext *DC1) {
92-
return DC0 && DC0 == DC1;
93-
}
94-
95-
static bool mayShadowImpl(const NamedDecl *ND0, const NamedDecl *ND1) {
96-
return isa<TemplateTypeParmDecl>(ND0) || isa<TemplateTypeParmDecl>(ND1);
97-
}
98-
99-
static bool isMemberOf(const ConfusableIdentifierCheck::ContextInfo *DC0,
100-
const ConfusableIdentifierCheck::ContextInfo *DC1) {
101-
return llvm::is_contained(DC1->Bases, DC0->PrimaryContext);
102-
}
103-
104-
static bool enclosesContext(const ConfusableIdentifierCheck::ContextInfo *DC0,
105-
const ConfusableIdentifierCheck::ContextInfo *DC1) {
106-
if (DC0->PrimaryContext == DC1->PrimaryContext)
107-
return true;
108-
109-
return llvm::is_contained(DC0->PrimaryContexts, DC1->PrimaryContext) ||
110-
llvm::is_contained(DC1->PrimaryContexts, DC0->PrimaryContext);
111-
}
112-
113-
static bool mayShadow(const NamedDecl *ND0,
114-
const ConfusableIdentifierCheck::ContextInfo *DC0,
115-
const NamedDecl *ND1,
116-
const ConfusableIdentifierCheck::ContextInfo *DC1) {
117-
118-
if (!DC0->Bases.empty() && !DC1->Bases.empty()) {
119-
// if any of the declaration is a non-private member of the other
120-
// declaration, it's shadowed by the former
121-
122-
if (ND1->getAccess() != AS_private && isMemberOf(DC1, DC0))
123-
return true;
91+
namespace {
92+
struct Entry {
93+
const NamedDecl *ND;
94+
const Decl *Parent;
95+
bool FromDerivedClass;
96+
};
97+
} // namespace
12498

125-
if (ND0->getAccess() != AS_private && isMemberOf(DC0, DC1))
99+
// Map from a context to the declarations in that context with the current
100+
// skeleton. At most one entry per distinct identifier is tracked. The
101+
// context is usually a `DeclContext`, but can also be a template declaration
102+
// that has no corresponding context, such as an alias template or variable
103+
// template.
104+
using DeclsWithinContextMap =
105+
llvm::DenseMap<const Decl *, llvm::SmallVector<Entry, 1>>;
106+
107+
static bool addToContext(DeclsWithinContextMap &DeclsWithinContext,
108+
const Decl *Context, Entry E) {
109+
auto &Decls = DeclsWithinContext[Context];
110+
if (!Decls.empty() &&
111+
Decls.back().ND->getIdentifier() == E.ND->getIdentifier()) {
112+
// Already have a declaration with this identifier in this context. Don't
113+
// track another one. This means that if an outer name is confusable with an
114+
// inner name, we'll only diagnose the outer name once, pointing at the
115+
// first inner declaration with that name.
116+
if (Decls.back().FromDerivedClass && !E.FromDerivedClass) {
117+
// Prefer the declaration that's not from the derived class, because that
118+
// conflicts with more declarations.
119+
Decls.back() = E;
126120
return true;
127-
}
128-
129-
if (!mayShadowImpl(DC0->NonTransparentContext, DC1->NonTransparentContext) &&
130-
!mayShadowImpl(ND0, ND1))
121+
}
131122
return false;
132-
133-
return enclosesContext(DC0, DC1);
123+
}
124+
Decls.push_back(E);
125+
return true;
134126
}
135127

136-
const ConfusableIdentifierCheck::ContextInfo *
137-
ConfusableIdentifierCheck::getContextInfo(const DeclContext *DC) {
138-
const DeclContext *PrimaryContext = DC->getPrimaryContext();
139-
auto [It, Inserted] = ContextInfos.try_emplace(PrimaryContext);
140-
if (!Inserted)
141-
return &It->second;
142-
143-
ContextInfo &Info = It->second;
144-
Info.PrimaryContext = PrimaryContext;
145-
Info.NonTransparentContext = PrimaryContext;
128+
static void addToEnclosingContexts(DeclsWithinContextMap &DeclsWithinContext,
129+
const Decl *Parent, const NamedDecl *ND) {
130+
const Decl *Outer = Parent;
131+
while (Outer) {
132+
if (const auto *NS = dyn_cast<NamespaceDecl>(Outer))
133+
Outer = NS->getCanonicalDecl();
134+
135+
if (!addToContext(DeclsWithinContext, Outer, {ND, Parent, false}))
136+
return;
137+
138+
if (const auto *RD = dyn_cast<CXXRecordDecl>(Outer)) {
139+
RD = RD->getDefinition();
140+
if (RD) {
141+
RD->forallBases([&](const CXXRecordDecl *Base) {
142+
addToContext(DeclsWithinContext, Base, {ND, Parent, true});
143+
return true;
144+
});
145+
}
146+
}
146147

147-
while (Info.NonTransparentContext->isTransparentContext()) {
148-
Info.NonTransparentContext = Info.NonTransparentContext->getParent();
149-
if (!Info.NonTransparentContext)
148+
auto *OuterDC = Outer->getDeclContext();
149+
if (!OuterDC)
150150
break;
151+
Outer = cast_or_null<Decl>(OuterDC->getNonTransparentContext());
151152
}
153+
}
154+
155+
void ConfusableIdentifierCheck::check(
156+
const ast_matchers::MatchFinder::MatchResult &Result) {
157+
const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("nameddecl");
158+
if (!ND)
159+
return;
152160

153-
if (Info.NonTransparentContext)
154-
Info.NonTransparentContext =
155-
Info.NonTransparentContext->getPrimaryContext();
161+
addDeclToCheck(ND,
162+
cast<Decl>(ND->getDeclContext()->getNonTransparentContext()));
156163

157-
while (DC) {
158-
if (!isa<LinkageSpecDecl>(DC) && !isa<ExportDecl>(DC))
159-
Info.PrimaryContexts.push_back(DC->getPrimaryContext());
160-
DC = DC->getParent();
164+
// Associate template parameters with this declaration of this template.
165+
if (const auto *TD = dyn_cast<TemplateDecl>(ND)) {
166+
for (const NamedDecl *Param : *TD->getTemplateParameters())
167+
addDeclToCheck(Param, TD->getTemplatedDecl());
161168
}
162169

163-
if (const auto *RD = dyn_cast<CXXRecordDecl>(PrimaryContext)) {
164-
RD = RD->getDefinition();
165-
if (RD) {
166-
Info.Bases.push_back(RD);
167-
RD->forallBases([&](const CXXRecordDecl *Base) {
168-
Info.Bases.push_back(Base);
169-
return false;
170-
});
171-
}
170+
// Associate function parameters with this declaration of this function.
171+
if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
172+
for (const NamedDecl *Param : FD->parameters())
173+
addDeclToCheck(Param, ND);
172174
}
173-
174-
return &Info;
175175
}
176176

177-
void ConfusableIdentifierCheck::check(
178-
const ast_matchers::MatchFinder::MatchResult &Result) {
179-
const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("nameddecl");
180-
if (!ND)
177+
void ConfusableIdentifierCheck::addDeclToCheck(const NamedDecl *ND,
178+
const Decl *Parent) {
179+
if (!ND || !Parent)
181180
return;
182181

183-
IdentifierInfo *NDII = ND->getIdentifier();
182+
const IdentifierInfo *NDII = ND->getIdentifier();
184183
if (!NDII)
185184
return;
186185

187186
StringRef NDName = NDII->getName();
188187
if (NDName.empty())
189188
return;
190189

191-
const ContextInfo *Info = getContextInfo(ND->getDeclContext());
190+
NameToDecls[NDII].push_back({ND, Parent});
191+
}
192+
193+
void ConfusableIdentifierCheck::onEndOfTranslationUnit() {
194+
llvm::StringMap<llvm::SmallVector<const IdentifierInfo *, 1>> SkeletonToNames;
195+
// Compute the skeleton for each identifier.
196+
for (auto &[Ident, Decls] : NameToDecls) {
197+
SkeletonToNames[skeleton(Ident->getName())].push_back(Ident);
198+
}
192199

193-
llvm::SmallVector<Entry> &Mapped = Mapper[skeleton(NDName)];
194-
for (const Entry &E : Mapped) {
195-
if (!mayShadow(ND, Info, E.Declaration, E.Info))
200+
// Visit each skeleton with more than one identifier.
201+
for (auto &[Skel, Idents] : SkeletonToNames) {
202+
if (Idents.size() < 2) {
196203
continue;
204+
}
197205

198-
const IdentifierInfo *ONDII = E.Declaration->getIdentifier();
199-
StringRef ONDName = ONDII->getName();
200-
if (ONDName == NDName)
201-
continue;
206+
// Find the declaration contexts that transitively contain each identifier.
207+
DeclsWithinContextMap DeclsWithinContext;
208+
for (const IdentifierInfo *II : Idents) {
209+
for (auto [ND, Parent] : NameToDecls[II]) {
210+
addToEnclosingContexts(DeclsWithinContext, Parent, ND);
211+
}
212+
}
202213

203-
diag(ND->getLocation(), "%0 is confusable with %1") << ND << E.Declaration;
204-
diag(E.Declaration->getLocation(), "other declaration found here",
205-
DiagnosticIDs::Note);
214+
// Check to see if any declaration is declared in a context that
215+
// transitively contains another declaration with a different identifier but
216+
// the same skeleton.
217+
for (const IdentifierInfo *II : Idents) {
218+
for (auto [OuterND, OuterParent] : NameToDecls[II]) {
219+
for (Entry Inner : DeclsWithinContext[OuterParent]) {
220+
// Don't complain if the identifiers are the same.
221+
if (OuterND->getIdentifier() == Inner.ND->getIdentifier())
222+
continue;
223+
224+
// Don't complain about a derived-class name shadowing a base class
225+
// private member.
226+
if (OuterND->getAccess() == AS_private && Inner.FromDerivedClass)
227+
continue;
228+
229+
// If the declarations are in the same context, only diagnose the
230+
// later one.
231+
if (OuterParent == Inner.Parent &&
232+
Inner.ND->getASTContext()
233+
.getSourceManager()
234+
.isBeforeInTranslationUnit(Inner.ND->getLocation(),
235+
OuterND->getLocation()))
236+
continue;
237+
238+
diag(Inner.ND->getLocation(), "%0 is confusable with %1")
239+
<< Inner.ND << OuterND;
240+
diag(OuterND->getLocation(), "other declaration found here",
241+
DiagnosticIDs::Note);
242+
}
243+
}
244+
}
206245
}
207246

208-
Mapped.push_back({ND, Info});
209-
}
210-
211-
void ConfusableIdentifierCheck::onEndOfTranslationUnit() {
212-
Mapper.clear();
213-
ContextInfos.clear();
247+
NameToDecls.clear();
214248
}
215249

216250
void ConfusableIdentifierCheck::registerMatchers(
217251
ast_matchers::MatchFinder *Finder) {
218-
Finder->addMatcher(ast_matchers::namedDecl().bind("nameddecl"), this);
252+
// Parameter declarations sometimes use the translation unit or some outer
253+
// enclosing context as their `DeclContext`, instead of their parent, so
254+
// we handle them specially in `check`.
255+
auto AnyParamDecl = ast_matchers::anyOf(
256+
ast_matchers::parmVarDecl(), ast_matchers::templateTypeParmDecl(),
257+
ast_matchers::nonTypeTemplateParmDecl(),
258+
ast_matchers::templateTemplateParmDecl());
259+
Finder->addMatcher(ast_matchers::namedDecl(ast_matchers::unless(AnyParamDecl))
260+
.bind("nameddecl"),
261+
this);
219262
}
220263

221264
} // namespace clang::tidy::misc

‎clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
//===--- ConfusableIdentifierCheck.h - clang-tidy
2-
//-------------------------------*- C++ -*-===//
1+
//===--- ConfusableIdentifierCheck.h - clang-tidy ---------------*- C++ -*-===//
32
//
43
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
54
// See https://llvm.org/LICENSE.txt for license information.
@@ -11,7 +10,7 @@
1110
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONFUSABLE_IDENTIFIER_CHECK_H
1211

1312
#include "../ClangTidyCheck.h"
14-
#include <unordered_map>
13+
#include "llvm/ADT/DenseMap.h"
1514

1615
namespace clang::tidy::misc {
1716

@@ -31,23 +30,13 @@ class ConfusableIdentifierCheck : public ClangTidyCheck {
3130
return TK_IgnoreUnlessSpelledInSource;
3231
}
3332

34-
struct ContextInfo {
35-
const DeclContext *PrimaryContext;
36-
const DeclContext *NonTransparentContext;
37-
llvm::SmallVector<const DeclContext *> PrimaryContexts;
38-
llvm::SmallVector<const CXXRecordDecl *> Bases;
39-
};
40-
4133
private:
42-
struct Entry {
43-
const NamedDecl *Declaration;
44-
const ContextInfo *Info;
45-
};
46-
47-
const ContextInfo *getContextInfo(const DeclContext *DC);
34+
void addDeclToCheck(const NamedDecl *ND, const Decl *Parent);
4835

49-
llvm::StringMap<llvm::SmallVector<Entry>> Mapper;
50-
std::unordered_map<const DeclContext *, ContextInfo> ContextInfos;
36+
llvm::DenseMap<
37+
const IdentifierInfo *,
38+
llvm::SmallVector<std::pair<const NamedDecl *, const Decl *>, 1>>
39+
NameToDecls;
5140
};
5241

5342
} // namespace clang::tidy::misc

0 commit comments

Comments
 (0)