diff options
Diffstat (limited to 'tools/clang')
| -rw-r--r-- | tools/clang/plugins/FindBadConstructs.cpp | 779 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructsAction.cpp | 55 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructsAction.h | 32 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructsConsumer.cpp | 687 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructsConsumer.h | 100 | ||||
| -rw-r--r-- | tools/clang/plugins/Options.h | 25 |
6 files changed, 899 insertions, 779 deletions
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp deleted file mode 100644 index 4b74fbe..0000000 --- a/tools/clang/plugins/FindBadConstructs.cpp +++ /dev/null @@ -1,779 +0,0 @@ -// 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. -// - WeakPtrFactory members that refer to their outer class should be the last -// member. -// - Enum types with a xxxx_LAST or xxxxLast const actually have that constant -// have the maximal value for that type. - -#include "clang/AST/ASTConsumer.h" -#include "clang/AST/AST.h" -#include "clang/AST/Attr.h" -#include "clang/AST/CXXInheritance.h" -#include "clang/AST/TypeLoc.h" -#include "clang/Basic/SourceManager.h" -#include "clang/Frontend/CompilerInstance.h" -#include "clang/Frontend/FrontendPluginRegistry.h" -#include "clang/Lex/Lexer.h" -#include "llvm/Support/raw_ostream.h" - -#include "ChromeClassTester.h" - -using namespace clang; - -namespace { - -const char kMethodRequiresOverride[] = - "[chromium-style] Overriding method must be marked with OVERRIDE."; -const char kMethodRequiresVirtual[] = - "[chromium-style] Overriding method must have \"virtual\" keyword."; -const char kNoExplicitDtor[] = - "[chromium-style] Classes that are ref-counted should have explicit " - "destructors that are declared protected or private."; -const char kPublicDtor[] = - "[chromium-style] Classes that are ref-counted should have " - "destructors that are declared protected or private."; -const char kProtectedNonVirtualDtor[] = - "[chromium-style] Classes that are ref-counted and have non-private " - "destructors should declare their destructor virtual."; -const char kWeakPtrFactoryOrder[] = - "[chromium-style] WeakPtrFactory members which refer to their outer class " - "must be the last member in the outer class definition."; -const char kBadLastEnumValue[] = - "[chromium-style] _LAST/Last constants of enum types must have the maximal " - "value for any constant of that type."; -const char kNoteInheritance[] = - "[chromium-style] %0 inherits from %1 here"; -const char kNoteImplicitDtor[] = - "[chromium-style] No explicit destructor for %0 defined"; -const char kNotePublicDtor[] = - "[chromium-style] Public destructor declared here"; -const char kNoteProtectedNonVirtualDtor[] = - "[chromium-style] Protected non-virtual destructor declared here"; - -bool TypeHasNonTrivialDtor(const Type* type) { - if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) - return !cxx_r->hasTrivialDestructor(); - - return false; -} - -// Returns the underlying Type for |type| by expanding typedefs and removing -// any namespace qualifiers. This is similar to desugaring, except that for -// ElaboratedTypes, desugar will unwrap too much. -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; -} - -struct FindBadConstructsOptions { - FindBadConstructsOptions() : check_base_classes(false), - check_virtuals_in_implementations(true), - check_weak_ptr_factory_order(false), - check_enum_last_value(false) { - } - bool check_base_classes; - bool check_virtuals_in_implementations; - bool check_weak_ptr_factory_order; - bool check_enum_last_value; -}; - -// Searches for constructs that we know we don't want in the Chromium code base. -class FindBadConstructsConsumer : public ChromeClassTester { - public: - FindBadConstructsConsumer(CompilerInstance& instance, - const FindBadConstructsOptions& options) - : ChromeClassTester(instance), - options_(options) { - // Register warning/error messages. - diag_method_requires_override_ = diagnostic().getCustomDiagID( - getErrorLevel(), kMethodRequiresOverride); - diag_method_requires_virtual_ = diagnostic().getCustomDiagID( - getErrorLevel(), kMethodRequiresVirtual); - diag_no_explicit_dtor_ = diagnostic().getCustomDiagID( - getErrorLevel(), kNoExplicitDtor); - diag_public_dtor_ = diagnostic().getCustomDiagID( - getErrorLevel(), kPublicDtor); - diag_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( - getErrorLevel(), kProtectedNonVirtualDtor); - diag_weak_ptr_factory_order_ = diagnostic().getCustomDiagID( - getErrorLevel(), kWeakPtrFactoryOrder); - diag_bad_enum_last_value_ = diagnostic().getCustomDiagID( - getErrorLevel(), kBadLastEnumValue); - - // Registers notes to make it easier to interpret warnings. - diag_note_inheritance_ = diagnostic().getCustomDiagID( - DiagnosticsEngine::Note, kNoteInheritance); - diag_note_implicit_dtor_ = diagnostic().getCustomDiagID( - DiagnosticsEngine::Note, kNoteImplicitDtor); - diag_note_public_dtor_ = diagnostic().getCustomDiagID( - DiagnosticsEngine::Note, kNotePublicDtor); - diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( - DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor); - } - - 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 || options_.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); - } - - CheckRefCountedDtors(record_location, record); - - if (options_.check_weak_ptr_factory_order) - CheckWeakPtrFactoryMembers(record_location, record); - } - - virtual void CheckChromeEnum(SourceLocation enum_location, - EnumDecl* enum_decl) { - if (!options_.check_enum_last_value) - return; - - bool got_one = false; - bool is_signed = false; - llvm::APSInt max_so_far; - EnumDecl::enumerator_iterator iter; - for (iter = enum_decl->enumerator_begin(); - iter != enum_decl->enumerator_end(); ++iter) { - llvm::APSInt current_value = iter->getInitVal(); - if (!got_one) { - max_so_far = current_value; - is_signed = current_value.isSigned(); - got_one = true; - } else { - if (is_signed != current_value.isSigned()) { - // This only happens in some cases when compiling C (not C++) files, - // so it is OK to bail out here. - return; - } - if (current_value > max_so_far) - max_so_far = current_value; - } - } - for (iter = enum_decl->enumerator_begin(); - iter != enum_decl->enumerator_end(); ++iter) { - std::string name = iter->getNameAsString(); - if (((name.size() > 4 && - name.compare(name.size() - 4, 4, "Last") == 0) || - (name.size() > 5 && - name.compare(name.size() - 5, 5, "_LAST") == 0)) && - iter->getInitVal() < max_so_far) { - diagnostic().Report(iter->getLocation(), diag_bad_enum_last_value_); - } - } - } - - private: - // The type of problematic ref-counting pattern that was encountered. - enum RefcountIssue { - None, - ImplicitDestructor, - PublicDestructor - }; - - FindBadConstructsOptions options_; - - unsigned diag_method_requires_override_; - unsigned diag_method_requires_virtual_; - unsigned diag_no_explicit_dtor_; - unsigned diag_public_dtor_; - unsigned diag_protected_non_virtual_dtor_; - unsigned diag_weak_ptr_factory_order_; - unsigned diag_bad_enum_last_value_; - unsigned diag_note_inheritance_; - unsigned diag_note_implicit_dtor_; - unsigned diag_note_public_dtor_; - unsigned diag_note_protected_non_virtual_dtor_; - - // 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(); - SourceManager& manager = instance().getSourceManager(); - FullSourceLoc full_loc(loc, manager); - SourceLocation spelling_loc = manager.getSpellingLoc(loc); - diagnostic().Report(full_loc, diag_method_requires_virtual_) - << FixItHint::CreateInsertion(spelling_loc, "virtual "); - } - - // 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 IsMethodInBannedOrTestingNamespace(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 (IsMethodInBannedOrTestingNamespace(overridden) || - InTestingNamespace(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 (IsMethodInBannedOrTestingNamespace(method)) - return; - - SourceManager& manager = instance().getSourceManager(); - SourceRange type_info_range = - method->getTypeSourceInfo()->getTypeLoc().getSourceRange(); - FullSourceLoc loc(type_info_range.getBegin(), manager); - - // Build the FixIt insertion point after the end of the method definition, - // including any const-qualifiers and attributes, and before the opening - // of the l-curly-brace (if inline) or the semi-color (if a declaration). - SourceLocation spelling_end = - manager.getSpellingLoc(type_info_range.getEnd()); - if (spelling_end.isValid()) { - SourceLocation token_end = Lexer::getLocForEndOfToken( - spelling_end, 0, manager, LangOptions()); - diagnostic().Report(token_end, diag_method_requires_override_) - << FixItHint::CreateInsertion(token_end, " OVERRIDE"); - } else { - diagnostic().Report(loc, diag_method_requires_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; - } - } - } - - // Check |record| for issues that are problematic for ref-counted types. - // Note that |record| may not be a ref-counted type, but a base class for - // a type that is. - // If there are issues, update |loc| with the SourceLocation of the issue - // and returns appropriately, or returns None if there are no issues. - static RefcountIssue CheckRecordForRefcountIssue( - const CXXRecordDecl* record, - SourceLocation &loc) { - if (!record->hasUserDeclaredDestructor()) { - loc = record->getLocation(); - return ImplicitDestructor; - } - - if (CXXDestructorDecl* dtor = record->getDestructor()) { - if (dtor->getAccess() == AS_public) { - loc = dtor->getInnerLocStart(); - return PublicDestructor; - } - } - - return None; - } - - // Adds either a warning or error, based on the current handling of - // -Werror. - DiagnosticsEngine::Level getErrorLevel() { - return diagnostic().getWarningsAsErrors() ? - DiagnosticsEngine::Error : DiagnosticsEngine::Warning; - } - - // Returns true if |base| specifies one of the Chromium reference counted - // classes (base::RefCounted / base::RefCountedThreadSafe). - 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; - } - - // Returns true if |base| specifies a class that has a public destructor, - // either explicitly or implicitly. - static bool HasPublicDtorCallback(const CXXBaseSpecifier* base, - CXXBasePath& path, - void* user_data) { - // Only examine paths that have public inheritance, as they are the - // only ones which will result in the destructor potentially being - // exposed. This check is largely redundant, as Chromium code should be - // exclusively using public inheritance. - if (path.Access != AS_public) - return false; - - CXXRecordDecl* record = dyn_cast<CXXRecordDecl>( - base->getType()->getAs<RecordType>()->getDecl()); - SourceLocation unused; - return None != CheckRecordForRefcountIssue(record, unused); - } - - // Outputs a C++ inheritance chain as a diagnostic aid. - void PrintInheritanceChain(const CXXBasePath& path) { - for (CXXBasePath::const_iterator it = path.begin(); it != path.end(); - ++it) { - diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_) - << it->Class << it->Base->getType(); - } - } - - unsigned DiagnosticForIssue(RefcountIssue issue) { - switch (issue) { - case ImplicitDestructor: - return diag_no_explicit_dtor_; - case PublicDestructor: - return diag_public_dtor_; - case None: - assert(false && "Do not call DiagnosticForIssue with issue None"); - return 0; - } - assert(false); - return 0; - } - - // Check |record| to determine if it has any problematic refcounting - // issues and, if so, print them as warnings/errors based on the current - // value of getErrorLevel(). - // - // If |record| is a C++ class, and if it inherits from one of the Chromium - // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe), - // ensure that there are no public destructors in the class hierarchy. This - // is to guard against accidentally stack-allocating a RefCounted class or - // sticking it in a non-ref-counted container (like scoped_ptr<>). - void CheckRefCountedDtors(SourceLocation record_location, - CXXRecordDecl* record) { - // Skip anonymous structs. - if (record->getIdentifier() == NULL) - return; - - // Determine if the current type is even ref-counted. - CXXBasePaths refcounted_path; - if (!record->lookupInBases( - &FindBadConstructsConsumer::IsRefCountedCallback, this, - refcounted_path)) { - return; // Class does not derive from a ref-counted base class. - } - - // Easy check: Check to see if the current type is problematic. - SourceLocation loc; - RefcountIssue issue = CheckRecordForRefcountIssue(record, loc); - if (issue != None) { - diagnostic().Report(loc, DiagnosticForIssue(issue)); - PrintInheritanceChain(refcounted_path.front()); - return; - } - if (CXXDestructorDecl* dtor = - refcounted_path.begin()->back().Class->getDestructor()) { - if (dtor->getAccess() == AS_protected && - !dtor->isVirtual()) { - loc = dtor->getInnerLocStart(); - diagnostic().Report(loc, diag_protected_non_virtual_dtor_); - return; - } - } - - // Long check: Check all possible base classes for problematic - // destructors. This checks for situations involving multiple - // inheritance, where the ref-counted class may be implementing an - // interface that has a public or implicit destructor. - // - // struct SomeInterface { - // virtual void DoFoo(); - // }; - // - // struct RefCountedInterface - // : public base::RefCounted<RefCountedInterface>, - // public SomeInterface { - // private: - // friend class base::Refcounted<RefCountedInterface>; - // virtual ~RefCountedInterface() {} - // }; - // - // While RefCountedInterface is "safe", in that its destructor is - // private, it's possible to do the following "unsafe" code: - // scoped_refptr<RefCountedInterface> some_class( - // new RefCountedInterface); - // // Calls SomeInterface::~SomeInterface(), which is unsafe. - // delete static_cast<SomeInterface*>(some_class.get()); - if (!options_.check_base_classes) - return; - - // Find all public destructors. This will record the class hierarchy - // that leads to the public destructor in |dtor_paths|. - CXXBasePaths dtor_paths; - if (!record->lookupInBases( - &FindBadConstructsConsumer::HasPublicDtorCallback, this, - dtor_paths)) { - return; - } - - for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin(); - it != dtor_paths.end(); ++it) { - // The record with the problem will always be the last record - // in the path, since it is the record that stopped the search. - const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>( - it->back().Base->getType()->getAs<RecordType>()->getDecl()); - - issue = CheckRecordForRefcountIssue(problem_record, loc); - - if (issue == ImplicitDestructor) { - diagnostic().Report(record_location, diag_no_explicit_dtor_); - PrintInheritanceChain(refcounted_path.front()); - diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record; - PrintInheritanceChain(*it); - } else if (issue == PublicDestructor) { - diagnostic().Report(record_location, diag_public_dtor_); - PrintInheritanceChain(refcounted_path.front()); - diagnostic().Report(loc, diag_note_public_dtor_); - PrintInheritanceChain(*it); - } - } - } - - // Check for any problems with WeakPtrFactory class members. This currently - // only checks that any WeakPtrFactory<T> member of T appears as the last - // data member in T. We could consider checking for bad uses of - // WeakPtrFactory to refer to other data members, but that would require - // looking at the initializer list in constructors to see what the factory - // points to. - // Note, if we later add other unrelated checks of data members, we should - // consider collapsing them in to one loop to avoid iterating over the data - // members more than once. - void CheckWeakPtrFactoryMembers(SourceLocation record_location, - CXXRecordDecl* record) { - // Skip anonymous structs. - if (record->getIdentifier() == NULL) - return; - - // Iterate through members of the class. - RecordDecl::field_iterator iter(record->field_begin()), - the_end(record->field_end()); - SourceLocation weak_ptr_factory_location; // Invalid initially. - for (; iter != the_end; ++iter) { - // If we enter the loop but have already seen a matching WeakPtrFactory, - // it means there is at least one member after the factory. - if (weak_ptr_factory_location.isValid()) { - diagnostic().Report(weak_ptr_factory_location, - diag_weak_ptr_factory_order_); - } - const TemplateSpecializationType* template_spec_type = - iter->getType().getTypePtr()->getAs<TemplateSpecializationType>(); - if (template_spec_type) { - const TemplateDecl* template_decl = - template_spec_type->getTemplateName().getAsTemplateDecl(); - if (template_decl && template_spec_type->getNumArgs() >= 1) { - if (template_decl->getNameAsString().compare("WeakPtrFactory") == 0 && - GetNamespace(template_decl) == "base") { - const TemplateArgument& arg = template_spec_type->getArg(0); - if (arg.getAsType().getTypePtr()->getAsCXXRecordDecl() == - record->getTypeForDecl()->getAsCXXRecordDecl()) { - weak_ptr_factory_location = iter->getLocation(); - } - } - } - } - } - } -}; - - -class FindBadConstructsAction : public PluginASTAction { - public: - FindBadConstructsAction() { - } - - protected: - // Overridden from PluginASTAction: - virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance, - llvm::StringRef ref) { - return new FindBadConstructsConsumer(instance, options_); - } - - 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-virtuals-in-implementations") { - // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed. - options_.check_virtuals_in_implementations = false; - } else if (args[i] == "check-base-classes") { - // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed. - options_.check_base_classes = true; - } else if (args[i] == "check-weak-ptr-factory-order") { - // TODO(dmichael): Remove this once http://crbug.com/303818 is fixed. - options_.check_weak_ptr_factory_order = true; - } else if (args[i] == "check-enum-last-value") { - // TODO(tsepez): Enable this by default once http://crbug.com/356815 - // and http://crbug.com/356816 are fixed. - options_.check_enum_last_value = true; - } else { - parsed = false; - llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n"; - } - } - - return parsed; - } - - private: - FindBadConstructsOptions options_; -}; - -} // namespace - -static FrontendPluginRegistry::Add<FindBadConstructsAction> -X("find-bad-constructs", "Finds bad C++ constructs"); diff --git a/tools/clang/plugins/FindBadConstructsAction.cpp b/tools/clang/plugins/FindBadConstructsAction.cpp new file mode 100644 index 0000000..838590d --- /dev/null +++ b/tools/clang/plugins/FindBadConstructsAction.cpp @@ -0,0 +1,55 @@ +// 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. + +#include "FindBadConstructsAction.h" + +#include "clang/Frontend/FrontendPluginRegistry.h" + +#include "FindBadConstructsConsumer.h" + +using namespace clang; + +namespace chrome_checker { + +FindBadConstructsAction::FindBadConstructsAction() { +} + +ASTConsumer* FindBadConstructsAction::CreateASTConsumer( + CompilerInstance& instance, + llvm::StringRef ref) { + return new FindBadConstructsConsumer(instance, options_); +} + +bool FindBadConstructsAction::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-virtuals-in-implementations") { + // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed. + options_.check_virtuals_in_implementations = false; + } else if (args[i] == "check-base-classes") { + // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed. + options_.check_base_classes = true; + } else if (args[i] == "check-weak-ptr-factory-order") { + // TODO(dmichael): Remove this once http://crbug.com/303818 is fixed. + options_.check_weak_ptr_factory_order = true; + } else if (args[i] == "check-enum-last-value") { + // TODO(tsepez): Enable this by default once http://crbug.com/356815 + // and http://crbug.com/356816 are fixed. + options_.check_enum_last_value = true; + } else { + parsed = false; + llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n"; + } + } + + return parsed; +} + +} // namespace chrome_checker + +static FrontendPluginRegistry::Add<chrome_checker::FindBadConstructsAction> X( + "find-bad-constructs", + "Finds bad C++ constructs"); diff --git a/tools/clang/plugins/FindBadConstructsAction.h b/tools/clang/plugins/FindBadConstructsAction.h new file mode 100644 index 0000000..887d339 --- /dev/null +++ b/tools/clang/plugins/FindBadConstructsAction.h @@ -0,0 +1,32 @@ +// 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. + +#ifndef TOOLS_CLANG_PLUGINS_FINDBADCONSTRUCTIONS_ACTION_H_ +#define TOOLS_CLANG_PLUGINS_FINDBADCONSTRUCTIONS_ACTION_H_ + +#include "clang/Frontend/FrontendAction.h" + +#include "Options.h" + +namespace chrome_checker { + +class FindBadConstructsAction : public clang::PluginASTAction { + public: + FindBadConstructsAction(); + + protected: + // Overridden from PluginASTAction: + virtual clang::ASTConsumer* CreateASTConsumer( + clang::CompilerInstance& instance, + llvm::StringRef ref); + virtual bool ParseArgs(const clang::CompilerInstance& instance, + const std::vector<std::string>& args); + + private: + Options options_; +}; + +} // namespace chrome_checker + +#endif // TOOLS_CLANG_PLUGINS_FINDBADCONSTRUCTIONS_ACTION_H_ diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp new file mode 100644 index 0000000..c4219a1 --- /dev/null +++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp @@ -0,0 +1,687 @@ +// 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. + +#include "FindBadConstructsConsumer.h" + +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/raw_ostream.h" + +using namespace clang; + +namespace chrome_checker { + +namespace { + +const char kMethodRequiresOverride[] = + "[chromium-style] Overriding method must be marked with OVERRIDE."; +const char kMethodRequiresVirtual[] = + "[chromium-style] Overriding method must have \"virtual\" keyword."; +const char kNoExplicitDtor[] = + "[chromium-style] Classes that are ref-counted should have explicit " + "destructors that are declared protected or private."; +const char kPublicDtor[] = + "[chromium-style] Classes that are ref-counted should have " + "destructors that are declared protected or private."; +const char kProtectedNonVirtualDtor[] = + "[chromium-style] Classes that are ref-counted and have non-private " + "destructors should declare their destructor virtual."; +const char kWeakPtrFactoryOrder[] = + "[chromium-style] WeakPtrFactory members which refer to their outer class " + "must be the last member in the outer class definition."; +const char kBadLastEnumValue[] = + "[chromium-style] _LAST/Last constants of enum types must have the maximal " + "value for any constant of that type."; +const char kNoteInheritance[] = "[chromium-style] %0 inherits from %1 here"; +const char kNoteImplicitDtor[] = + "[chromium-style] No explicit destructor for %0 defined"; +const char kNotePublicDtor[] = + "[chromium-style] Public destructor declared here"; +const char kNoteProtectedNonVirtualDtor[] = + "[chromium-style] Protected non-virtual destructor declared here"; + +bool TypeHasNonTrivialDtor(const Type* type) { + if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl()) + return !cxx_r->hasTrivialDestructor(); + + return false; +} + +// Returns the underlying Type for |type| by expanding typedefs and removing +// any namespace qualifiers. This is similar to desugaring, except that for +// ElaboratedTypes, desugar will unwrap too much. +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; +} + +} // namespace + +FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance, + const Options& options) + : ChromeClassTester(instance), options_(options) { + // Register warning/error messages. + diag_method_requires_override_ = + diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride); + diag_method_requires_virtual_ = + diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresVirtual); + diag_no_explicit_dtor_ = + diagnostic().getCustomDiagID(getErrorLevel(), kNoExplicitDtor); + diag_public_dtor_ = + diagnostic().getCustomDiagID(getErrorLevel(), kPublicDtor); + diag_protected_non_virtual_dtor_ = + diagnostic().getCustomDiagID(getErrorLevel(), kProtectedNonVirtualDtor); + diag_weak_ptr_factory_order_ = + diagnostic().getCustomDiagID(getErrorLevel(), kWeakPtrFactoryOrder); + diag_bad_enum_last_value_ = + diagnostic().getCustomDiagID(getErrorLevel(), kBadLastEnumValue); + + // Registers notes to make it easier to interpret warnings. + diag_note_inheritance_ = + diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteInheritance); + diag_note_implicit_dtor_ = + diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteImplicitDtor); + diag_note_public_dtor_ = + diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNotePublicDtor); + diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID( + DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor); +} + +void FindBadConstructsConsumer::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 || options_.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); + } + + CheckRefCountedDtors(record_location, record); + + if (options_.check_weak_ptr_factory_order) + CheckWeakPtrFactoryMembers(record_location, record); +} + +void FindBadConstructsConsumer::CheckChromeEnum(SourceLocation enum_location, + EnumDecl* enum_decl) { + if (!options_.check_enum_last_value) + return; + + bool got_one = false; + bool is_signed = false; + llvm::APSInt max_so_far; + EnumDecl::enumerator_iterator iter; + for (iter = enum_decl->enumerator_begin(); + iter != enum_decl->enumerator_end(); + ++iter) { + llvm::APSInt current_value = iter->getInitVal(); + if (!got_one) { + max_so_far = current_value; + is_signed = current_value.isSigned(); + got_one = true; + } else { + if (is_signed != current_value.isSigned()) { + // This only happens in some cases when compiling C (not C++) files, + // so it is OK to bail out here. + return; + } + if (current_value > max_so_far) + max_so_far = current_value; + } + } + for (iter = enum_decl->enumerator_begin(); + iter != enum_decl->enumerator_end(); + ++iter) { + std::string name = iter->getNameAsString(); + if (((name.size() > 4 && name.compare(name.size() - 4, 4, "Last") == 0) || + (name.size() > 5 && name.compare(name.size() - 5, 5, "_LAST") == 0)) && + iter->getInitVal() < max_so_far) { + diagnostic().Report(iter->getLocation(), diag_bad_enum_last_value_); + } + } +} + +void FindBadConstructsConsumer::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 FindBadConstructsConsumer::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(); + SourceManager& manager = instance().getSourceManager(); + FullSourceLoc full_loc(loc, manager); + SourceLocation spelling_loc = manager.getSpellingLoc(loc); + diagnostic().Report(full_loc, diag_method_requires_virtual_) + << FixItHint::CreateInsertion(spelling_loc, "virtual "); + } + + // 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 FindBadConstructsConsumer::InTestingNamespace(const Decl* record) { + return GetNamespace(record).find("testing") != std::string::npos; +} + +bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace( + 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 (IsMethodInBannedOrTestingNamespace(overridden) || + InTestingNamespace(overridden)) { + return true; + } + } + + return false; +} + +void FindBadConstructsConsumer::CheckOverriddenMethod( + const CXXMethodDecl* method) { + if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>()) + return; + + if (isa<CXXDestructorDecl>(method) || method->isPure()) + return; + + if (IsMethodInBannedOrTestingNamespace(method)) + return; + + SourceManager& manager = instance().getSourceManager(); + SourceRange type_info_range = + method->getTypeSourceInfo()->getTypeLoc().getSourceRange(); + FullSourceLoc loc(type_info_range.getBegin(), manager); + + // Build the FixIt insertion point after the end of the method definition, + // including any const-qualifiers and attributes, and before the opening + // of the l-curly-brace (if inline) or the semi-color (if a declaration). + SourceLocation spelling_end = + manager.getSpellingLoc(type_info_range.getEnd()); + if (spelling_end.isValid()) { + SourceLocation token_end = + Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions()); + diagnostic().Report(token_end, diag_method_requires_override_) + << FixItHint::CreateInsertion(token_end, " OVERRIDE"); + } else { + diagnostic().Report(loc, diag_method_requires_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 FindBadConstructsConsumer::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 FindBadConstructsConsumer::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; + } + } +} + +// Check |record| for issues that are problematic for ref-counted types. +// Note that |record| may not be a ref-counted type, but a base class for +// a type that is. +// If there are issues, update |loc| with the SourceLocation of the issue +// and returns appropriately, or returns None if there are no issues. +FindBadConstructsConsumer::RefcountIssue +FindBadConstructsConsumer::CheckRecordForRefcountIssue( + const CXXRecordDecl* record, + SourceLocation& loc) { + if (!record->hasUserDeclaredDestructor()) { + loc = record->getLocation(); + return ImplicitDestructor; + } + + if (CXXDestructorDecl* dtor = record->getDestructor()) { + if (dtor->getAccess() == AS_public) { + loc = dtor->getInnerLocStart(); + return PublicDestructor; + } + } + + return None; +} + +// Adds either a warning or error, based on the current handling of +// -Werror. +DiagnosticsEngine::Level FindBadConstructsConsumer::getErrorLevel() { + return diagnostic().getWarningsAsErrors() ? DiagnosticsEngine::Error + : DiagnosticsEngine::Warning; +} + +// Returns true if |base| specifies one of the Chromium reference counted +// classes (base::RefCounted / base::RefCountedThreadSafe). +bool FindBadConstructsConsumer::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; +} + +// Returns true if |base| specifies a class that has a public destructor, +// either explicitly or implicitly. +bool FindBadConstructsConsumer::HasPublicDtorCallback( + const CXXBaseSpecifier* base, + CXXBasePath& path, + void* user_data) { + // Only examine paths that have public inheritance, as they are the + // only ones which will result in the destructor potentially being + // exposed. This check is largely redundant, as Chromium code should be + // exclusively using public inheritance. + if (path.Access != AS_public) + return false; + + CXXRecordDecl* record = + dyn_cast<CXXRecordDecl>(base->getType()->getAs<RecordType>()->getDecl()); + SourceLocation unused; + return None != CheckRecordForRefcountIssue(record, unused); +} + +// Outputs a C++ inheritance chain as a diagnostic aid. +void FindBadConstructsConsumer::PrintInheritanceChain(const CXXBasePath& path) { + for (CXXBasePath::const_iterator it = path.begin(); it != path.end(); ++it) { + diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_) + << it->Class << it->Base->getType(); + } +} + +unsigned FindBadConstructsConsumer::DiagnosticForIssue(RefcountIssue issue) { + switch (issue) { + case ImplicitDestructor: + return diag_no_explicit_dtor_; + case PublicDestructor: + return diag_public_dtor_; + case None: + assert(false && "Do not call DiagnosticForIssue with issue None"); + return 0; + } + assert(false); + return 0; +} + +// Check |record| to determine if it has any problematic refcounting +// issues and, if so, print them as warnings/errors based on the current +// value of getErrorLevel(). +// +// If |record| is a C++ class, and if it inherits from one of the Chromium +// ref-counting classes (base::RefCounted / base::RefCountedThreadSafe), +// ensure that there are no public destructors in the class hierarchy. This +// is to guard against accidentally stack-allocating a RefCounted class or +// sticking it in a non-ref-counted container (like scoped_ptr<>). +void FindBadConstructsConsumer::CheckRefCountedDtors( + SourceLocation record_location, + CXXRecordDecl* record) { + // Skip anonymous structs. + if (record->getIdentifier() == NULL) + return; + + // Determine if the current type is even ref-counted. + CXXBasePaths refcounted_path; + if (!record->lookupInBases(&FindBadConstructsConsumer::IsRefCountedCallback, + this, + refcounted_path)) { + return; // Class does not derive from a ref-counted base class. + } + + // Easy check: Check to see if the current type is problematic. + SourceLocation loc; + RefcountIssue issue = CheckRecordForRefcountIssue(record, loc); + if (issue != None) { + diagnostic().Report(loc, DiagnosticForIssue(issue)); + PrintInheritanceChain(refcounted_path.front()); + return; + } + if (CXXDestructorDecl* dtor = + refcounted_path.begin()->back().Class->getDestructor()) { + if (dtor->getAccess() == AS_protected && !dtor->isVirtual()) { + loc = dtor->getInnerLocStart(); + diagnostic().Report(loc, diag_protected_non_virtual_dtor_); + return; + } + } + + // Long check: Check all possible base classes for problematic + // destructors. This checks for situations involving multiple + // inheritance, where the ref-counted class may be implementing an + // interface that has a public or implicit destructor. + // + // struct SomeInterface { + // virtual void DoFoo(); + // }; + // + // struct RefCountedInterface + // : public base::RefCounted<RefCountedInterface>, + // public SomeInterface { + // private: + // friend class base::Refcounted<RefCountedInterface>; + // virtual ~RefCountedInterface() {} + // }; + // + // While RefCountedInterface is "safe", in that its destructor is + // private, it's possible to do the following "unsafe" code: + // scoped_refptr<RefCountedInterface> some_class( + // new RefCountedInterface); + // // Calls SomeInterface::~SomeInterface(), which is unsafe. + // delete static_cast<SomeInterface*>(some_class.get()); + if (!options_.check_base_classes) + return; + + // Find all public destructors. This will record the class hierarchy + // that leads to the public destructor in |dtor_paths|. + CXXBasePaths dtor_paths; + if (!record->lookupInBases(&FindBadConstructsConsumer::HasPublicDtorCallback, + this, + dtor_paths)) { + return; + } + + for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin(); + it != dtor_paths.end(); + ++it) { + // The record with the problem will always be the last record + // in the path, since it is the record that stopped the search. + const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>( + it->back().Base->getType()->getAs<RecordType>()->getDecl()); + + issue = CheckRecordForRefcountIssue(problem_record, loc); + + if (issue == ImplicitDestructor) { + diagnostic().Report(record_location, diag_no_explicit_dtor_); + PrintInheritanceChain(refcounted_path.front()); + diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record; + PrintInheritanceChain(*it); + } else if (issue == PublicDestructor) { + diagnostic().Report(record_location, diag_public_dtor_); + PrintInheritanceChain(refcounted_path.front()); + diagnostic().Report(loc, diag_note_public_dtor_); + PrintInheritanceChain(*it); + } + } +} + +// Check for any problems with WeakPtrFactory class members. This currently +// only checks that any WeakPtrFactory<T> member of T appears as the last +// data member in T. We could consider checking for bad uses of +// WeakPtrFactory to refer to other data members, but that would require +// looking at the initializer list in constructors to see what the factory +// points to. +// Note, if we later add other unrelated checks of data members, we should +// consider collapsing them in to one loop to avoid iterating over the data +// members more than once. +void FindBadConstructsConsumer::CheckWeakPtrFactoryMembers( + SourceLocation record_location, + CXXRecordDecl* record) { + // Skip anonymous structs. + if (record->getIdentifier() == NULL) + return; + + // Iterate through members of the class. + RecordDecl::field_iterator iter(record->field_begin()), + the_end(record->field_end()); + SourceLocation weak_ptr_factory_location; // Invalid initially. + for (; iter != the_end; ++iter) { + // If we enter the loop but have already seen a matching WeakPtrFactory, + // it means there is at least one member after the factory. + if (weak_ptr_factory_location.isValid()) { + diagnostic().Report(weak_ptr_factory_location, + diag_weak_ptr_factory_order_); + } + const TemplateSpecializationType* template_spec_type = + iter->getType().getTypePtr()->getAs<TemplateSpecializationType>(); + if (template_spec_type) { + const TemplateDecl* template_decl = + template_spec_type->getTemplateName().getAsTemplateDecl(); + if (template_decl && template_spec_type->getNumArgs() >= 1) { + if (template_decl->getNameAsString().compare("WeakPtrFactory") == 0 && + GetNamespace(template_decl) == "base") { + const TemplateArgument& arg = template_spec_type->getArg(0); + if (arg.getAsType().getTypePtr()->getAsCXXRecordDecl() == + record->getTypeForDecl()->getAsCXXRecordDecl()) { + weak_ptr_factory_location = iter->getLocation(); + } + } + } + } + } +} + +} // namespace chrome_checker diff --git a/tools/clang/plugins/FindBadConstructsConsumer.h b/tools/clang/plugins/FindBadConstructsConsumer.h new file mode 100644 index 0000000..4e51193 --- /dev/null +++ b/tools/clang/plugins/FindBadConstructsConsumer.h @@ -0,0 +1,100 @@ +// 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. +// - WeakPtrFactory members that refer to their outer class should be the last +// member. +// - Enum types with a xxxx_LAST or xxxxLast const actually have that constant +// have the maximal value for that type. + +#include "clang/AST/AST.h" +#include "clang/AST/ASTConsumer.h" +#include "clang/AST/Attr.h" +#include "clang/AST/CXXInheritance.h" +#include "clang/AST/TypeLoc.h" +#include "clang/Basic/SourceManager.h" + +#include "ChromeClassTester.h" +#include "Options.h" + +namespace chrome_checker { + +// Searches for constructs that we know we don't want in the Chromium code base. +class FindBadConstructsConsumer : public ChromeClassTester { + public: + FindBadConstructsConsumer(clang::CompilerInstance& instance, + const Options& options); + + // ChromeClassTester overrides: + virtual void CheckChromeClass(clang::SourceLocation record_location, + clang::CXXRecordDecl* record); + virtual void CheckChromeEnum(clang::SourceLocation enum_location, + clang::EnumDecl* enum_decl); + + private: + // The type of problematic ref-counting pattern that was encountered. + enum RefcountIssue { None, ImplicitDestructor, PublicDestructor }; + + void CheckCtorDtorWeight(clang::SourceLocation record_location, + clang::CXXRecordDecl* record); + + void CheckVirtualMethod(const clang::CXXMethodDecl* method, + bool warn_on_inline_bodies); + + bool InTestingNamespace(const clang::Decl* record); + bool IsMethodInBannedOrTestingNamespace(const clang::CXXMethodDecl* method); + + void CheckOverriddenMethod(const clang::CXXMethodDecl* method); + void CheckVirtualMethods(clang::SourceLocation record_location, + clang::CXXRecordDecl* record, + bool warn_on_inline_bodies); + + void CountType(const clang::Type* type, + int* trivial_member, + int* non_trivial_member, + int* templated_non_trivial_member); + + static RefcountIssue CheckRecordForRefcountIssue( + const clang::CXXRecordDecl* record, + clang::SourceLocation& loc); + clang::DiagnosticsEngine::Level getErrorLevel(); + static bool IsRefCountedCallback(const clang::CXXBaseSpecifier* base, + clang::CXXBasePath& path, + void* user_data); + static bool HasPublicDtorCallback(const clang::CXXBaseSpecifier* base, + clang::CXXBasePath& path, + void* user_data); + void PrintInheritanceChain(const clang::CXXBasePath& path); + unsigned DiagnosticForIssue(RefcountIssue issue); + void CheckRefCountedDtors(clang::SourceLocation record_location, + clang::CXXRecordDecl* record); + + void CheckWeakPtrFactoryMembers(clang::SourceLocation record_location, + clang::CXXRecordDecl* record); + + const Options options_; + + unsigned diag_method_requires_override_; + unsigned diag_method_requires_virtual_; + unsigned diag_no_explicit_dtor_; + unsigned diag_public_dtor_; + unsigned diag_protected_non_virtual_dtor_; + unsigned diag_weak_ptr_factory_order_; + unsigned diag_bad_enum_last_value_; + unsigned diag_note_inheritance_; + unsigned diag_note_implicit_dtor_; + unsigned diag_note_public_dtor_; + unsigned diag_note_protected_non_virtual_dtor_; +}; + +} // namespace chrome_checker diff --git a/tools/clang/plugins/Options.h b/tools/clang/plugins/Options.h new file mode 100644 index 0000000..bb50124 --- /dev/null +++ b/tools/clang/plugins/Options.h @@ -0,0 +1,25 @@ +// 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. + +#ifndef TOOLS_CLANG_PLUGINS_OPTIONS_H_ +#define TOOLS_CLANG_PLUGINS_OPTIONS_H_ + +namespace chrome_checker { + +struct Options { + Options() + : check_base_classes(false), + check_virtuals_in_implementations(true), + check_weak_ptr_factory_order(false), + check_enum_last_value(false) {} + + bool check_base_classes; + bool check_virtuals_in_implementations; + bool check_weak_ptr_factory_order; + bool check_enum_last_value; +}; + +} // namespace chrome_checker + +#endif // TOOLS_CLANG_PLUGINS_OPTIONS_H_ |