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