Bug 773014 - Part 1: Add an analysis for marking classes as global-only; r=jrmuizel
authorEhsan Akhgari <ehsan@mozilla.com>
Mon, 22 Dec 2014 18:10:32 -0500
changeset 221006 baf97266ac9b138c73b6fce6a65cf99383a3cc6d
parent 221005 c12fab47017b88bf16e4841da432cae8f06ee14d
child 221007 6b3f00c078b1ca1e01f5d73fef37955b894c1034
push id28009
push userryanvm@gmail.com
push dateTue, 23 Dec 2014 18:17:16 +0000
treeherdermozilla-central@44344099d119 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs773014
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 773014 - Part 1: Add an analysis for marking classes as global-only; r=jrmuizel
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestGlobalClass.cpp
build/clang-plugin/tests/moz.build
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -34,34 +34,43 @@ class DiagnosticsMatcher {
 public:
   DiagnosticsMatcher();
 
   ASTConsumerPtr makeASTConsumer() {
     return astMatcher.newASTConsumer();
   }
 
 private:
-  class StackClassChecker : public MatchFinder::MatchCallback {
+  class ScopeChecker : public MatchFinder::MatchCallback {
   public:
+    enum Scope {
+      eLocal,
+      eGlobal
+    };
+    ScopeChecker(Scope scope_) :
+      scope(scope_) {}
     virtual void run(const MatchFinder::MatchResult &Result);
     void noteInferred(QualType T, DiagnosticsEngine &Diag);
+  private:
+    Scope scope;
   };
 
   class NonHeapClassChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
     void noteInferred(QualType T, DiagnosticsEngine &Diag);
   };
 
   class ArithmeticArgChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
-  StackClassChecker stackClassChecker;
+  ScopeChecker stackClassChecker;
+  ScopeChecker globalClassChecker;
   NonHeapClassChecker nonheapClassChecker;
   ArithmeticArgChecker arithmeticArgChecker;
   MatchFinder astMatcher;
 };
 
 namespace {
 
 bool isInIgnoredNamespace(const Decl *decl) {
@@ -231,17 +240,18 @@ public:
 /**
  * Where classes may be allocated. Regular classes can be allocated anywhere,
  * non-heap classes on the stack or as static variables, and stack classes only
  * on the stack. Note that stack classes subsumes non-heap classes.
  */
 enum ClassAllocationNature {
   RegularClass = 0,
   NonHeapClass = 1,
-  StackClass = 2
+  StackClass = 2,
+  GlobalClass = 3
 };
 
 /// A cached data of whether classes are stack classes, non-heap classes, or
 /// neither.
 DenseMap<const CXXRecordDecl *,
   std::pair<const Decl *, ClassAllocationNature> > inferredAllocCauses;
 
 ClassAllocationNature getClassAttrs(QualType T);
@@ -250,16 +260,19 @@ ClassAllocationNature getClassAttrs(CXXR
   // Normalize so that D points to the definition if it exists. If it doesn't,
   // then we can't allocate it anyways.
   if (!D->hasDefinition())
     return RegularClass;
   D = D->getDefinition();
   // Base class: anyone with this annotation is obviously a stack class
   if (MozChecker::hasCustomAnnotation(D, "moz_stack_class"))
     return StackClass;
+  // Base class: anyone with this annotation is obviously a global class
+  if (MozChecker::hasCustomAnnotation(D, "moz_global_class"))
+    return GlobalClass;
 
   // See if we cached the result.
   DenseMap<const CXXRecordDecl *,
     std::pair<const Decl *, ClassAllocationNature> >::iterator it =
     inferredAllocCauses.find(D);
   if (it != inferredAllocCauses.end()) {
     return it->second.second;
   }
@@ -278,30 +291,37 @@ ClassAllocationNature getClassAttrs(CXXR
   // going.
   for (CXXRecordDecl::base_class_iterator base = D->bases_begin(),
        e = D->bases_end(); base != e; ++base) {
     ClassAllocationNature super = getClassAttrs(base->getType());
     if (super == StackClass) {
       inferredAllocCauses[D] = std::make_pair(
         base->getType()->getAsCXXRecordDecl(), StackClass);
       return StackClass;
+    } else if (super == GlobalClass) {
+      inferredAllocCauses[D] = std::make_pair(
+        base->getType()->getAsCXXRecordDecl(), GlobalClass);
+      return GlobalClass;
     } else if (super == NonHeapClass) {
       inferredAllocCauses[D] = std::make_pair(
         base->getType()->getAsCXXRecordDecl(), NonHeapClass);
       type = NonHeapClass;
     }
   }
 
   // Maybe it has a member which is a stack class.
   for (RecordDecl::field_iterator field = D->field_begin(), e = D->field_end();
        field != e; ++field) {
     ClassAllocationNature fieldType = getClassAttrs(field->getType());
     if (fieldType == StackClass) {
       inferredAllocCauses[D] = std::make_pair(*field, StackClass);
       return StackClass;
+    } else if (fieldType == GlobalClass) {
+      inferredAllocCauses[D] = std::make_pair(*field, GlobalClass);
+      return GlobalClass;
     } else if (fieldType == NonHeapClass) {
       inferredAllocCauses[D] = std::make_pair(*field, NonHeapClass);
       type = NonHeapClass;
     }
   }
 
   return type;
 }
@@ -319,16 +339,22 @@ namespace clang {
 namespace ast_matchers {
 
 /// This matcher will match any class with the stack class assertion or an
 /// array of such classes.
 AST_MATCHER(QualType, stackClassAggregate) {
   return getClassAttrs(Node) == StackClass;
 }
 
+/// This matcher will match any class with the global class assertion or an
+/// array of such classes.
+AST_MATCHER(QualType, globalClassAggregate) {
+  return getClassAttrs(Node) == GlobalClass;
+}
+
 /// This matcher will match any class with the stack class assertion or an
 /// array of such classes.
 AST_MATCHER(QualType, nonheapClassAggregate) {
   return getClassAttrs(Node) == NonHeapClass;
 }
 
 /// This matcher will match any function declaration that is declared as a heap
 /// allocator.
@@ -388,40 +414,55 @@ bool isPlacementNew(const CXXNewExpr *ex
   if (expr->getNumPlacementArgs() == 0)
     return false;
   if (MozChecker::hasCustomAnnotation(expr->getOperatorNew(),
       "moz_heap_allocator"))
     return false;
   return true;
 }
 
-DiagnosticsMatcher::DiagnosticsMatcher() {
+DiagnosticsMatcher::DiagnosticsMatcher()
+  : stackClassChecker(ScopeChecker::eLocal),
+    globalClassChecker(ScopeChecker::eGlobal)
+{
   // Stack class assertion: non-local variables of a stack class are forbidden
   // (non-localness checked in the callback)
   astMatcher.addMatcher(varDecl(hasType(stackClassAggregate())).bind("node"),
     &stackClassChecker);
   // Stack class assertion: new stack class is forbidden (unless placement new)
   astMatcher.addMatcher(newExpr(hasType(pointerType(
       pointee(stackClassAggregate())
     ))).bind("node"), &stackClassChecker);
+  // Global class assertion: non-global variables of a global class are forbidden
+  // (globalness checked in the callback)
+  astMatcher.addMatcher(varDecl(hasType(globalClassAggregate())).bind("node"),
+    &globalClassChecker);
+  // Global class assertion: new global class is forbidden
+  astMatcher.addMatcher(newExpr(hasType(pointerType(
+      pointee(globalClassAggregate())
+    ))).bind("node"), &globalClassChecker);
   // Non-heap class assertion: new non-heap class is forbidden (unless placement
   // new)
   astMatcher.addMatcher(newExpr(hasType(pointerType(
       pointee(nonheapClassAggregate())
     ))).bind("node"), &nonheapClassChecker);
 
-  // Any heap allocation function that returns a non-heap or a stack class is
-  // definitely doing something wrong
+  // Any heap allocation function that returns a non-heap or a stack class or
+  // a global class is definitely doing something wrong
   astMatcher.addMatcher(callExpr(callee(functionDecl(allOf(heapAllocator(),
       returns(pointerType(pointee(nonheapClassAggregate()))))))).bind("node"),
     &nonheapClassChecker);
   astMatcher.addMatcher(callExpr(callee(functionDecl(allOf(heapAllocator(),
       returns(pointerType(pointee(stackClassAggregate()))))))).bind("node"),
     &stackClassChecker);
 
+  astMatcher.addMatcher(callExpr(callee(functionDecl(allOf(heapAllocator(),
+      returns(pointerType(pointee(globalClassAggregate()))))))).bind("node"),
+    &globalClassChecker);
+
   astMatcher.addMatcher(callExpr(allOf(hasDeclaration(noArithmeticExprInArgs()),
           anyOf(
               hasDescendant(binaryOperator(allOf(binaryArithmeticOperator(),
                   hasLHS(hasDescendant(declRefExpr())),
                   hasRHS(hasDescendant(declRefExpr()))
               )).bind("node")),
               hasDescendant(unaryOperator(allOf(unaryArithmeticOperator(),
                   hasUnaryOperand(allOf(hasType(builtinType()),
@@ -440,68 +481,86 @@ DiagnosticsMatcher::DiagnosticsMatcher()
                   hasUnaryOperand(allOf(hasType(builtinType()),
                                         anyOf(hasDescendant(declRefExpr()), declRefExpr())))
               )).bind("node"))
           )
       )).bind("call"),
     &arithmeticArgChecker);
 }
 
-void DiagnosticsMatcher::StackClassChecker::run(
+void DiagnosticsMatcher::ScopeChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
   unsigned stackID = Diag.getDiagnosticIDs()->getCustomDiagID(
     DiagnosticIDs::Error, "variable of type %0 only valid on the stack");
+  unsigned globalID = Diag.getDiagnosticIDs()->getCustomDiagID(
+    DiagnosticIDs::Error, "variable of type %0 only valid as global");
+  unsigned errorID = (scope == eGlobal) ? globalID : stackID;
   if (const VarDecl *d = Result.Nodes.getNodeAs<VarDecl>("node")) {
-    // Ignore the match if it's a local variable.
-    if (d->hasLocalStorage())
-      return;
+    if (scope == eLocal) {
+      // Ignore the match if it's a local variable.
+      if (d->hasLocalStorage())
+        return;
+    } else if (scope == eGlobal) {
+      // Ignore the match if it's a global variable or a static member of a
+      // class.  The latter is technically not in the global scope, but for the
+      // use case of classes that intend to avoid introducing static
+      // initializers that is fine.
+      if (d->hasGlobalStorage() && !d->isStaticLocal())
+        return;
+    }
 
-    Diag.Report(d->getLocation(), stackID) << d->getType();
+    Diag.Report(d->getLocation(), errorID) << d->getType();
     noteInferred(d->getType(), Diag);
   } else if (const CXXNewExpr *expr =
       Result.Nodes.getNodeAs<CXXNewExpr>("node")) {
     // If it's placement new, then this match doesn't count.
-    if (isPlacementNew(expr))
+    if (scope == eLocal && isPlacementNew(expr))
       return;
-    Diag.Report(expr->getStartLoc(), stackID) << expr->getAllocatedType();
+    Diag.Report(expr->getStartLoc(), errorID) << expr->getAllocatedType();
     noteInferred(expr->getAllocatedType(), Diag);
   } else if (const CallExpr *expr =
       Result.Nodes.getNodeAs<CallExpr>("node")) {
     QualType badType = expr->getCallReturnType()->getPointeeType();
-    Diag.Report(expr->getLocStart(), stackID) << badType;
+    Diag.Report(expr->getLocStart(), errorID) << badType;
     noteInferred(badType, Diag);
   }
 }
 
-void DiagnosticsMatcher::StackClassChecker::noteInferred(QualType T,
+void DiagnosticsMatcher::ScopeChecker::noteInferred(QualType T,
     DiagnosticsEngine &Diag) {
   unsigned inheritsID = Diag.getDiagnosticIDs()->getCustomDiagID(
     DiagnosticIDs::Note,
-    "%0 is a stack class because it inherits from a stack class %1");
+    "%0 is a %2 class because it inherits from a %2 class %1");
   unsigned memberID = Diag.getDiagnosticIDs()->getCustomDiagID(
     DiagnosticIDs::Note,
-    "%0 is a stack class because member %1 is a stack class %2");
+    "%0 is a %3 class because member %1 is a %3 class %2");
+  const char* attribute = (scope == eGlobal) ?
+    "moz_global_class" : "moz_stack_class";
+  const char* type = (scope == eGlobal) ?
+    "global" : "stack";
 
-  // Find the CXXRecordDecl that is the stack class of interest
+  // Find the CXXRecordDecl that is the local/global class of interest
   while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
     T = arrTy->getElementType();
   CXXRecordDecl *clazz = T->getAsCXXRecordDecl();
 
   // Direct result, we're done.
-  if (MozChecker::hasCustomAnnotation(clazz, "moz_stack_class"))
+  if (MozChecker::hasCustomAnnotation(clazz, attribute))
     return;
 
   const Decl *cause = inferredAllocCauses[clazz].first;
   if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(cause)) {
-    Diag.Report(clazz->getLocation(), inheritsID) << T << CRD->getDeclName();
+    Diag.Report(clazz->getLocation(), inheritsID) <<
+      T << CRD->getDeclName() << type;
   } else if (const FieldDecl *FD = dyn_cast<FieldDecl>(cause)) {
-    Diag.Report(FD->getLocation(), memberID) << T << FD << FD->getType();
+    Diag.Report(FD->getLocation(), memberID) <<
+      T << FD << FD->getType() << type;
   }
-  
+
   // Recursively follow this back.
   noteInferred(cast<ValueDecl>(cause)->getType(), Diag);
 }
 
 void DiagnosticsMatcher::NonHeapClassChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
   unsigned stackID = Diag.getDiagnosticIDs()->getCustomDiagID(
new file mode 100644
--- /dev/null
+++ b/build/clang-plugin/tests/TestGlobalClass.cpp
@@ -0,0 +1,52 @@
+#define MOZ_GLOBAL_CLASS __attribute__((annotate("moz_global_class")))
+#include <stddef.h>
+
+struct MOZ_GLOBAL_CLASS Global {
+  int i;
+  void *operator new(size_t x) throw() { return 0; }
+  void *operator new(size_t blah, char *buffer) { return buffer; }
+};
+
+template <class T>
+struct MOZ_GLOBAL_CLASS TemplateClass {
+  T i;
+};
+
+void gobble(void *) { }
+
+void misuseGlobalClass(int len) {
+  Global notValid; // expected-error {{variable of type 'Global' only valid as global}}
+  Global alsoNotValid[2]; // expected-error {{variable of type 'Global [2]' only valid as global}}
+  static Global valid; // expected-error {{variable of type 'Global' only valid as global}}
+  static Global alsoValid[2]; // expected-error {{variable of type 'Global [2]' only valid as global}}
+
+  gobble(&valid);
+  gobble(&notValid);
+  gobble(&alsoValid[0]);
+
+  gobble(new Global); // expected-error {{variable of type 'Global' only valid as global}}
+  gobble(new Global[10]); // expected-error {{variable of type 'Global' only valid as global}}
+  gobble(new TemplateClass<int>); // expected-error {{variable of type 'TemplateClass<int>' only valid as global}}
+  gobble(len <= 5 ? &valid : new Global); // expected-error {{variable of type 'Global' only valid as global}}
+
+  char buffer[sizeof(Global)];
+  gobble(new (buffer) Global); // expected-error {{variable of type 'Global' only valid as global}}
+}
+
+Global valid;
+struct RandomClass {
+  Global nonstaticMember; // expected-note {{'RandomClass' is a global class because member 'nonstaticMember' is a global class 'Global'}}
+  static Global staticMember;
+};
+struct MOZ_GLOBAL_CLASS RandomGlobalClass {
+  Global nonstaticMember;
+  static Global staticMember;
+};
+
+struct BadInherit : Global {}; // expected-note {{'BadInherit' is a global class because it inherits from a global class 'Global'}}
+struct MOZ_GLOBAL_CLASS GoodInherit : Global {};
+
+void misuseGlobalClassEvenMore(int len) {
+  BadInherit moreInvalid; // expected-error {{variable of type 'BadInherit' only valid as global}}
+  RandomClass evenMoreInvalid; // expected-error {{variable of type 'RandomClass' only valid as global}}
+}
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -2,16 +2,17 @@
 # vim: set filetype=python:
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 SOURCES += [
     'TestBadImplicitConversionCtor.cpp',
     'TestCustomHeap.cpp',
+    'TestGlobalClass.cpp',
     'TestMustOverride.cpp',
     'TestNoArithmeticExprInArgument.cpp',
     'TestNonHeapClass.cpp',
     'TestStackClass.cpp',
 ]
 
 DISABLE_STL_WRAPPING = True
 NO_VISIBILITY_FLAGS = True