OLD | NEW |
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved. | 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 | 2 // Use of this source code is governed by a BSD-style license that can be |
3 // found in the LICENSE file. | 3 // found in the LICENSE file. |
4 | 4 |
5 // This file defines a bunch of recurring problems in the Chromium C++ code. | 5 // This file defines a bunch of recurring problems in the Chromium C++ code. |
6 // | 6 // |
7 // Checks that are implemented: | 7 // Checks that are implemented: |
8 // - Constructors/Destructors should not be inlined if they are of a complex | 8 // - Constructors/Destructors should not be inlined if they are of a complex |
9 // class type. | 9 // class type. |
10 // - Missing "virtual" keywords on methods that should be virtual. | 10 // - Missing "virtual" keywords on methods that should be virtual. |
(...skipping 11 matching lines...) Expand all Loading... |
22 #include "clang/Frontend/CompilerInstance.h" | 22 #include "clang/Frontend/CompilerInstance.h" |
23 #include "clang/Frontend/FrontendPluginRegistry.h" | 23 #include "clang/Frontend/FrontendPluginRegistry.h" |
24 #include "llvm/Support/raw_ostream.h" | 24 #include "llvm/Support/raw_ostream.h" |
25 | 25 |
26 #include "ChromeClassTester.h" | 26 #include "ChromeClassTester.h" |
27 | 27 |
28 using namespace clang; | 28 using namespace clang; |
29 | 29 |
30 namespace { | 30 namespace { |
31 | 31 |
| 32 const char kNoExplicitDtor[] = |
| 33 "[chromium-style] Classes that are ref-counted should have explicit " |
| 34 "destructors that are declared protected or private."; |
| 35 const char kPublicDtor[] = |
| 36 "[chromium-style] Classes that are ref-counted should not have " |
| 37 "public destructors."; |
| 38 const char kNoteInheritance[] = |
| 39 "[chromium-style] %0 inherits from %1 here"; |
| 40 const char kNoteImplicitDtor[] = |
| 41 "[chromium-style] No explicit destructor for %0 defined"; |
| 42 const char kNotePublicDtor[] = |
| 43 "[chromium-style] Public destructor declared here"; |
| 44 |
32 bool TypeHasNonTrivialDtor(const Type* type) { | 45 bool TypeHasNonTrivialDtor(const Type* type) { |
33 if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) | 46 if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) |
34 return cxx_r->hasTrivialDestructor(); | 47 return cxx_r->hasTrivialDestructor(); |
35 | 48 |
36 return false; | 49 return false; |
37 } | 50 } |
38 | 51 |
39 // Returns the underlying Type for |type| by expanding typedefs and removing | 52 // Returns the underlying Type for |type| by expanding typedefs and removing |
40 // any namespace qualifiers. | 53 // any namespace qualifiers. This is similar to desugaring, except that for |
| 54 // ElaboratedTypes, desugar will unwrap too much. |
41 const Type* UnwrapType(const Type* type) { | 55 const Type* UnwrapType(const Type* type) { |
42 if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type)) | 56 if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type)) |
43 return UnwrapType(elaborated->getNamedType().getTypePtr()); | 57 return UnwrapType(elaborated->getNamedType().getTypePtr()); |
44 if (const TypedefType* typedefed = dyn_cast<TypedefType>(type)) | 58 if (const TypedefType* typedefed = dyn_cast<TypedefType>(type)) |
45 return UnwrapType(typedefed->desugar().getTypePtr()); | 59 return UnwrapType(typedefed->desugar().getTypePtr()); |
46 return type; | 60 return type; |
47 } | 61 } |
48 | 62 |
49 // Searches for constructs that we know we don't want in the Chromium code base. | 63 // Searches for constructs that we know we don't want in the Chromium code base. |
50 class FindBadConstructsConsumer : public ChromeClassTester { | 64 class FindBadConstructsConsumer : public ChromeClassTester { |
51 public: | 65 public: |
52 FindBadConstructsConsumer(CompilerInstance& instance, | 66 FindBadConstructsConsumer(CompilerInstance& instance, |
53 bool check_refcounted_dtors, | 67 bool check_base_classes, |
54 bool check_virtuals_in_implementations) | 68 bool check_virtuals_in_implementations) |
55 : ChromeClassTester(instance), | 69 : ChromeClassTester(instance), |
56 check_refcounted_dtors_(check_refcounted_dtors), | 70 check_base_classes_(check_base_classes), |
57 check_virtuals_in_implementations_(check_virtuals_in_implementations) { | 71 check_virtuals_in_implementations_(check_virtuals_in_implementations) { |
| 72 // Register warning/error messages. |
| 73 diag_no_explicit_dtor_ = diagnostic().getCustomDiagID( |
| 74 getErrorLevel(), kNoExplicitDtor); |
| 75 diag_public_dtor_ = diagnostic().getCustomDiagID( |
| 76 getErrorLevel(), kPublicDtor); |
| 77 |
| 78 // Registers notes to make it easier to interpret warnings. |
| 79 diag_note_inheritance_ = diagnostic().getCustomDiagID( |
| 80 DiagnosticsEngine::Note, kNoteInheritance); |
| 81 diag_note_implicit_dtor_ = diagnostic().getCustomDiagID( |
| 82 DiagnosticsEngine::Note, kNoteImplicitDtor); |
| 83 diag_note_public_dtor_ = diagnostic().getCustomDiagID( |
| 84 DiagnosticsEngine::Note, kNotePublicDtor); |
58 } | 85 } |
59 | 86 |
60 virtual void CheckChromeClass(SourceLocation record_location, | 87 virtual void CheckChromeClass(SourceLocation record_location, |
61 CXXRecordDecl* record) { | 88 CXXRecordDecl* record) { |
62 bool implementation_file = InImplementationFile(record_location); | 89 bool implementation_file = InImplementationFile(record_location); |
63 | 90 |
64 if (!implementation_file) { | 91 if (!implementation_file) { |
65 // Only check for "heavy" constructors/destructors in header files; | 92 // Only check for "heavy" constructors/destructors in header files; |
66 // within implementation files, there is no performance cost. | 93 // within implementation files, there is no performance cost. |
67 CheckCtorDtorWeight(record_location, record); | 94 CheckCtorDtorWeight(record_location, record); |
68 } | 95 } |
69 | 96 |
70 if (!implementation_file || check_virtuals_in_implementations_) { | 97 if (!implementation_file || check_virtuals_in_implementations_) { |
71 bool warn_on_inline_bodies = !implementation_file; | 98 bool warn_on_inline_bodies = !implementation_file; |
72 | 99 |
73 // Check that all virtual methods are marked accordingly with both | 100 // Check that all virtual methods are marked accordingly with both |
74 // virtual and OVERRIDE. | 101 // virtual and OVERRIDE. |
75 CheckVirtualMethods(record_location, record, warn_on_inline_bodies); | 102 CheckVirtualMethods(record_location, record, warn_on_inline_bodies); |
76 } | 103 } |
77 | 104 |
78 if (check_refcounted_dtors_) | 105 CheckRefCountedDtors(record_location, record); |
79 CheckRefCountedDtors(record_location, record); | |
80 } | 106 } |
81 | 107 |
82 private: | 108 private: |
83 bool check_refcounted_dtors_; | 109 // The type of problematic ref-counting pattern that was encountered. |
| 110 enum RefcountIssue { |
| 111 None, |
| 112 ImplicitDestructor, |
| 113 PublicDestructor |
| 114 }; |
| 115 |
| 116 bool check_base_classes_; |
84 bool check_virtuals_in_implementations_; | 117 bool check_virtuals_in_implementations_; |
85 | 118 |
86 // Returns true if |base| specifies one of the Chromium reference counted | 119 unsigned diag_no_explicit_dtor_; |
87 // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is | 120 unsigned diag_public_dtor_; |
88 // ignored. | 121 unsigned diag_note_inheritance_; |
89 static bool IsRefCountedCallback(const CXXBaseSpecifier* base, | 122 unsigned diag_note_implicit_dtor_; |
90 CXXBasePath& path, | 123 unsigned diag_note_public_dtor_; |
91 void* user_data) { | |
92 FindBadConstructsConsumer* self = | |
93 static_cast<FindBadConstructsConsumer*>(user_data); | |
94 | |
95 const TemplateSpecializationType* base_type = | |
96 dyn_cast<TemplateSpecializationType>( | |
97 UnwrapType(base->getType().getTypePtr())); | |
98 if (!base_type) { | |
99 // Base-most definition is not a template, so this cannot derive from | |
100 // base::RefCounted. However, it may still be possible to use with a | |
101 // scoped_refptr<> and support ref-counting, so this is not a perfect | |
102 // guarantee of safety. | |
103 return false; | |
104 } | |
105 | |
106 TemplateName name = base_type->getTemplateName(); | |
107 if (TemplateDecl* decl = name.getAsTemplateDecl()) { | |
108 std::string base_name = decl->getNameAsString(); | |
109 | |
110 // Check for both base::RefCounted and base::RefCountedThreadSafe. | |
111 if (base_name.compare(0, 10, "RefCounted") == 0 && | |
112 self->GetNamespace(decl) == "base") { | |
113 return true; | |
114 } | |
115 } | |
116 return false; | |
117 } | |
118 | |
119 // Prints errors if the destructor of a RefCounted class is public. | |
120 void CheckRefCountedDtors(SourceLocation record_location, | |
121 CXXRecordDecl* record) { | |
122 // Skip anonymous structs. | |
123 if (record->getIdentifier() == NULL) | |
124 return; | |
125 | |
126 CXXBasePaths paths; | |
127 if (!record->lookupInBases( | |
128 &FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) { | |
129 return; // Class does not derive from a ref-counted base class. | |
130 } | |
131 | |
132 if (!record->hasUserDeclaredDestructor()) { | |
133 emitWarning( | |
134 record_location, | |
135 "Classes that are ref-counted should have explicit " | |
136 "destructors that are protected or private."); | |
137 } else if (CXXDestructorDecl* dtor = record->getDestructor()) { | |
138 if (dtor->getAccess() == AS_public) { | |
139 emitWarning( | |
140 dtor->getInnerLocStart(), | |
141 "Classes that are ref-counted should not have " | |
142 "public destructors."); | |
143 } | |
144 } | |
145 } | |
146 | 124 |
147 // Prints errors if the constructor/destructor weight is too heavy. | 125 // Prints errors if the constructor/destructor weight is too heavy. |
148 void CheckCtorDtorWeight(SourceLocation record_location, | 126 void CheckCtorDtorWeight(SourceLocation record_location, |
149 CXXRecordDecl* record) { | 127 CXXRecordDecl* record) { |
150 // We don't handle anonymous structs. If this record doesn't have a | 128 // We don't handle anonymous structs. If this record doesn't have a |
151 // name, it's of the form: | 129 // name, it's of the form: |
152 // | 130 // |
153 // struct { | 131 // struct { |
154 // ... | 132 // ... |
155 // } name_; | 133 // } name_; |
(...skipping 227 matching lines...) Expand 10 before | Expand all | Expand 10 after Loading... |
383 break; | 361 break; |
384 } | 362 } |
385 default: { | 363 default: { |
386 // Stupid assumption: anything we see that isn't the above is one of | 364 // Stupid assumption: anything we see that isn't the above is one of |
387 // the 20 integer types. | 365 // the 20 integer types. |
388 (*trivial_member)++; | 366 (*trivial_member)++; |
389 break; | 367 break; |
390 } | 368 } |
391 } | 369 } |
392 } | 370 } |
| 371 |
| 372 // Check |record| for issues that are problematic for ref-counted types. |
| 373 // Note that |record| may not be a ref-counted type, but a base class for |
| 374 // a type that is. |
| 375 // If there are issues, update |loc| with the SourceLocation of the issue |
| 376 // and returns appropriately, or returns None if there are no issues. |
| 377 static RefcountIssue CheckRecordForRefcountIssue( |
| 378 const CXXRecordDecl* record, |
| 379 SourceLocation &loc) { |
| 380 if (!record->hasUserDeclaredDestructor()) { |
| 381 loc = record->getLocation(); |
| 382 return ImplicitDestructor; |
| 383 } |
| 384 |
| 385 if (CXXDestructorDecl* dtor = record->getDestructor()) { |
| 386 if (dtor->getAccess() == AS_public) { |
| 387 loc = dtor->getInnerLocStart(); |
| 388 return PublicDestructor; |
| 389 } |
| 390 } |
| 391 |
| 392 return None; |
| 393 } |
| 394 |
| 395 // Adds either a warning or error, based on the current handling of |
| 396 // -Werror. |
| 397 DiagnosticsEngine::Level getErrorLevel() { |
| 398 return diagnostic().getWarningsAsErrors() ? |
| 399 DiagnosticsEngine::Error : DiagnosticsEngine::Warning; |
| 400 } |
| 401 |
| 402 // Returns true if |base| specifies one of the Chromium reference counted |
| 403 // classes (base::RefCounted / base::RefCountedThreadSafe). |
| 404 static bool IsRefCountedCallback(const CXXBaseSpecifier* base, |
| 405 CXXBasePath& path, |
| 406 void* user_data) { |
| 407 FindBadConstructsConsumer* self = |
| 408 static_cast<FindBadConstructsConsumer*>(user_data); |
| 409 |
| 410 const TemplateSpecializationType* base_type = |
| 411 dyn_cast<TemplateSpecializationType>( |
| 412 UnwrapType(base->getType().getTypePtr())); |
| 413 if (!base_type) { |
| 414 // Base-most definition is not a template, so this cannot derive from |
| 415 // base::RefCounted. However, it may still be possible to use with a |
| 416 // scoped_refptr<> and support ref-counting, so this is not a perfect |
| 417 // guarantee of safety. |
| 418 return false; |
| 419 } |
| 420 |
| 421 TemplateName name = base_type->getTemplateName(); |
| 422 if (TemplateDecl* decl = name.getAsTemplateDecl()) { |
| 423 std::string base_name = decl->getNameAsString(); |
| 424 |
| 425 // Check for both base::RefCounted and base::RefCountedThreadSafe. |
| 426 if (base_name.compare(0, 10, "RefCounted") == 0 && |
| 427 self->GetNamespace(decl) == "base") { |
| 428 return true; |
| 429 } |
| 430 } |
| 431 |
| 432 return false; |
| 433 } |
| 434 |
| 435 // Returns true if |base| specifies a class that has a public destructor, |
| 436 // either explicitly or implicitly. |
| 437 static bool HasPublicDtorCallback(const CXXBaseSpecifier* base, |
| 438 CXXBasePath& path, |
| 439 void* user_data) { |
| 440 // Only examine paths that have public inheritance, as they are the |
| 441 // only ones which will result in the destructor potentially being |
| 442 // exposed. This check is largely redundant, as Chromium code should be |
| 443 // exclusively using public inheritance. |
| 444 if (path.Access != AS_public) |
| 445 return false; |
| 446 |
| 447 CXXRecordDecl* record = dyn_cast<CXXRecordDecl>( |
| 448 base->getType()->getAs<RecordType>()->getDecl()); |
| 449 SourceLocation unused; |
| 450 return None != CheckRecordForRefcountIssue(record, unused); |
| 451 } |
| 452 |
| 453 // Outputs a C++ inheritance chain as a diagnostic aid. |
| 454 void PrintInheritanceChain(const CXXBasePath& path) { |
| 455 for (CXXBasePath::const_iterator it = path.begin(); it != path.end(); |
| 456 ++it) { |
| 457 diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_) |
| 458 << it->Class << it->Base->getType(); |
| 459 } |
| 460 } |
| 461 |
| 462 // Check |record| to determine if it has any problematic refcounting |
| 463 // issues and, if so, print them as warnings/errors based on the current |
| 464 // value of getErrorLevel(). |
| 465 // |
| 466 // If |record| is a C++ class, and if it inherits from one of the Chromium |
| 467 // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe), |
| 468 // ensure that there are no public destructors in the class hierarchy. This |
| 469 // is to guard against accidentally stack-allocating a RefCounted class or |
| 470 // sticking it in a non-ref-counted container (like scoped_ptr<>). |
| 471 void CheckRefCountedDtors(SourceLocation record_location, |
| 472 CXXRecordDecl* record) { |
| 473 // Skip anonymous structs. |
| 474 if (record->getIdentifier() == NULL) |
| 475 return; |
| 476 |
| 477 // Determine if the current type is even ref-counted. |
| 478 CXXBasePaths refcounted_path; |
| 479 if (!record->lookupInBases( |
| 480 &FindBadConstructsConsumer::IsRefCountedCallback, this, |
| 481 refcounted_path)) { |
| 482 return; // Class does not derive from a ref-counted base class. |
| 483 } |
| 484 |
| 485 // Easy check: Check to see if the current type is problematic. |
| 486 SourceLocation loc; |
| 487 RefcountIssue issue = CheckRecordForRefcountIssue(record, loc); |
| 488 if (issue == ImplicitDestructor) { |
| 489 diagnostic().Report(loc, diag_no_explicit_dtor_); |
| 490 PrintInheritanceChain(refcounted_path.front()); |
| 491 return; |
| 492 } |
| 493 if (issue == PublicDestructor) { |
| 494 diagnostic().Report(loc, diag_public_dtor_); |
| 495 PrintInheritanceChain(refcounted_path.front()); |
| 496 return; |
| 497 } |
| 498 |
| 499 // Long check: Check all possible base classes for problematic |
| 500 // destructors. This checks for situations involving multiple |
| 501 // inheritance, where the ref-counted class may be implementing an |
| 502 // interface that has a public or implicit destructor. |
| 503 // |
| 504 // struct SomeInterface { |
| 505 // virtual void DoFoo(); |
| 506 // }; |
| 507 // |
| 508 // struct RefCountedInterface |
| 509 // : public base::RefCounted<RefCountedInterface>, |
| 510 // public SomeInterface { |
| 511 // private: |
| 512 // friend class base::Refcounted<RefCountedInterface>; |
| 513 // virtual ~RefCountedInterface() {} |
| 514 // }; |
| 515 // |
| 516 // While RefCountedInterface is "safe", in that its destructor is |
| 517 // private, it's possible to do the following "unsafe" code: |
| 518 // scoped_refptr<RefCountedInterface> some_class( |
| 519 // new RefCountedInterface); |
| 520 // // Calls SomeInterface::~SomeInterface(), which is unsafe. |
| 521 // delete static_cast<SomeInterface*>(some_class.get()); |
| 522 if (!check_base_classes_) |
| 523 return; |
| 524 |
| 525 // Find all public destructors. This will record the class hierarchy |
| 526 // that leads to the public destructor in |dtor_paths|. |
| 527 CXXBasePaths dtor_paths; |
| 528 if (!record->lookupInBases( |
| 529 &FindBadConstructsConsumer::HasPublicDtorCallback, this, |
| 530 dtor_paths)) { |
| 531 return; |
| 532 } |
| 533 |
| 534 for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin(); |
| 535 it != dtor_paths.end(); ++it) { |
| 536 // The record with the problem will always be the last record |
| 537 // in the path, since it is the record that stopped the search. |
| 538 const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>( |
| 539 it->back().Base->getType()->getAs<RecordType>()->getDecl()); |
| 540 |
| 541 issue = CheckRecordForRefcountIssue(problem_record, loc); |
| 542 |
| 543 if (issue == ImplicitDestructor) { |
| 544 diagnostic().Report(record_location, diag_no_explicit_dtor_); |
| 545 PrintInheritanceChain(refcounted_path.front()); |
| 546 diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record; |
| 547 PrintInheritanceChain(*it); |
| 548 } else if (issue == PublicDestructor) { |
| 549 diagnostic().Report(record_location, diag_public_dtor_); |
| 550 PrintInheritanceChain(refcounted_path.front()); |
| 551 diagnostic().Report(loc, diag_note_public_dtor_); |
| 552 PrintInheritanceChain(*it); |
| 553 } |
| 554 } |
| 555 } |
393 }; | 556 }; |
394 | 557 |
395 class FindBadConstructsAction : public PluginASTAction { | 558 class FindBadConstructsAction : public PluginASTAction { |
396 public: | 559 public: |
397 FindBadConstructsAction() | 560 FindBadConstructsAction() |
398 : check_refcounted_dtors_(true), | 561 : check_base_classes_(false), |
399 check_virtuals_in_implementations_(true) { | 562 check_virtuals_in_implementations_(true) { |
400 } | 563 } |
401 | 564 |
402 protected: | 565 protected: |
403 // Overridden from PluginASTAction: | 566 // Overridden from PluginASTAction: |
404 virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance, | 567 virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance, |
405 llvm::StringRef ref) { | 568 llvm::StringRef ref) { |
406 return new FindBadConstructsConsumer( | 569 return new FindBadConstructsConsumer( |
407 instance, check_refcounted_dtors_, check_virtuals_in_implementations_); | 570 instance, check_base_classes_, check_virtuals_in_implementations_); |
408 } | 571 } |
409 | 572 |
410 virtual bool ParseArgs(const CompilerInstance& instance, | 573 virtual bool ParseArgs(const CompilerInstance& instance, |
411 const std::vector<std::string>& args) { | 574 const std::vector<std::string>& args) { |
412 bool parsed = true; | 575 bool parsed = true; |
413 | 576 |
414 for (size_t i = 0; i < args.size() && parsed; ++i) { | 577 for (size_t i = 0; i < args.size() && parsed; ++i) { |
415 if (args[i] == "skip-refcounted-dtors") { | 578 if (args[i] == "skip-virtuals-in-implementations") { |
416 check_refcounted_dtors_ = false; | 579 // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed. |
417 } else if (args[i] == "skip-virtuals-in-implementations") { | |
418 check_virtuals_in_implementations_ = false; | 580 check_virtuals_in_implementations_ = false; |
| 581 } else if (args[i] == "check-base-classes") { |
| 582 // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed. |
| 583 check_base_classes_ = true; |
419 } else { | 584 } else { |
420 parsed = false; | 585 parsed = false; |
421 llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n"; | 586 llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n"; |
422 } | 587 } |
423 } | 588 } |
424 | 589 |
425 return parsed; | 590 return parsed; |
426 } | 591 } |
427 | 592 |
428 private: | 593 private: |
429 bool check_refcounted_dtors_; | 594 bool check_base_classes_; |
430 bool check_virtuals_in_implementations_; | 595 bool check_virtuals_in_implementations_; |
431 }; | 596 }; |
432 | 597 |
433 } // namespace | 598 } // namespace |
434 | 599 |
435 static FrontendPluginRegistry::Add<FindBadConstructsAction> | 600 static FrontendPluginRegistry::Add<FindBadConstructsAction> |
436 X("find-bad-constructs", "Finds bad C++ constructs"); | 601 X("find-bad-constructs", "Finds bad C++ constructs"); |
OLD | NEW |