Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression. r?mystor draft
authorAndi-Bogdan Postelnicu <bpostelnicu@mozilla.com>
Tue, 19 Jul 2016 09:59:22 +0300
changeset 389381 d05a22fa3d4cc386b795f6d2a357ffbc4029ebf9
parent 388900 0fbdcd21fad76a00328e67875c6f40dc219235f4
child 525742 4d1f97fcd3abd2458c3dcd93aba46456a271fb6f
push id23392
push userbmo:bpostelnicu@mozilla.com
push dateTue, 19 Jul 2016 06:59:52 +0000
reviewersmystor
bugs1283395
milestone50.0a1
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression. r?mystor MozReview-Commit-ID: AybStmi6MIH
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestAssertWithAssignment.cpp
build/clang-plugin/tests/moz.build
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -34,16 +34,42 @@ typedef ASTConsumer *ASTConsumerPtr;
 // ones for compatibility with older versions.
 #define cxxConstructExpr constructExpr
 #define cxxConstructorDecl constructorDecl
 #define cxxMethodDecl methodDecl
 #define cxxNewExpr newExpr
 #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;
 class DiagnosticsMatcher {
 public:
   DiagnosticsMatcher();
 
   ASTConsumerPtr makeASTConsumer() { return astMatcher.newASTConsumer(); }
@@ -119,31 +145,37 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class RefCountedCopyConstructorChecker : public MatchFinder::MatchCallback {
   public:
     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;
   NaNExprChecker nanExprChecker;
   NoAddRefReleaseOnReturnChecker noAddRefReleaseOnReturnChecker;
   RefCountedInsideLambdaChecker refCountedInsideLambdaChecker;
   ExplicitOperatorBoolChecker explicitOperatorBoolChecker;
   NoDuplicateRefCntMemberChecker noDuplicateRefCntMemberChecker;
   NeedsNoVTableTypeChecker needsNoVTableTypeChecker;
   NonMemMovableTemplateArgChecker nonMemMovableTemplateArgChecker;
   NonMemMovableMemberChecker nonMemMovableMemberChecker;
   ExplicitImplicitChecker explicitImplicitChecker;
   NoAutoTypeChecker noAutoTypeChecker;
   NoExplicitMoveConstructorChecker noExplicitMoveConstructorChecker;
   RefCountedCopyConstructorChecker refCountedCopyConstructorChecker;
+  AssertAssignmentChecker assertAttributionChecker;
   MatchFinder astMatcher;
 };
 
 namespace {
 
 std::string getDeclarationNamespace(const Decl *decl) {
   const DeclContext *DC =
       decl->getDeclContext()->getEnclosingNamespaceContext();
@@ -757,16 +789,25 @@ AST_MATCHER(QualType, autoNonAutoableTyp
 
 AST_MATCHER(CXXConstructorDecl, isExplicitMoveConstructor) {
   return Node.isExplicit() && Node.isMoveConstructor();
 }
 
 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;
+}
 }
 }
 
 namespace {
 
 void CustomTypeAnnotation::dumpAnnotationReason(DiagnosticsEngine &Diag,
                                                 QualType T,
                                                 SourceLocation Loc) {
@@ -1065,16 +1106,20 @@ DiagnosticsMatcher::DiagnosticsMatcher()
       &noExplicitMoveConstructorChecker);
 
   astMatcher.addMatcher(
       cxxConstructExpr(
           hasDeclaration(cxxConstructorDecl(isCompilerProvidedCopyConstructor(),
                                             ofClass(hasRefCntMember()))))
           .bind("node"),
       &refCountedCopyConstructorChecker);
+
+  astMatcher.addMatcher(
+      callExpr(isAssertAssignmentTestFunc()).bind("funcCall"),
+      &assertAttributionChecker);
 }
 
 // These enum variants determine whether an allocation has occured in the code.
 enum AllocationVariety {
   AV_None,
   AV_Global,
   AV_Automatic,
   AV_Temporary,
@@ -1540,16 +1585,28 @@ void DiagnosticsMatcher::RefCountedCopyC
   // Everything we needed to know was checked in the matcher - we just report
   // the error here
   const CXXConstructExpr *E = Result.Nodes.getNodeAs<CXXConstructExpr>("node");
 
   Diag.Report(E->getLocation(), ErrorID);
   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,
                                    StringRef fileName) override {
 #if CLANG_VERSION_FULL >= 306
     std::unique_ptr<MozChecker> checker(llvm::make_unique<MozChecker>(CI));
     ASTConsumerPtr other(checker->getOtherConsumer());
 
new file mode 100644
--- /dev/null
+++ b/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);
+}
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -3,16 +3,17 @@
 # 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/.
 
 # dummy library name to avoid skipping building the sources here.
 Library('clang-plugin-tests')
 
 SOURCES += [
+    'TestAssertWithAssignment.cpp',
     'TestBadImplicitConversionCtor.cpp',
     'TestCustomHeap.cpp',
     'TestExplicitOperatorBool.cpp',
     'TestGlobalClass.cpp',
     'TestHeapClass.cpp',
     'TestInheritTypeAnnotationsFromTemplateArgs.cpp',
     'TestMultipleAnnotations.cpp',
     'TestMustOverride.cpp',