Bug 1331434 - Part 1: Add an analysis to require a return after calls to annotated functions, r=ehsan

MozReview-Commit-ID: 7NqXap8FdSn

build/clang-plugin/Checks.inc

@@ -10,6 +10,7 @@ CHECK(ExplicitImplicitChecker, "implicit-constructor")
 CHECK(ExplicitOperatorBoolChecker, "explicit-operator-bool")
 CHECK(KungFuDeathGripChecker, "kungfu-death-grip")
 CHECK(MustOverrideChecker, "must-override")
+CHECK(MustReturnFromCallerChecker, "must-return-from-caller")
 CHECK(MustUseChecker, "must-use")
 CHECK(NaNExprChecker, "nan-expr")
 CHECK(NeedsNoVTableTypeChecker, "needs-no-vtable-type")

build/clang-plugin/ChecksIncludes.inc

@@ -11,6 +11,7 @@
 #include "ExplicitOperatorBoolChecker.h"
 #include "KungFuDeathGripChecker.h"
 #include "MustOverrideChecker.h"
+#include "MustReturnFromCallerChecker.h"
 #include "MustUseChecker.h"
 #include "NaNExprChecker.h"
 #include "NeedsNoVTableTypeChecker.h"

build/clang-plugin/CustomMatchers.h

@@ -231,6 +231,11 @@ AST_MATCHER(CXXMethodDecl, isNonVirtual) {
   const CXXMethodDecl *Decl = Node.getCanonicalDecl();
   return Decl && !Decl->isVirtual();
 }
+
+AST_MATCHER(FunctionDecl, isMozMustReturnFromCaller) {
+  const FunctionDecl *Decl = Node.getCanonicalDecl();
+  return Decl && hasCustomAnnotation(Decl, "moz_must_return_from_caller");
+}
 }
 }
 

build/clang-plugin/MustReturnFromCallerChecker.cpp

@@ -0,0 +1,105 @@
+/* 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/. */
+
+#include "MustReturnFromCallerChecker.h"
+#include "CustomMatchers.h"
+
+void MustReturnFromCallerChecker::registerMatchers(MatchFinder* AstMatcher) {
+  // Look for a call to a MOZ_MUST_RETURN_FROM_CALLER function
+  AstMatcher->addMatcher(callExpr(callee(functionDecl(isMozMustReturnFromCaller())),
+                                  anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+                                        hasAncestor(functionDecl().bind("containing-func")))).bind("call"),
+                         this);
+}
+
+void MustReturnFromCallerChecker::check(
+    const MatchFinder::MatchResult& Result) {
+  const auto *ContainingLambda =
+    Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
+  const auto *ContainingFunc =
+    Result.Nodes.getNodeAs<FunctionDecl>("containing-func");
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+
+  Stmt *Body = nullptr;
+  if (ContainingLambda) {
+    Body = ContainingLambda->getBody();
+  } else if (ContainingFunc) {
+    Body = ContainingFunc->getBody();
+  } else {
+    return;
+  }
+  assert(Body && "Should have a body by this point");
+
+  // Generate the CFG for the enclosing function or decl.
+  CFG::BuildOptions Options;
+  std::unique_ptr<CFG> TheCFG =
+    CFG::buildCFG(nullptr, Body, Result.Context, Options);
+  if (!TheCFG) {
+    return;
+  }
+
+  // Determine which block in the CFG we want to look at the successors of.
+  StmtToBlockMap BlockMap(TheCFG.get(), Result.Context);
+  size_t CallIndex;
+  const auto *Block = BlockMap.blockContainingStmt(Call, &CallIndex);
+  assert(Block && "This statement should be within the CFG!");
+
+  if (!immediatelyReturns(Block, Result.Context, CallIndex + 1)) {
+    diag(Call->getLocStart(),
+         "You must immediately return after calling this function",
+         DiagnosticIDs::Error);
+  }
+}
+
+bool
+MustReturnFromCallerChecker::immediatelyReturns(RecurseGuard<const CFGBlock *> Block,
+                                                ASTContext *TheContext,
+                                                size_t FromIdx) {
+  if (Block.isRepeat()) {
+    return false;
+  }
+
+  for (size_t I = FromIdx; I < Block->size(); ++I) {
+    Optional<CFGStmt> S = (*Block)[I].getAs<CFGStmt>();
+    if (!S) {
+      continue;
+    }
+
+    auto AfterTrivials = IgnoreTrivials(S->getStmt());
+
+    // If we are looking at a ConstructExpr, a DeclRefExpr or a MemberExpr it's
+    // OK to use them after a call to a MOZ_MUST_RETURN_FROM_CALLER function.
+    // It is also, of course, OK to look at a ReturnStmt.
+    if (isa<ReturnStmt>(AfterTrivials) ||
+        isa<CXXConstructExpr>(AfterTrivials) ||
+        isa<DeclRefExpr>(AfterTrivials) ||
+        isa<MemberExpr>(AfterTrivials)) {
+      continue;
+    }
+
+    // It's also OK to call any function or method which is annotated with
+    // MOZ_MAY_CALL_AFTER_MUST_RETURN. We consider all CXXConversionDecls
+    // to be MOZ_MAY_CALL_AFTER_MUST_RETURN (like operator T*()).
+    if (auto CE = dyn_cast<CallExpr>(AfterTrivials)) {
+      auto Callee = CE->getDirectCallee();
+      if (Callee && hasCustomAnnotation(Callee, "moz_may_call_after_must_return")) {
+        continue;
+      }
+
+      if (Callee && isa<CXXConversionDecl>(Callee)) {
+        continue;
+      }
+    }
+
+    // Otherwise, this expression is problematic.
+    return false;
+  }
+
+  for (auto Succ = Block->succ_begin(); Succ != Block->succ_end(); ++Succ) {
+    if (!immediatelyReturns(Block.recurse(*Succ), TheContext, 0)) {
+      return false;
+    }
+  }
+  return true;
+}

build/clang-plugin/MustReturnFromCallerChecker.h

@@ -0,0 +1,26 @@
+/* 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/. */
+
+#ifndef MustReturnFromCallerChecker_h__
+#define MustReturnFromCallerChecker_h__
+
+#include "plugin.h"
+#include "Utils.h"
+#include "RecurseGuard.h"
+#include "StmtToBlockMap.h"
+
+class MustReturnFromCallerChecker : public BaseCheck {
+public:
+  MustReturnFromCallerChecker(StringRef CheckName,
+                              ContextType *Context = nullptr)
+    : BaseCheck(CheckName, Context) {}
+  void registerMatchers(MatchFinder* AstMatcher) override;
+  void check(const MatchFinder::MatchResult &Result) override;
+private:
+  bool immediatelyReturns(RecurseGuard<const CFGBlock *> Block,
+                          ASTContext *TheContext,
+                          size_t FromIdx);
+};
+
+#endif

build/clang-plugin/RecurseGuard.h

@@ -0,0 +1,63 @@
+/* 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/. */
+
+#ifndef RecurseGuard_h__
+#define RecurseGuard_h__
+
+#include "Utils.h"
+
+// This class acts as a tracker for avoiding infinite recursion when traversing
+// chains in CFGs etc.
+//
+// Constructing a RecurseGuard sets up a shared backing store which tracks the
+// currently observed objects. Whenever recursing, use RecurseGuard.recurse(T)
+// to construct another RecurseGuard with the same backing store.
+//
+// The RecurseGuard object will unregister its object when it is destroyed, and
+// has a method `isRepeat()` which will return `true` if the item was already
+// seen.
+template<typename T>
+class RecurseGuard {
+public:
+  RecurseGuard(T Thing) : Thing(Thing), Set(new DenseSet<T>()), Repeat(false) {
+    Set->insert(Thing);
+  }
+  RecurseGuard(T Thing, std::shared_ptr<DenseSet<T>>& Set)
+    : Thing(Thing), Set(Set), Repeat(false) {
+    Repeat = !Set->insert(Thing).second;
+  }
+  RecurseGuard(const RecurseGuard &) = delete;
+  RecurseGuard(RecurseGuard && Other)
+    : Thing(Other.Thing), Set(Other.Set), Repeat(Other.Repeat) {
+    Other.Repeat = true;
+  }
+  ~RecurseGuard() {
+    if (!Repeat) {
+      Set->erase(Thing);
+    }
+  }
+
+  bool isRepeat() { return Repeat; }
+
+  T get() { return Thing; }
+
+  operator T() {
+    return Thing;
+  }
+
+  T operator ->() {
+    return Thing;
+  }
+
+  RecurseGuard recurse(T NewThing) {
+    return RecurseGuard(NewThing, Set);
+  }
+
+private:
+  T Thing;
+  std::shared_ptr<DenseSet<T>> Set;
+  bool Repeat;
+};
+
+#endif // RecurseGuard_h__

build/clang-plugin/StmtToBlockMap.h

@@ -0,0 +1,86 @@
+/* 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/. */
+
+#ifndef StmtToBlockMap_h__
+#define StmtToBlockMap_h__
+
+#include "Utils.h"
+
+// This method is copied from clang-tidy's ExprSequence.cpp.
+//
+// Returns the Stmt nodes that are parents of 'S', skipping any potential
+// intermediate non-Stmt nodes.
+//
+// In almost all cases, this function returns a single parent or no parents at
+// all.
+inline SmallVector<const Stmt *, 1> getParentStmts(const Stmt *S,
+                                                   ASTContext *Context) {
+  SmallVector<const Stmt *, 1> Result;
+
+  ASTContext::DynTypedNodeList Parents = Context->getParents(*S);
+
+  SmallVector<ast_type_traits::DynTypedNode, 1> NodesToProcess(Parents.begin(),
+                                                               Parents.end());
+
+  while (!NodesToProcess.empty()) {
+    ast_type_traits::DynTypedNode Node = NodesToProcess.back();
+    NodesToProcess.pop_back();
+
+    if (const auto *S = Node.get<Stmt>()) {
+      Result.push_back(S);
+    } else {
+      Parents = Context->getParents(Node);
+      NodesToProcess.append(Parents.begin(), Parents.end());
+    }
+  }
+
+  return Result;
+}
+
+// This class is a modified version of the class from clang-tidy's ExprSequence.cpp
+//
+// Maps `Stmt`s to the `CFGBlock` that contains them. Some `Stmt`s may be
+// contained in more than one `CFGBlock`; in this case, they are mapped to the
+// innermost block (i.e. the one that is furthest from the root of the tree).
+// An optional outparameter provides the index into the block where the `Stmt`
+// was found.
+class StmtToBlockMap {
+public:
+  // Initializes the map for the given `CFG`.
+  StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext) : Context(TheContext) {
+    for (const auto *B : *TheCFG) {
+      for (size_t I = 0; I < B->size(); ++I) {
+        if (Optional<CFGStmt> S = (*B)[I].getAs<CFGStmt>()) {
+          Map[S->getStmt()] = std::make_pair(B, I);
+        }
+      }
+    }
+  }
+
+  // Returns the block that S is contained in. Some `Stmt`s may be contained
+  // in more than one `CFGBlock`; in this case, this function returns the
+  // innermost block (i.e. the one that is furthest from the root of the tree).
+  //
+  // The optional outparameter `Index` is set to the index into the block where
+  // the `Stmt` was found.
+  const CFGBlock *blockContainingStmt(const Stmt *S, size_t *Index = nullptr) const {
+    while (!Map.count(S)) {
+      SmallVector<const Stmt *, 1> Parents = getParentStmts(S, Context);
+      if (Parents.empty())
+        return nullptr;
+      S = Parents[0];
+    }
+
+    const auto &E = Map.lookup(S);
+    if (Index) *Index = E.second;
+    return E.first;
+  }
+
+private:
+  ASTContext *Context;
+
+  llvm::DenseMap<const Stmt *, std::pair<const CFGBlock *, size_t>> Map;
+};
+
+#endif // StmtToBlockMap_h__

build/clang-plugin/moz.build

@@ -16,6 +16,7 @@ UNIFIED_SOURCES += [
     'KungFuDeathGripChecker.cpp',
     'MozCheckAction.cpp',
     'MustOverrideChecker.cpp',
+    'MustReturnFromCallerChecker.cpp',
     'MustUseChecker.cpp',
     'NaNExprChecker.cpp',
     'NeedsNoVTableTypeChecker.cpp',

build/clang-plugin/plugin.h

@@ -5,6 +5,7 @@
 #ifndef plugin_h__
 #define plugin_h__
 
+#include "clang/Analysis/CFG.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"

build/clang-plugin/tests/TestMustReturnFromCaller.cpp

@@ -0,0 +1,183 @@
+#include <cstddef>
+
+#define MOZ_MUST_RETURN_FROM_CALLER __attribute__((annotate("moz_must_return_from_caller")))
+#define MOZ_MAY_CALL_AFTER_MUST_RETURN __attribute__((annotate("moz_may_call_after_must_return")))
+
+void MOZ_MUST_RETURN_FROM_CALLER Throw() {}
+void DoAnythingElse();
+int MakeAnInt();
+int MOZ_MAY_CALL_AFTER_MUST_RETURN SafeMakeInt();
+bool Condition();
+
+class Foo {
+public:
+  __attribute__((annotate("moz_implicit"))) Foo(std::nullptr_t);
+  Foo();
+};
+
+void a1() {
+  Throw();
+}
+
+int a2() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  return MakeAnInt();
+}
+
+int a3() {
+  Throw();
+  return 5;
+}
+
+int a4() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  return Condition() ? MakeAnInt() : MakeAnInt();
+}
+
+void a5() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  DoAnythingElse();
+}
+
+int a6() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  DoAnythingElse();
+  return MakeAnInt();
+}
+
+int a7() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  DoAnythingElse();
+  return Condition() ? MakeAnInt() : MakeAnInt();
+}
+
+int a8() {
+  Throw();
+  return SafeMakeInt();
+}
+
+int a9() {
+  if (Condition()) {
+    Throw();
+  }
+  return SafeMakeInt();
+}
+
+void b1() {
+  if (Condition()) {
+    Throw();
+  }
+}
+
+int b2() {
+  if (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+  }
+  return MakeAnInt();
+}
+
+int b3() {
+  if (Condition()) {
+    Throw();
+  }
+  return 5;
+}
+
+int b4() {
+  if (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+  }
+  return Condition() ? MakeAnInt() : MakeAnInt();
+}
+
+void b5() {
+  if (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+  }
+  DoAnythingElse();
+}
+
+void b6() {
+  if (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+    DoAnythingElse();
+  }
+}
+
+void b7() {
+  if (Condition()) {
+    Throw();
+    return;
+  }
+  DoAnythingElse();
+}
+
+void b8() {
+  if (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+    DoAnythingElse();
+    return;
+  }
+  DoAnythingElse();
+}
+
+void b9() {
+  while (Condition()) {
+    Throw(); // expected-error {{You must immediately return after calling this function}}
+  }
+}
+
+void b10() {
+  while (Condition()) {
+    Throw();
+    return;
+  }
+}
+
+void b11() {
+  Throw(); // expected-error {{You must immediately return after calling this function}}
+  if (Condition()) {
+    return;
+  } else {
+    return;
+  }
+}
+
+void b12() {
+  switch (MakeAnInt()) {
+  case 1:
+    break;
+  default:
+    Throw();
+    return;
+  }
+}
+
+void b13() {
+  if (Condition()) {
+    Throw();
+  }
+  return;
+}
+
+Foo b14() {
+  if (Condition()) {
+    Throw();
+    return nullptr;
+  }
+  return nullptr;
+}
+
+Foo b15() {
+  if (Condition()) {
+    Throw();
+  }
+  return nullptr;
+}
+
+Foo b16() {
+  if (Condition()) {
+    Throw();
+  }
+  return Foo();
+}

build/clang-plugin/tests/moz.build

@@ -18,6 +18,7 @@ SOURCES += [
     'TestKungFuDeathGrip.cpp',
     'TestMultipleAnnotations.cpp',
     'TestMustOverride.cpp',
+    'TestMustReturnFromCaller.cpp',
     'TestMustUse.cpp',
     'TestNANTestingExpr.cpp',
     'TestNANTestingExprC.c',

mfbt/Attributes.h

@@ -510,9 +510,18 @@
  *   be used with types which are intended to be implicitly constructed into other
  *   other types before being assigned to variables.
  * MOZ_REQUIRED_BASE_METHOD: Applies to virtual class method declarations.
- *  Sometimes derived classes override methods that need to be called by their
- *  overridden counterparts. This marker indicates that the marked method must
- *  be called by the method that it overrides.
+ *   Sometimes derived classes override methods that need to be called by their
+ *   overridden counterparts. This marker indicates that the marked method must
+ *   be called by the method that it overrides.
+ * MOZ_MUST_RETURN_FROM_CALLER: Applies to function or method declarations.
+ *   Callers of the annotated function/method must return from that function
+ *   within the calling block using an explicit `return` statement.
+ *   Only calls to Constructors, references to local and member variables,
+ *   and calls to functions or methods marked as MOZ_MAY_CALL_AFTER_MUST_RETURN
+ *   may be made after the MUST_RETURN_FROM_CALLER call.
+ * MOZ_MAY_CALL_AFTER_MUST_RETURN: Applies to function or method declarations.
+ *   Calls to these methods may be made in functions after calls a
+ *   MOZ_MUST_RETURN_FROM_CALLER function or method.
  */
 #ifdef MOZ_CLANG_PLUGIN
 #  define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
@@ -550,6 +559,10 @@
     __attribute__((annotate("moz_non_param")))
 #  define MOZ_REQUIRED_BASE_METHOD \
     __attribute__((annotate("moz_required_base_method")))
+#  define MOZ_MUST_RETURN_FROM_CALLER \
+    __attribute__((annotate("moz_must_return_from_caller")))
+#  define MOZ_MAY_CALL_AFTER_MUST_RETURN \
+    __attribute__((annotate("moz_may_call_after_must_return")))
 /*
  * It turns out that clang doesn't like void func() __attribute__ {} without a
  * warning, so use pragmas to disable the warning. This code won't work on GCC
@@ -586,6 +599,8 @@
 #  define MOZ_NON_PARAM /* nothing */
 #  define MOZ_NON_AUTOABLE /* nothing */
 #  define MOZ_REQUIRED_BASE_METHOD /* nothing */
+#  define MOZ_MUST_RETURN_FROM_CALLER /* nothing */
+#  define MOZ_MAY_CALL_AFTER_MUST_RETURN /* nothing */
 #endif /* MOZ_CLANG_PLUGIN */
 
 #define MOZ_RAII MOZ_NON_TEMPORARY_CLASS MOZ_STACK_CLASS