diff options
| -rw-r--r-- | build/common.gypi | 2 | ||||
| -rw-r--r-- | tools/clang/plugins/ChromeClassTester.cpp | 208 | ||||
| -rw-r--r-- | tools/clang/plugins/ChromeClassTester.h | 63 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructs.cpp | 245 | ||||
| -rw-r--r-- | tools/clang/plugins/Makefile | 20 | ||||
| -rw-r--r-- | tools/clang/plugins/README.chromium | 4 | ||||
| -rw-r--r-- | tools/clang_check/README.chromium | 9 | ||||
| -rwxr-xr-x | tools/clang_check/cxx_wrapper.sh | 20 | ||||
| -rwxr-xr-x | tools/clang_check/link_wrapper.sh | 13 | ||||
| -rwxr-xr-x | tools/clang_check/run_plugin.sh | 13 |
10 files changed, 541 insertions, 56 deletions
diff --git a/build/common.gypi b/build/common.gypi index 280676b..50c12f5 100644 --- a/build/common.gypi +++ b/build/common.gypi @@ -278,7 +278,7 @@ # |gclient runhooks| to let clang run a plugin in every compilation. # Only has an effect if 'clang=1' is in GYP_DEFINES as well. # Example: - # GYP_DEFINES='clang=1 clang_load=/abs/path/to/libPrintFunctionNames.dylib clang_plugin=print-fns' gclient runhooks + # GYP_DEFINES='clang=1 clang_load=/abs/path/to/libPrintFunctionNames.dylib clang_add_plugin=print-fns' gclient runhooks 'clang_load%': '', 'clang_add_plugin%': '', diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp new file mode 100644 index 0000000..106f16e --- /dev/null +++ b/tools/clang/plugins/ChromeClassTester.cpp @@ -0,0 +1,208 @@ +// Copyright (c) 2011 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. + +// A general interface for filtering and only acting on classes in Chromium C++ +// code. + +#include "ChromeClassTester.h" + +#include "clang/Basic/FileManager.h" + +using namespace clang; + +namespace { + +bool starts_with(const std::string& one, const std::string& two) { + return one.substr(0, two.size()) == two; +} + +bool ends_with(const std::string& one, const std::string& two) { + return one.substr(one.size() - two.size(), two.size()) == two; +} + +} // namespace + +ChromeClassTester::ChromeClassTester(CompilerInstance& instance) + : instance_(instance), + diagnostic_(instance.getDiagnostics()) { + banned_namespaces_.push_back("std"); + banned_namespaces_.push_back("__gnu_cxx"); + + banned_directories_.push_back("third_party"); + banned_directories_.push_back("native_client"); + banned_directories_.push_back("breakpad"); + banned_directories_.push_back("courgette"); + banned_directories_.push_back("ppapi"); + banned_directories_.push_back("/usr"); + banned_directories_.push_back("testing"); + banned_directories_.push_back("googleurl"); + banned_directories_.push_back("v8"); + banned_directories_.push_back("sdch"); + + // You are standing in a mazy of twisty dependencies, all resolved by + // putting everything in the header. + banned_directories_.push_back("chrome/test/automation"); + + // Used in really low level threading code that probably shouldn't be out of + // lined. + ignored_record_names_.push_back("ThreadLocalBoolean"); + + // A complicated pickle derived struct that is all packed integers. + ignored_record_names_.push_back("Header"); + + // Part of the GPU system that uses multiple included header + // weirdness. Never getting this right. + ignored_record_names_.push_back("Validators"); + + // RAII class that's simple enough (media/base/callback.h). + ignored_record_names_.push_back("AutoTaskRunner"); + ignored_record_names_.push_back("AutoCallbackRunner"); + + // Part of our public interface that nacl and friends use. (Arguably, this + // should mean that this is a higher priority but fixing this looks hard.) + ignored_record_names_.push_back("PluginVersionInfo"); +} + +ChromeClassTester::~ChromeClassTester() {} + +void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) { + if (CXXRecordDecl* record = dyn_cast<CXXRecordDecl>(tag)) { + // If this is a POD or a class template or a type dependent on a + // templated class, assume there's no ctor/dtor/virtual method + // optimization that we can do. + if (record->isPOD() || + record->getDescribedClassTemplate() || + record->getTemplateSpecializationKind() || + record->isDependentType()) + return; + + if (InBannedNamespace(record)) + return; + + SourceLocation record_location = record->getInnerLocStart(); + if (InBannedDirectory(record_location)) + return; + + // For now, due to large amounts of inlining in our unit test code, there's + // no way in hell we'll pass our checks but we want to deploy clang plugins + // now. + // + // TODO(erg): Deinline all our test code, and only perform something like + // this check for the "virtual" inlining tests on GMOCK code (since all of + // that is autogenerated and doesn't specify the "virtual" keyword). + if (IsTestCode(record)) { + return; + } + + // We sadly need to maintain a blacklist of types that violate these + // rules, but do so for good reason or due to limitations of this + // checker (i.e., we don't handle extern templates very well). + if (IsIgnoredType(record)) + return; + + CheckChromeClass(record_location, record); + } +} + +void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) { + FullSourceLoc full(loc, instance().getSourceManager()); + std::string err; + err = "[chrome-style] "; + err += raw_error; + unsigned id = diagnostic().getCustomDiagID(Diagnostic::Warning, err); + DiagnosticBuilder B = diagnostic().Report(full, id); +} + +bool ChromeClassTester::IsTestCode(Decl* record) { + if (instance_.hasSourceManager()) { + SourceManager& m = instance_.getSourceManager(); + std::string name = m.getFileEntryForID(m.getMainFileID())->getName(); + return name.find("test") != name.npos || + name.find("mock") != name.npos; + } + return false; +} + +bool ChromeClassTester::InBannedNamespace(Decl* record) { + std::string n = GetNamespace(record); + if (n != "") { + return std::find(banned_namespaces_.begin(), banned_namespaces_.end(), n) + != banned_namespaces_.end(); + } + + return false; +} + +std::string ChromeClassTester::GetNamespace(Decl* record) { + return GetNamespaceImpl(record->getDeclContext(), ""); +} + +std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context, + std::string candidate) { + switch (context->getDeclKind()) { + case Decl::TranslationUnit: { + return candidate; + } + case Decl::Namespace: { + const NamespaceDecl* decl = dyn_cast<NamespaceDecl>(context); + std::string name_str; + llvm::raw_string_ostream OS(name_str); + if (decl->isAnonymousNamespace()) + OS << "<anonymous namespace>"; + else + OS << decl; + return GetNamespaceImpl(context->getParent(), + OS.str()); + } + default: { + return GetNamespaceImpl(context->getParent(), candidate); + } + } +} + +bool ChromeClassTester::InBannedDirectory(const SourceLocation& loc) { + if (loc.isFileID() && loc.isValid()) { + bool invalid = false; + const char* buffer_name = instance_.getSourceManager().getBufferName( + loc, &invalid); + if (!invalid && buffer_name) { + std::string b(buffer_name); + + // Don't complain about these things in implementation files. + if (ends_with(b, ".cc") || ends_with(b, ".cpp")) { + return true; + } + + // Don't complain about autogenerated protobuf files. + if (ends_with(b, ".pb.h")) { + return true; + } + + // Strip preceding path crap. + if (starts_with(b, "./")) + b = b.substr(2); + + for (std::vector<std::string>::const_iterator it = + banned_directories_.begin(); + it != banned_directories_.end(); ++it) { + if (starts_with(b, *it)) + return true; + } + } + } + + return false; +} + +bool ChromeClassTester::IsIgnoredType(RecordDecl* record) { + std::string base_name = record->getNameAsString(); + for (std::vector<std::string>::const_iterator it = + ignored_record_names_.begin(); + it != ignored_record_names_.end(); ++it) { + if (base_name == *it) + return true; + } + + return false; +} diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h new file mode 100644 index 0000000..b9985d7 --- /dev/null +++ b/tools/clang/plugins/ChromeClassTester.h @@ -0,0 +1,63 @@ +// Copyright (c) 2011 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_CHROMECLASSTESTER_H_ +#define TOOLS_CLANG_PLUGINS_CHROMECLASSTESTER_H_ + +#include "clang/AST/ASTConsumer.h" +#include "clang/AST/AST.h" +#include "clang/AST/TypeLoc.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Frontend/CompilerInstance.h" + +#include <vector> + +// A class on top of ASTConsumer that forwards classes defined in Chromium +// headers to subclasses which implement CheckChromeClass(). +class ChromeClassTester : public clang::ASTConsumer { + public: + explicit ChromeClassTester(clang::CompilerInstance& instance); + virtual ~ChromeClassTester(); + + // ASTConsumer: + virtual void HandleTagDeclDefinition(clang::TagDecl* tag); + + protected: + clang::CompilerInstance& instance() { return instance_; } + clang::Diagnostic& diagnostic() { return diagnostic_; } + + // Emits a simple warning; this shouldn't be used if you require printf-style + // printing. + void emitWarning(clang::SourceLocation loc, const char* error); + + private: + // Template method which is called with only classes that are defined in + // chrome header files. + virtual void CheckChromeClass(const clang::SourceLocation& record_location, + clang::CXXRecordDecl* record) = 0; + + // Utility methods used for filtering out non-chrome classes (and ones we + // delibrately ignore) in HandleTagDeclDefinition(). + bool IsTestCode(clang::Decl* record); + bool InBannedNamespace(clang::Decl* record); + std::string GetNamespace(clang::Decl* record); + std::string GetNamespaceImpl(const clang::DeclContext* context, + std::string candidate); + bool InBannedDirectory(const clang::SourceLocation& loc); + bool IsIgnoredType(clang::RecordDecl* record); + + clang::CompilerInstance& instance_; + clang::Diagnostic& diagnostic_; + + // List of banned namespaces. + std::vector<std::string> banned_namespaces_; + + // List of banned directories. + std::vector<std::string> banned_directories_; + + // List of types that we don't check. + std::vector<std::string> ignored_record_names_; +}; + +#endif // TOOLS_CLANG_PLUGINS_CHROMECLASSTESTER_H_ diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp new file mode 100644 index 0000000..60b44dba --- /dev/null +++ b/tools/clang/plugins/FindBadConstructs.cpp @@ -0,0 +1,245 @@ +// Copyright (c) 2011 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. +// - Virtual methods with nonempty implementations in their headers. +// +// Things that are still TODO: +// - Deriving from base::RefCounted and friends should mandate non-public +// destructors. + +#include "clang/Frontend/FrontendPluginRegistry.h" +#include "clang/AST/ASTConsumer.h" +#include "clang/AST/AST.h" +#include "clang/AST/TypeLoc.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Frontend/CompilerInstance.h" +#include "llvm/Support/raw_ostream.h" + +#include "ChromeClassTester.h" + +using namespace clang; + +namespace { + +bool TypeHasNonTrivialDtor(const Type* type) { + if (const CXXRecordDecl* cxx_r = type->getCXXRecordDeclForPointerType()) + return cxx_r->hasTrivialDestructor(); + + return false; +} + +// Searches for constructs that we know we don't want in the Chromium code base. +class FindBadConstructsConsumer : public ChromeClassTester { + public: + FindBadConstructsConsumer(CompilerInstance& instance) + : ChromeClassTester(instance) {} + + virtual void CheckChromeClass(const SourceLocation& record_location, + CXXRecordDecl* record) { + CheckCtorDtorWeight(record_location, record); + CheckVirtualMethods(record_location, record); + } + + // Prints errors if the constructor/destructor weight it too heavy. + void CheckCtorDtorWeight(const 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 a declared 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()) { + 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 a declared destructor."); + } else if (CXXDestructorDecl* dtor = record->getDestructor()) { + if (dtor->hasInlineBody()) { + emitWarning(dtor->getInnerLocStart(), + "Complex destructor has an inline body."); + } + } + } + } + + // Makes sure there is a "virtual" keyword on virtual methods and that there + // are no inline function bodies on them (but "{}" is allowed). + void CheckVirtualMethods(const SourceLocation& record_location, + CXXRecordDecl* record) { + for (CXXRecordDecl::method_iterator it = record->method_begin(); + it != record->method_end(); ++it) { + if (it->isCopyAssignmentOperator() || + dyn_cast<CXXConstructorDecl>(*it)) { + // Ignore constructors and assignment operators. + } else if (dyn_cast<CXXDestructorDecl>(*it)) { + // TODO: I'd love to handle this case, but + // CXXDestructorDecl::isImplicitlyDefined() asserts if I call it + // here, and against my better judgment, I don't want to *always* + // disallow implicit destructors. + } else { + if (it->isVirtual() && !it->isVirtualAsWritten()) { + emitWarning(it->getTypeSpecStartLoc(), + "Overridden method must have \"virtual\" keyword."); + } + + // Virtual methods should not have inline definitions beyond "{}". + if (it->isVirtual() && it->hasBody() && it->hasInlineBody()) { + if (CompoundStmt* cs = dyn_cast<CompoundStmt>(it->getBody())) { + if (cs->size()) { + emitWarning( + cs->getLBracLoc(), + "virtual methods with non-empty bodies shouldn't be " + "declared inline."); + } + } + } + } + } + } + + void CountType(const Type* type, + int* trivial_member, + int* non_trivial_member, + int* templated_non_trivial_member) { + switch (type->getTypeClass()) { + case Type::Record: { + // Simplifying; the whole class isn't trivial if the dtor is, but + // we use this as a signal about complexity. + if (TypeHasNonTrivialDtor(type)) + (*trivial_member)++; + else + (*non_trivial_member)++; + break; + } + case Type::TemplateSpecialization: { + TemplateName name = + dyn_cast<TemplateSpecializationType>(type)->getTemplateName(); + bool whitelisted_template = false; + + // HACK: I'm at a loss about how to get the syntax checker to get + // whether a template is exterened or not. For the first pass here, + // just do retarded string comparisons. + if (TemplateDecl* decl = name.getAsTemplateDecl()) { + std::string base_name = decl->getNameAsString(); + if (base_name == "basic_string") + whitelisted_template = true; + } + + if (whitelisted_template) + (*non_trivial_member)++; + else + (*templated_non_trivial_member)++; + break; + } + case Type::Elaborated: { + CountType( + dyn_cast<ElaboratedType>(type)->getNamedType().getTypePtr(), + trivial_member, non_trivial_member, templated_non_trivial_member); + break; + } + case Type::Typedef: { + while (const TypedefType *TT = dyn_cast<TypedefType>(type)) { + type = TT->getDecl()->getUnderlyingType().getTypePtr(); + } + CountType(type, trivial_member, non_trivial_member, + templated_non_trivial_member); + break; + } + default: { + // Stupid assumption: anything we see that isn't the above is one of + // the 20 integer types. + (*trivial_member)++; + break; + } + } + } +}; + +class FindBadConstructsAction : public PluginASTAction { + protected: + ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) { + return new FindBadConstructsConsumer(CI); + } + + bool ParseArgs(const CompilerInstance &CI, + const std::vector<std::string>& args) { + // We don't take any additional arguments here. + return true; + } +}; + +} // namespace + +static FrontendPluginRegistry::Add<FindBadConstructsAction> +X("find-bad-constructs", "Finds bad C++ constructs"); diff --git a/tools/clang/plugins/Makefile b/tools/clang/plugins/Makefile new file mode 100644 index 0000000..1aa1280 --- /dev/null +++ b/tools/clang/plugins/Makefile @@ -0,0 +1,20 @@ +# This file requires the clang build system, at least for now. So to use this +# Makefile, you should execute the following commands to copy this directory +# into a clang checkout: +# +# cp -R <this directory> /path/to/clang/examples/chromeplugin +# cd /path/to/clang/examples/chromeplugin +# make +# +# You can't just link the directories, or specify clang as a full path for some +# reason. :( +# +# TODO(thakis): Improve the situation. + +CLANG_LEVEL := ../.. +LIBRARYNAME = FindBadConstructs + +LINK_LIBS_IN_SHARED = 1 +SHARED_LIBRARY = 1 + +include $(CLANG_LEVEL)/Makefile diff --git a/tools/clang/plugins/README.chromium b/tools/clang/plugins/README.chromium new file mode 100644 index 0000000..a2ce0ff --- /dev/null +++ b/tools/clang/plugins/README.chromium @@ -0,0 +1,4 @@ +Documentation for this code is: + +- http://code.google.com/p/chromium/wiki/Clang +- http://code.google.com/p/chromium/wiki/WritingClangPlugins diff --git a/tools/clang_check/README.chromium b/tools/clang_check/README.chromium deleted file mode 100644 index fcaa7e0..0000000 --- a/tools/clang_check/README.chromium +++ /dev/null @@ -1,9 +0,0 @@ -These are terrible, terrible hacks. - -They are meant to be temporary. clang currently doesn't allow running a plugin -AND doing the normal codegen process. We want our syntax complaining plugins to -run during normal compile, but for now, you can user run_plugin.sh to hack the -build system to do a syntax check. - -Also see http://code.google.com/p/chromium/wiki/WritingClangPlugins - diff --git a/tools/clang_check/cxx_wrapper.sh b/tools/clang_check/cxx_wrapper.sh deleted file mode 100755 index 9f0ff0f..0000000 --- a/tools/clang_check/cxx_wrapper.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/bin/bash -# -# Copyright (c) 2010 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. -# -# Strip out any parameters that will make clang do anything other than syntax -# checking. -TARGETOBJ=`echo $@ | sed -e 's/.*-o \(.*\.o\).*/\1/;'` - -ADJUSTED=`echo "$@" | sed -e 's/ -c / /g;' |\ - sed -e 's/ -O0 / /g;' |\ - sed -e 's/ -g / /g;' |\ - sed -e 's/Google Inc/GoogleInc/g;' |\ - sed -e 's/ -Wunused-value / /g;' |\ - sed -e 's/ -Wswitch-enum / /g;' |\ - sed -e 's/ -pipe / /g;' |\ - sed -e 's/ -o.*\.o / /g;'` - -clang++ -fsyntax-only $ADJUSTED && touch "$TARGETOBJ" diff --git a/tools/clang_check/link_wrapper.sh b/tools/clang_check/link_wrapper.sh deleted file mode 100755 index c716a56..0000000 --- a/tools/clang_check/link_wrapper.sh +++ /dev/null @@ -1,13 +0,0 @@ -#!/bin/bash -# -# Copyright (c) 2010 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. -# -# Note how we don't actually link anything here. - -# sed always does greedy matching. This is ok in the compiler wrapper scripts -# because -TARGETOBJ=`echo $@ | perl -pe 's/.*-o ([^\s]+).*/$1/;'` - -touch $TARGETOBJ diff --git a/tools/clang_check/run_plugin.sh b/tools/clang_check/run_plugin.sh deleted file mode 100755 index 2404d05..0000000 --- a/tools/clang_check/run_plugin.sh +++ /dev/null @@ -1,13 +0,0 @@ -#!/bin/bash -# -# Copyright (c) 2010 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. -# -# A script that runs a clang plugin against the chrome target on Linux. - -export CXXFLAGS="-Xclang -load -Xclang /work/llvm/Release+Asserts/lib/libFindBadConstructs.so -Xclang -plugin -Xclang find-bad-constructs" -export CXX=`dirname $0`/cxx_wrapper.sh -export LINK=`dirname $0`/link_wrapper.sh - -make -j2 chrome |