Commit my clang plugin and fix up documentation.

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6368055

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@73659 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 540 insertions, 0 deletions

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