1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
|
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// This file defines a bunch of recurring problems in the Chromium C++ code.
//
// Checks that are implemented:
// - Constructors/Destructors should not be inlined if they are of a complex
// class type.
// - Missing "virtual" keywords on methods that should be virtual.
// - Non-annotated overriding virtual methods.
// - Virtual methods with nonempty implementations in their headers.
// - Classes that derive from base::RefCounted / base::RefCountedThreadSafe
// should have protected or private destructors.
#include "clang/Frontend/FrontendPluginRegistry.h"
#include "clang/AST/ASTConsumer.h"
#include "clang/AST/AST.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/CompilerInstance.h"
#include "llvm/Support/raw_ostream.h"
#include "ChromeClassTester.h"
using namespace clang;
namespace {
bool TypeHasNonTrivialDtor(const Type* type) {
if (const CXXRecordDecl* cxx_r = type->getCXXRecordDeclForPointerType())
return cxx_r->hasTrivialDestructor();
return false;
}
// Returns the underlying Type for |type| by expanding typedefs and removing
// any namespace qualifiers.
const Type* UnwrapType(const Type* type) {
if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
return UnwrapType(elaborated->getNamedType().getTypePtr());
if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
return UnwrapType(typedefed->desugar().getTypePtr());
return type;
}
// Searches for constructs that we know we don't want in the Chromium code base.
class FindBadConstructsConsumer : public ChromeClassTester {
public:
FindBadConstructsConsumer(CompilerInstance& instance,
bool check_refcounted_dtors,
bool check_virtuals_in_implementations)
: ChromeClassTester(instance),
check_refcounted_dtors_(check_refcounted_dtors),
check_virtuals_in_implementations_(check_virtuals_in_implementations) {
}
virtual void CheckChromeClass(SourceLocation record_location,
CXXRecordDecl* record) {
bool implementation_file = InImplementationFile(record_location);
if (!implementation_file) {
// Only check for "heavy" constructors/destructors in header files;
// within implementation files, there is no performance cost.
CheckCtorDtorWeight(record_location, record);
}
if (!implementation_file || check_virtuals_in_implementations_) {
bool warn_on_inline_bodies = !implementation_file;
// Check that all virtual methods are marked accordingly with both
// virtual and OVERRIDE.
CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
}
if (check_refcounted_dtors_)
CheckRefCountedDtors(record_location, record);
}
private:
bool check_refcounted_dtors_;
bool check_virtuals_in_implementations_;
// Returns true if |base| specifies one of the Chromium reference counted
// classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
// ignored.
static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
CXXBasePath& path,
void* user_data) {
FindBadConstructsConsumer* self =
static_cast<FindBadConstructsConsumer*>(user_data);
const TemplateSpecializationType* base_type =
dyn_cast<TemplateSpecializationType>(
UnwrapType(base->getType().getTypePtr()));
if (!base_type) {
// Base-most definition is not a template, so this cannot derive from
// base::RefCounted. However, it may still be possible to use with a
// scoped_refptr<> and support ref-counting, so this is not a perfect
// guarantee of safety.
return false;
}
TemplateName name = base_type->getTemplateName();
if (TemplateDecl* decl = name.getAsTemplateDecl()) {
std::string base_name = decl->getNameAsString();
// Check for both base::RefCounted and base::RefCountedThreadSafe.
if (base_name.compare(0, 10, "RefCounted") == 0 &&
self->GetNamespace(decl) == "base") {
return true;
}
}
return false;
}
// Prints errors if the destructor of a RefCounted class is public.
void CheckRefCountedDtors(SourceLocation record_location,
CXXRecordDecl* record) {
// Skip anonymous structs.
if (record->getIdentifier() == NULL)
return;
CXXBasePaths paths;
if (!record->lookupInBases(
&FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) {
return; // Class does not derive from a ref-counted base class.
}
if (!record->hasUserDeclaredDestructor()) {
emitWarning(
record_location,
"Classes that are ref-counted should have explicit "
"destructors that are protected or private.");
} else if (CXXDestructorDecl* dtor = record->getDestructor()) {
if (dtor->getAccess() == AS_public) {
emitWarning(
dtor->getInnerLocStart(),
"Classes that are ref-counted should not have "
"public destructors.");
}
}
}
// Prints errors if the constructor/destructor weight is too heavy.
void CheckCtorDtorWeight(SourceLocation record_location,
CXXRecordDecl* record) {
// We don't handle anonymous structs. If this record doesn't have a
// name, it's of the form:
//
// struct {
// ...
// } name_;
if (record->getIdentifier() == NULL)
return;
// Count the number of templated base classes as a feature of whether the
// destructor can be inlined.
int templated_base_classes = 0;
for (CXXRecordDecl::base_class_const_iterator it = record->bases_begin();
it != record->bases_end(); ++it) {
if (it->getTypeSourceInfo()->getTypeLoc().getTypeLocClass() ==
TypeLoc::TemplateSpecialization) {
++templated_base_classes;
}
}
// Count the number of trivial and non-trivial member variables.
int trivial_member = 0;
int non_trivial_member = 0;
int templated_non_trivial_member = 0;
for (RecordDecl::field_iterator it = record->field_begin();
it != record->field_end(); ++it) {
CountType(it->getType().getTypePtr(),
&trivial_member,
&non_trivial_member,
&templated_non_trivial_member);
}
// Check to see if we need to ban inlined/synthesized constructors. Note
// that the cutoffs here are kind of arbitrary. Scores over 10 break.
int dtor_score = 0;
// Deriving from a templated base class shouldn't be enough to trigger
// the ctor warning, but if you do *anything* else, it should.
//
// TODO(erg): This is motivated by templated base classes that don't have
// any data members. Somehow detect when templated base classes have data
// members and treat them differently.
dtor_score += templated_base_classes * 9;
// Instantiating a template is an insta-hit.
dtor_score += templated_non_trivial_member * 10;
// The fourth normal class member should trigger the warning.
dtor_score += non_trivial_member * 3;
int ctor_score = dtor_score;
// You should be able to have 9 ints before we warn you.
ctor_score += trivial_member;
if (ctor_score >= 10) {
if (!record->hasUserDeclaredConstructor()) {
emitWarning(record_location,
"Complex class/struct needs an explicit out-of-line "
"constructor.");
} else {
// Iterate across all the constructors in this file and yell if we
// find one that tries to be inline.
for (CXXRecordDecl::ctor_iterator it = record->ctor_begin();
it != record->ctor_end(); ++it) {
if (it->hasInlineBody()) {
if (it->isCopyConstructor() &&
!record->hasUserDeclaredCopyConstructor()) {
emitWarning(record_location,
"Complex class/struct needs an explicit out-of-line "
"copy constructor.");
} else {
emitWarning(it->getInnerLocStart(),
"Complex constructor has an inlined body.");
}
}
}
}
}
// The destructor side is equivalent except that we don't check for
// trivial members; 20 ints don't need a destructor.
if (dtor_score >= 10 && !record->hasTrivialDestructor()) {
if (!record->hasUserDeclaredDestructor()) {
emitWarning(
record_location,
"Complex class/struct needs an explicit out-of-line "
"destructor.");
} else if (CXXDestructorDecl* dtor = record->getDestructor()) {
if (dtor->hasInlineBody()) {
emitWarning(dtor->getInnerLocStart(),
"Complex destructor has an inline body.");
}
}
}
}
void CheckVirtualMethod(const CXXMethodDecl* method,
bool warn_on_inline_bodies) {
if (!method->isVirtual())
return;
if (!method->isVirtualAsWritten()) {
SourceLocation loc = method->getTypeSpecStartLoc();
if (isa<CXXDestructorDecl>(method))
loc = method->getInnerLocStart();
emitWarning(loc, "Overriding method must have \"virtual\" keyword.");
}
// Virtual methods should not have inline definitions beyond "{}". This
// only matters for header files.
if (warn_on_inline_bodies && method->hasBody() &&
method->hasInlineBody()) {
if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
if (cs->size()) {
emitWarning(
cs->getLBracLoc(),
"virtual methods with non-empty bodies shouldn't be "
"declared inline.");
}
}
}
}
bool InTestingNamespace(const Decl* record) {
return GetNamespace(record).find("testing") != std::string::npos;
}
bool IsMethodInBannedNamespace(const CXXMethodDecl* method) {
if (InBannedNamespace(method))
return true;
for (CXXMethodDecl::method_iterator i = method->begin_overridden_methods();
i != method->end_overridden_methods();
++i) {
const CXXMethodDecl* overridden = *i;
if (IsMethodInBannedNamespace(overridden))
return true;
}
return false;
}
void CheckOverriddenMethod(const CXXMethodDecl* method) {
if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>())
return;
if (isa<CXXDestructorDecl>(method) || method->isPure())
return;
if (IsMethodInBannedNamespace(method))
return;
SourceLocation loc = method->getTypeSpecStartLoc();
emitWarning(loc, "Overriding method must be marked with OVERRIDE.");
}
// Makes sure there is a "virtual" keyword on virtual methods.
//
// Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
// trick to get around that. If a class has member variables whose types are
// in the "testing" namespace (which is how gmock works behind the scenes),
// there's a really high chance we won't care about these errors
void CheckVirtualMethods(SourceLocation record_location,
CXXRecordDecl* record,
bool warn_on_inline_bodies) {
for (CXXRecordDecl::field_iterator it = record->field_begin();
it != record->field_end(); ++it) {
CXXRecordDecl* record_type =
it->getTypeSourceInfo()->getTypeLoc().getTypePtr()->
getAsCXXRecordDecl();
if (record_type) {
if (InTestingNamespace(record_type)) {
return;
}
}
}
for (CXXRecordDecl::method_iterator it = record->method_begin();
it != record->method_end(); ++it) {
if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) {
// Ignore constructors and assignment operators.
} else if (isa<CXXDestructorDecl>(*it) &&
!record->hasUserDeclaredDestructor()) {
// Ignore non-user-declared destructors.
} else {
CheckVirtualMethod(*it, warn_on_inline_bodies);
CheckOverriddenMethod(*it);
}
}
}
void CountType(const Type* type,
int* trivial_member,
int* non_trivial_member,
int* templated_non_trivial_member) {
switch (type->getTypeClass()) {
case Type::Record: {
// Simplifying; the whole class isn't trivial if the dtor is, but
// we use this as a signal about complexity.
if (TypeHasNonTrivialDtor(type))
(*trivial_member)++;
else
(*non_trivial_member)++;
break;
}
case Type::TemplateSpecialization: {
TemplateName name =
dyn_cast<TemplateSpecializationType>(type)->getTemplateName();
bool whitelisted_template = false;
// HACK: I'm at a loss about how to get the syntax checker to get
// whether a template is exterened or not. For the first pass here,
// just do retarded string comparisons.
if (TemplateDecl* decl = name.getAsTemplateDecl()) {
std::string base_name = decl->getNameAsString();
if (base_name == "basic_string")
whitelisted_template = true;
}
if (whitelisted_template)
(*non_trivial_member)++;
else
(*templated_non_trivial_member)++;
break;
}
case Type::Elaborated: {
CountType(
dyn_cast<ElaboratedType>(type)->getNamedType().getTypePtr(),
trivial_member, non_trivial_member, templated_non_trivial_member);
break;
}
case Type::Typedef: {
while (const TypedefType* TT = dyn_cast<TypedefType>(type)) {
type = TT->getDecl()->getUnderlyingType().getTypePtr();
}
CountType(type, trivial_member, non_trivial_member,
templated_non_trivial_member);
break;
}
default: {
// Stupid assumption: anything we see that isn't the above is one of
// the 20 integer types.
(*trivial_member)++;
break;
}
}
}
};
class FindBadConstructsAction : public PluginASTAction {
public:
FindBadConstructsAction()
: check_refcounted_dtors_(true),
check_virtuals_in_implementations_(true) {
}
protected:
// Overridden from PluginASTAction:
virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance,
llvm::StringRef ref) {
return new FindBadConstructsConsumer(
instance, check_refcounted_dtors_, check_virtuals_in_implementations_);
}
virtual bool ParseArgs(const CompilerInstance& instance,
const std::vector<std::string>& args) {
bool parsed = true;
for (size_t i = 0; i < args.size() && parsed; ++i) {
if (args[i] == "skip-refcounted-dtors") {
check_refcounted_dtors_ = false;
} else if (args[i] == "skip-virtuals-in-implementations") {
check_virtuals_in_implementations_ = false;
} else {
parsed = false;
llvm::errs() << "Unknown argument: " << args[i] << "\n";
}
}
return parsed;
}
private:
bool check_refcounted_dtors_;
bool check_virtuals_in_implementations_;
};
} // namespace
static FrontendPluginRegistry::Add<FindBadConstructsAction>
X("find-bad-constructs", "Finds bad C++ constructs");
|