Bug 1153304 - Add an analysis to prohibit the usage of pointers to refcounted types inside C++ lambdas; r=jrmuizel

build/clang-plugin/clang-plugin.cpp

@@ -79,6 +79,11 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class RefCountedInsideLambdaChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker stackClassChecker;
   ScopeChecker globalClassChecker;
   NonHeapClassChecker nonheapClassChecker;
@@ -86,6 +91,7 @@ private:
   TrivialCtorDtorChecker trivialCtorDtorChecker;
   NaNExprChecker nanExprChecker;
   NoAddRefReleaseOnReturnChecker noAddRefReleaseOnReturnChecker;
+  RefCountedInsideLambdaChecker refCountedInsideLambdaChecker;
   MatchFinder astMatcher;
 };
 
@@ -354,6 +360,60 @@ ClassAllocationNature getClassAttrs(QualType T) {
   return clazz ? getClassAttrs(clazz) : RegularClass;
 }
 
+/// A cached data of whether classes are refcounted or not.
+typedef DenseMap<const CXXRecordDecl *,
+  std::pair<const Decl *, bool> > RefCountedMap;
+RefCountedMap refCountedClasses;
+
+bool classHasAddRefRelease(const CXXRecordDecl *D) {
+  const RefCountedMap::iterator& it = refCountedClasses.find(D);
+  if (it != refCountedClasses.end()) {
+    return it->second.second;
+  }
+
+  bool seenAddRef = false;
+  bool seenRelease = false;
+  for (const auto& method : D->methods()) {
+    std::string name = method->getNameAsString();
+    if (name == "AddRef") {
+      seenAddRef = true;
+    } else if (name == "Release") {
+      seenRelease = true;
+    }
+  }
+  refCountedClasses[D] = std::make_pair(D, seenAddRef && seenRelease);
+  return seenAddRef && seenRelease;
+}
+
+bool isClassRefCounted(QualType T);
+
+bool isClassRefCounted(const CXXRecordDecl *D) {
+  // Normalize so that D points to the definition if it exists.
+  if (!D->hasDefinition())
+    return false;
+  D = D->getDefinition();
+  // Base class: anyone with AddRef/Release is obviously a refcounted class.
+  if (classHasAddRefRelease(D))
+    return true;
+
+  // Look through all base cases to figure out if the parent is a refcounted class.
+  for (const auto& base : D->bases()) {
+    bool super = isClassRefCounted(base.getType());
+    if (super) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
+bool isClassRefCounted(QualType T) {
+  while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
+    T = arrTy->getElementType();
+  CXXRecordDecl *clazz = T->getAsCXXRecordDecl();
+  return clazz ? isClassRefCounted(clazz) : RegularClass;
+}
+
 }
 
 namespace clang {
@@ -481,6 +541,11 @@ AST_MATCHER(MemberExpr, isAddRefOrRelease) {
   return false;
 }
 
+/// This matcher will select classes which are refcounted.
+AST_MATCHER(QualType, isRefCounted) {
+  return isClassRefCounted(Node);
+}
+
 }
 }
 
@@ -577,6 +642,11 @@ DiagnosticsMatcher::DiagnosticsMatcher()
                                                       hasParent(callExpr())).bind("member")
       )).bind("node"),
     &noAddRefReleaseOnReturnChecker);
+
+  astMatcher.addMatcher(lambdaExpr(
+            hasDescendant(declRefExpr(hasType(pointerType(pointee(isRefCounted())))).bind("node"))
+        ),
+    &refCountedInsideLambdaChecker);
 }
 
 void DiagnosticsMatcher::ScopeChecker::run(
@@ -775,6 +845,20 @@ void DiagnosticsMatcher::NoAddRefReleaseOnReturnChecker::run(
   Diag.Report(node->getLocStart(), errorID) << func << method;
 }
 
+void DiagnosticsMatcher::RefCountedInsideLambdaChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned errorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "Refcounted variable %0 of type %1 cannot be used inside a lambda");
+  unsigned noteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "Please consider using a smart pointer");
+  const DeclRefExpr *node = Result.Nodes.getNodeAs<DeclRefExpr>("node");
+
+  Diag.Report(node->getLocStart(), errorID) << node->getFoundDecl() <<
+    node->getType()->getPointeeType();
+  Diag.Report(node->getLocStart(), noteID);
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {

build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp

@@ -0,0 +1,58 @@
+#define MOZ_STRONG_REF __attribute__((annotate("moz_strong_ref")))
+
+struct RefCountedBase {
+  void AddRef();
+  void Release();
+};
+
+template <class T>
+struct SmartPtr {
+  T* MOZ_STRONG_REF t;
+  T* operator->() const;
+};
+
+struct R : RefCountedBase {
+  void method();
+};
+
+void take(...);
+void foo() {
+  R* ptr;
+  SmartPtr<R> sp;
+  take([&]() {
+    ptr->method(); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([&]() {
+    sp->method();
+  });
+  take([&]() {
+    take(ptr); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([&]() {
+    take(sp);
+  });
+  take([=]() {
+    ptr->method(); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([=]() {
+    sp->method();
+  });
+  take([=]() {
+    take(ptr); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([=]() {
+    take(sp);
+  });
+  take([ptr]() {
+    ptr->method(); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([sp]() {
+    sp->method();
+  });
+  take([ptr]() {
+    take(ptr); // expected-error{{Refcounted variable 'ptr' of type 'R' cannot be used inside a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  take([sp]() {
+    take(sp);
+  });
+}

build/clang-plugin/tests/moz.build

@@ -14,6 +14,7 @@ SOURCES += [
     'TestNoAddRefReleaseOnReturn.cpp',
     'TestNoArithmeticExprInArgument.cpp',
     'TestNonHeapClass.cpp',
+    'TestNoRefcountedInsideLambdas.cpp',
     'TestStackClass.cpp',
     'TestTrivialCtorDtor.cpp',
 ]