Bug 1536825. Fix the interaction of ignoreTrivials and typechecks in MOZ_CAN_RUN_SCRIPT analysis. r=andi
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 21 Mar 2019 11:50:55 +0000
changeset 465437 1133a148d29c657567b6985e08af96adab3328f7
parent 465436 15a5f72d591f9af00b5379495e8d7258e5840226
child 465438 664d49ea52757224083e93a9742e676bb8689ef4
push id112505
push useropoprus@mozilla.com
push dateThu, 21 Mar 2019 23:01:57 +0000
treeherdermozilla-inbound@feda786b35cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersandi
bugs1536825
milestone68.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 1536825. Fix the interaction of ignoreTrivials and typechecks in MOZ_CAN_RUN_SCRIPT analysis. r=andi We need to typecheck the trivials too, not just the final thing after trivials are stripped, because casts are trivials. Differential Revision: https://phabricator.services.mozilla.com/D24186
build/clang-plugin/CanRunScriptChecker.cpp
build/clang-plugin/CustomMatchers.h
build/clang-plugin/Utils.h
build/clang-plugin/tests/TestCanRunScript.cpp
--- a/build/clang-plugin/CanRunScriptChecker.cpp
+++ b/build/clang-plugin/CanRunScriptChecker.cpp
@@ -76,26 +76,28 @@ void CanRunScriptChecker::registerMatche
   // A matcher that matches some cases that are known live due to local
   // information (as in, not relying on the rest of this analysius to guarantee
   // their liveness).  There's some conceptual overlap with the set of unless()
   // clauses in InvalidArg here, but for our purposes this limited set of cases
   // is fine.
   auto LocalKnownLive = anyOf(KnownLiveSmartPtr, MozKnownLiveCall);
 
   auto InvalidArg =
-      // We want to find any expression,
-      ignoreTrivials(expr(
-          // which has a refcounted pointer type,
-          anyOf(
-            hasType(Refcounted),
-            hasType(pointsTo(Refcounted)),
-            hasType(references(Refcounted)),
-            hasType(isSmartPtrToRefCounted())
-          ),
-          // and which is not this,
+      ignoreTrivialsConditional(
+        // We want to consider things if there is anything refcounted involved,
+        // including in any of the trivials that we otherwise strip off.
+        anyOf(
+          hasType(Refcounted),
+          hasType(pointsTo(Refcounted)),
+          hasType(references(Refcounted)),
+          hasType(isSmartPtrToRefCounted())
+        ),
+        // We want to find any expression,
+        expr(
+          // which is not this,
           unless(cxxThisExpr()),
           // and which is not a stack smart ptr
           unless(KnownLiveSmartPtr),
           // and which is not a method call on a stack smart ptr,
           unless(cxxMemberCallExpr(on(KnownLiveSmartPtr))),
           // and which is not calling operator* or operator-> on a thing that is
           // already known to be live.
           unless(
@@ -106,16 +108,18 @@ void CanRunScriptChecker::registerMatche
               argumentCountIs(1)
             )
           ),
           // and which is not a parameter of the parent function,
           unless(declRefExpr(to(parmVarDecl()))),
           // and which is not a default arg with value nullptr, since those are
           // always safe.
           unless(cxxDefaultArgExpr(isNullDefaultArg())),
+          // and which is not a literal nullptr
+          unless(cxxNullPtrLiteralExpr()),
           // and which is not a dereference of a parameter of the parent
           // function (including "this"),
           unless(
             unaryOperator(
               unaryDereferenceOperator(),
               hasUnaryOperand(
                 anyOf(
                   // If we're doing *someArg, the argument of the dereference is
--- a/build/clang-plugin/CustomMatchers.h
+++ b/build/clang-plugin/CustomMatchers.h
@@ -200,16 +200,41 @@ AST_MATCHER(CXXConstructorDecl, isIntere
       // We don't want deleted constructors.
       !Declaration->isDeleted();
 }
 
 AST_MATCHER_P(Expr, ignoreTrivials, internal::Matcher<Expr>, InnerMatcher) {
   return InnerMatcher.matches(*IgnoreTrivials(&Node), Finder, Builder);
 }
 
+// Takes two matchers: the first one is a condition; the second is a matcher to be
+// applied once we are done unwrapping trivials.  While the condition does not match
+// and we're looking at a trivial, will keep unwrapping the trivial and trying again.
+// Once the condition matches, we will go ahead and unwrap all trivials and apply the
+// inner matcher to the result.
+//
+// The expected use here is if we want to condition a match on some typecheck but
+// apply the match to only non-trivials, because there are trivials (e.g. casts) that
+// can change types.
+AST_MATCHER_P2(Expr, ignoreTrivialsConditional,
+               internal::Matcher<Expr>, Condition,
+               internal::Matcher<Expr>, InnerMatcher) {
+  const Expr *node = &Node;
+  while (true) {
+    if (Condition.matches(*node, Finder, Builder)) {
+      return InnerMatcher.matches(*IgnoreTrivials(node), Finder, Builder);
+    }
+    const Expr *newNode = MaybeSkipOneTrivial(node);
+    if (newNode == node) {
+      return false;
+    }
+    node = newNode;
+  }
+}
+
 // We can't call this "isImplicit" since it clashes with an existing matcher in
 // clang.
 AST_MATCHER(CXXConstructorDecl, isMarkedImplicit) {
   return hasCustomAttribute<moz_implicit>(&Node);
 }
 
 AST_MATCHER(CXXRecordDecl, isConcreteClass) { return !Node.isAbstract(); }
 
--- a/build/clang-plugin/Utils.h
+++ b/build/clang-plugin/Utils.h
@@ -275,42 +275,61 @@ inline bool typeIsRefPtr(QualType Q) {
     return true;
   }
   return false;
 }
 
 // The method defined in clang for ignoring implicit nodes doesn't work with
 // some AST trees. To get around this, we define our own implementation of
 // IgnoreTrivials.
-inline const Stmt *IgnoreTrivials(const Stmt *s) {
-  while (true) {
+inline const Stmt *MaybeSkipOneTrivial(const Stmt *s) {
     if (!s) {
       return nullptr;
-    } else if (auto *ewc = dyn_cast<ExprWithCleanups>(s)) {
-      s = ewc->getSubExpr();
-    } else if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s)) {
-      s = mte->GetTemporaryExpr();
-    } else if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s)) {
-      s = bte->getSubExpr();
-    } else if (auto *ce = dyn_cast<CastExpr>(s)) {
+    }
+    if (auto *ewc = dyn_cast<ExprWithCleanups>(s)) {
+      return ewc->getSubExpr();
+    }
+    if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s)) {
+      return mte->GetTemporaryExpr();
+    }
+    if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s)) {
+      return bte->getSubExpr();
+    }
+    if (auto *ce = dyn_cast<CastExpr>(s)) {
       s = ce->getSubExpr();
-    } else if (auto *pe = dyn_cast<ParenExpr>(s)) {
+    }
+    if (auto *pe = dyn_cast<ParenExpr>(s)) {
       s = pe->getSubExpr();
-    } else {
-      break;
     }
+    // Not a trivial.
+    return s;
+}
+
+inline const Stmt *IgnoreTrivials(const Stmt *s) {
+  while (true) {
+    const Stmt* newS = MaybeSkipOneTrivial(s);
+    if (newS == s) {
+      return newS;
+    }
+    s = newS;
   }
 
-  return s;
+  // Unreachable
+  return nullptr;
 }
 
 inline const Expr *IgnoreTrivials(const Expr *e) {
   return cast_or_null<Expr>(IgnoreTrivials(static_cast<const Stmt *>(e)));
 }
 
+// Returns the input if the input is not a trivial.
+inline const Expr *MaybeSkipOneTrivial(const Expr *e) {
+  return cast_or_null<Expr>(MaybeSkipOneTrivial(static_cast<const Stmt *>(e)));
+}
+
 const FieldDecl *getBaseRefCntMember(QualType T);
 
 inline const FieldDecl *getBaseRefCntMember(const CXXRecordDecl *D) {
   const FieldDecl *RefCntMember = getClassRefCntMember(D);
   if (RefCntMember && isClassRefCounted(D)) {
     return RefCntMember;
   }
 
--- a/build/clang-plugin/tests/TestCanRunScript.cpp
+++ b/build/clang-plugin/tests/TestCanRunScript.cpp
@@ -31,16 +31,24 @@ struct RefCountedBase {
 
   MOZ_CAN_RUN_SCRIPT void method_test2() {
     test2(this);
   }
 
   virtual void method_test3() { // expected-note {{caller function declared here}}
     test(); // expected-error {{functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT}}
   }
+
+  MOZ_CAN_RUN_SCRIPT void method_test4() {
+    method_test();
+  }
+
+  MOZ_CAN_RUN_SCRIPT void method_test5() {
+    this->method_test();
+  }
 };
 
 MOZ_CAN_RUN_SCRIPT void testLambda() {
   auto doIt = []() MOZ_CAN_RUN_SCRIPT {
     test();
   };
 
   auto doItWrong = []() { // expected-note {{caller function declared here}}
@@ -139,16 +147,21 @@ MOZ_CAN_RUN_SCRIPT_BOUNDARY void test5()
   refptr4->method_test3();
 }
 
 // We should be able to call test5 from a non-can_run_script function.
 void test5_b() {
   test5();
 }
 
+MOZ_CAN_RUN_SCRIPT void test6() {
+  void* x = new RefCountedBase();
+  test2((RefCountedBase*)x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+}
+
 MOZ_CAN_RUN_SCRIPT void test_ref(const RefCountedBase&) {
 
 }
 
 MOZ_CAN_RUN_SCRIPT void test_ref_1() {
   RefCountedBase* t = new RefCountedBase;
   test_ref(*t); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
 }
@@ -187,16 +200,26 @@ MOZ_CAN_RUN_SCRIPT void test_ref_7() {
 }
 
 MOZ_CAN_RUN_SCRIPT void test_ref_8() {
   RefPtr<RefCountedBase> t = new RefCountedBase;
   auto& ref = *t;
   test_ref(MOZ_KnownLive(ref));
 }
 
+MOZ_CAN_RUN_SCRIPT void test_ref_9() {
+  void* x = new RefCountedBase();
+  test_ref(*(RefCountedBase*)x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+}
+
+MOZ_CAN_RUN_SCRIPT void test_ref_10() {
+  void* x = new RefCountedBase();
+  test_ref((RefCountedBase&)*x); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}} expected-error {{ISO C++ does not allow indirection on operand of type 'void *'}}
+}
+
 MOZ_CAN_RUN_SCRIPT void test_maybe() {
   mozilla::Maybe<RefCountedBase*> unsafe;
   unsafe.emplace(new RefCountedBase);
   (*unsafe)->method_test(); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
 }
 
 MOZ_CAN_RUN_SCRIPT void test_maybe_2() {
   // FIXME(bz): This should not generate an error!