Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression. r=mystor

MozReview-Commit-ID: AybStmi6MIH

--HG--
extra : rebase_source : 6379599db347975436181c3807893f939f624b24

build/clang-plugin/clang-plugin.cpp

@@ -39,6 +39,32 @@ typedef ASTConsumer *ASTConsumerPtr;
 #define cxxRecordDecl recordDecl
 #endif
 
+// Check if the given expression contains an assignment expression.
+// This can either take the form of a Binary Operator or a
+// Overloaded Operator Call.
+bool HasSideEffectAssignment(const Expr *expr) {
+  if (auto opCallExpr = dyn_cast_or_null<CXXOperatorCallExpr>(expr)) {
+    auto binOp = opCallExpr->getOperator();
+    if (binOp == OO_Equal || (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual)) {
+      return true;
+    }
+  } else if (auto binOpExpr = dyn_cast_or_null<BinaryOperator>(expr)) {
+    if (binOpExpr->isAssignmentOp()) {
+      return true;
+    }
+  }
+
+  // Recurse to children.
+  for (const Stmt *SubStmt : expr->children()) {
+    auto childExpr = dyn_cast_or_null<Expr>(SubStmt);
+    if (childExpr && HasSideEffectAssignment(childExpr)) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 namespace {
 
 using namespace clang::ast_matchers;
@@ -124,6 +150,11 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class AssertAssignmentChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker scopeChecker;
   ArithmeticArgChecker arithmeticArgChecker;
   TrivialCtorDtorChecker trivialCtorDtorChecker;
@@ -139,6 +170,7 @@ private:
   NoAutoTypeChecker noAutoTypeChecker;
   NoExplicitMoveConstructorChecker noExplicitMoveConstructorChecker;
   RefCountedCopyConstructorChecker refCountedCopyConstructorChecker;
+  AssertAssignmentChecker assertAttributionChecker;
   MatchFinder astMatcher;
 };
 
@@ -762,6 +794,15 @@ AST_MATCHER(CXXConstructorDecl, isExplicitMoveConstructor) {
 AST_MATCHER(CXXConstructorDecl, isCompilerProvidedCopyConstructor) {
   return !Node.isUserProvided() && Node.isCopyConstructor();
 }
+
+AST_MATCHER(CallExpr, isAssertAssignmentTestFunc) {
+  static const std::string assertName = "MOZ_AssertAssignmentTest";
+  const FunctionDecl *method = Node.getDirectCallee();
+
+  return method
+      && method->getDeclName().isIdentifier()
+      && method->getName() == assertName;
+}
 }
 }
 
@@ -1070,6 +1111,10 @@ DiagnosticsMatcher::DiagnosticsMatcher() {
                                             ofClass(hasRefCntMember()))))
           .bind("node"),
       &refCountedCopyConstructorChecker);
+
+  astMatcher.addMatcher(
+      callExpr(isAssertAssignmentTestFunc()).bind("funcCall"),
+      &assertAttributionChecker);
 }
 
 // These enum variants determine whether an allocation has occured in the code.
@@ -1545,6 +1590,18 @@ void DiagnosticsMatcher::RefCountedCopyConstructorChecker::run(
   Diag.Report(E->getLocation(), NoteID);
 }
 
+void DiagnosticsMatcher::AssertAssignmentChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned assignInsteadOfComp = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "Forbidden assignment in assert expression");
+  const CallExpr *funcCall = Result.Nodes.getNodeAs<CallExpr>("funcCall");
+
+  if (funcCall && HasSideEffectAssignment(funcCall)) {
+    Diag.Report(funcCall->getLocStart(), assignInsteadOfComp);
+  }
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI,

build/clang-plugin/tests/TestAssertWithAssignment.cpp

@@ -0,0 +1,68 @@
+#include "mozilla/MacroArgs.h"
+
+static __attribute__((always_inline)) bool MOZ_AssertAssignmentTest(bool expr) {
+  return expr;
+}
+
+#define MOZ_UNLIKELY(x) (__builtin_expect(!!(x), 0))
+#define MOZ_CRASH() do { } while(0)
+#define MOZ_CHECK_ASSERT_ASSIGNMENT(expr) MOZ_AssertAssignmentTest(!!(expr))
+
+#define MOZ_ASSERT_HELPER1(expr) \
+  do { \
+    if (MOZ_UNLIKELY(!MOZ_CHECK_ASSERT_ASSIGNMENT(expr))) { \
+      MOZ_CRASH();\
+    } \
+  } while(0) \
+
+/* Now the two-argument form. */
+#define MOZ_ASSERT_HELPER2(expr, explain) \
+  do { \
+    if (MOZ_UNLIKELY(!MOZ_CHECK_ASSERT_ASSIGNMENT(expr))) { \
+      MOZ_CRASH();\
+    } \
+  } while(0) \
+
+#define MOZ_RELEASE_ASSERT_GLUE(a, b) a b
+#define MOZ_RELEASE_ASSERT(...) \
+  MOZ_RELEASE_ASSERT_GLUE( \
+    MOZ_PASTE_PREFIX_AND_ARG_COUNT(MOZ_ASSERT_HELPER, __VA_ARGS__), \
+    (__VA_ARGS__))
+
+#define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
+
+void FunctionTest(int p) {
+  MOZ_ASSERT(p = 1); // expected-error {{Forbidden assignment in assert expression}}
+}
+
+void FunctionTest2(int p) {
+  MOZ_ASSERT(((p = 1)));  // expected-error {{Forbidden assignment in assert expression}}
+}
+
+void FunctionTest3(int p) {
+  MOZ_ASSERT(p != 3);
+}
+
+class TestOverloading {
+  int value;
+public:
+  explicit TestOverloading(int _val) : value(_val) {}
+  // different operators
+  explicit operator bool() const { return true; }
+  TestOverloading& operator=(const int _val) { value = _val; return *this; }
+
+  int& GetInt() {return value;}
+};
+
+void TestOverloadingFunc() {
+  TestOverloading p(2);
+  int f;
+
+  MOZ_ASSERT(p);
+  MOZ_ASSERT(p = 3); // expected-error {{Forbidden assignment in assert expression}}
+  MOZ_ASSERT(p, "p is not valid");
+  MOZ_ASSERT(p = 3, "p different than 3"); // expected-error {{Forbidden assignment in assert expression}}
+  MOZ_ASSERT(p.GetInt() = 2); // expected-error {{Forbidden assignment in assert expression}}
+  MOZ_ASSERT(p.GetInt() == 2);
+  MOZ_ASSERT(p.GetInt() == 2, f = 3);
+}

build/clang-plugin/tests/moz.build

@@ -8,6 +8,7 @@
 Library('clang-plugin-tests')
 
 SOURCES += [
+    'TestAssertWithAssignment.cpp',
     'TestBadImplicitConversionCtor.cpp',
     'TestCustomHeap.cpp',
     'TestExplicitOperatorBool.cpp',