Bug 927430 - Add a static analysis to detect is-NaN testing using |x == x| or |x != x|; r=jrmuizel

--HG--
extra : rebase_source : d80c866ecc7e5f415c83db757e82bd4bac299b67

build/clang-plugin/clang-plugin.cpp

@@ -69,11 +69,17 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class NaNExprChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker stackClassChecker;
   ScopeChecker globalClassChecker;
   NonHeapClassChecker nonheapClassChecker;
   ArithmeticArgChecker arithmeticArgChecker;
   TrivialCtorDtorChecker trivialCtorDtorChecker;
+  NaNExprChecker nanExprChecker;
   MatchFinder astMatcher;
 };
 
@@ -416,6 +422,39 @@ AST_MATCHER(UnaryOperator, unaryArithmeticOperator) {
          opcode == UO_Minus ||
          opcode == UO_Not;
 }
+
+/// This matcher will match == and != binary operators.
+AST_MATCHER(BinaryOperator, binaryEqualityOperator) {
+  BinaryOperatorKind opcode = Node.getOpcode();
+  return opcode == BO_EQ || opcode == BO_NE;
+}
+
+/// This matcher will match floating point types.
+AST_MATCHER(QualType, isFloat) {
+  return Node->isRealFloatingType();
+}
+
+/// This matcher will match locations in system headers.  This is adopted from
+/// isExpansionInSystemHeader in newer clangs, but modified in order to work
+/// with old clangs that we use on infra.
+AST_MATCHER(BinaryOperator, isInSystemHeader) {
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
+  if (ExpansionLoc.isInvalid()) {
+    return false;
+  }
+  return SourceManager.isInSystemHeader(ExpansionLoc);
+}
+
+/// This matcher will match locations in SkScalar.h.  This header contains a
+/// known NaN-testing expression which we would like to whitelist.
+AST_MATCHER(BinaryOperator, isInSkScalarDotH) {
+  SourceLocation Loc = Node.getOperatorLoc();
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  SmallString<1024> FileName = SourceManager.getFilename(Loc);
+  return llvm::sys::path::rbegin(FileName)->equals("SkScalar.h");
+}
+
 }
 }
 
@@ -499,6 +538,13 @@ DiagnosticsMatcher::DiagnosticsMatcher()
 
   astMatcher.addMatcher(recordDecl(hasTrivialCtorDtor()).bind("node"),
     &trivialCtorDtorChecker);
+
+  astMatcher.addMatcher(binaryOperator(allOf(binaryEqualityOperator(),
+          hasLHS(has(declRefExpr(hasType(qualType((isFloat())))).bind("lhs"))),
+          hasRHS(has(declRefExpr(hasType(qualType((isFloat())))).bind("rhs"))),
+          unless(anyOf(isInSystemHeader(), isInSkScalarDotH()))
+      )).bind("node"),
+    &nanExprChecker);
 }
 
 void DiagnosticsMatcher::ScopeChecker::run(
@@ -648,6 +694,42 @@ void DiagnosticsMatcher::TrivialCtorDtorChecker::run(
     Diag.Report(node->getLocStart(), errorID) << node;
 }
 
+void DiagnosticsMatcher::NaNExprChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  if (!Result.Context->getLangOpts().CPlusPlus) {
+    // mozilla::IsNaN is not usable in C, so there is no point in issuing these warnings.
+    return;
+  }
+
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned errorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "comparing a floating point value to itself for NaN checking can lead to incorrect results");
+  unsigned noteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "consider using mozilla::IsNaN instead");
+  const BinaryOperator *expr = Result.Nodes.getNodeAs<BinaryOperator>("node");
+  const DeclRefExpr *lhs = Result.Nodes.getNodeAs<DeclRefExpr>("lhs");
+  const DeclRefExpr *rhs = Result.Nodes.getNodeAs<DeclRefExpr>("rhs");
+  const ImplicitCastExpr *lhsExpr = dyn_cast<ImplicitCastExpr>(expr->getLHS());
+  const ImplicitCastExpr *rhsExpr = dyn_cast<ImplicitCastExpr>(expr->getRHS());
+  // The AST subtree that we are looking for will look like this:
+  // -BinaryOperator ==/!=
+  //  |-ImplicitCastExpr LValueToRValue
+  //  | |-DeclRefExpr
+  //  |-ImplicitCastExpr LValueToRValue
+  //    |-DeclRefExpr
+  // The check below ensures that we are dealing with the correct AST subtree shape, and
+  // also that both of the found DeclRefExpr's point to the same declaration.
+  if (lhs->getFoundDecl() == rhs->getFoundDecl() &&
+      lhsExpr && rhsExpr &&
+      std::distance(lhsExpr->child_begin(), lhsExpr->child_end()) == 1 &&
+      std::distance(rhsExpr->child_begin(), rhsExpr->child_end()) == 1 &&
+      *lhsExpr->child_begin() == lhs &&
+      *rhsExpr->child_begin() == rhs) {
+    Diag.Report(expr->getLocStart(), errorID);
+    Diag.Report(expr->getLocStart(), noteID);
+  }
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {

build/clang-plugin/tests/Makefile.in

@@ -4,6 +4,7 @@
 
 # Build without any warning flags, and with clang verify flag for a
 # syntax-only build (no codegen).
+OS_CFLAGS := $(filter-out -W%,$(OS_CFLAGS)) -fsyntax-only -Xclang -verify
 OS_CXXFLAGS := $(filter-out -W%,$(OS_CXXFLAGS)) -fsyntax-only -Xclang -verify
 
 include $(topsrcdir)/config/rules.mk

build/clang-plugin/tests/TestNANTestingExpr.cpp

@@ -0,0 +1,16 @@
+void test(bool x);
+void foo() {
+  float f, f2;
+  typedef double mydouble;
+  mydouble d;
+  double d2;
+  test(f == f); // expected-error{{comparing a floating point value to itself for NaN checking can lead to incorrect results}} expected-note{{consider using mozilla::IsNaN instead}}
+  test(d == d); // expected-error{{comparing a floating point value to itself for NaN checking can lead to incorrect results}} expected-note{{consider using mozilla::IsNaN instead}}
+  test(f != f); // expected-error{{comparing a floating point value to itself for NaN checking can lead to incorrect results}} expected-note{{consider using mozilla::IsNaN instead}}
+  test(d != d); // expected-error{{comparing a floating point value to itself for NaN checking can lead to incorrect results}} expected-note{{consider using mozilla::IsNaN instead}}
+  test(f != d);
+  test(d == (d - f));
+  test(f == f2);
+  test(d == d2);
+  test(d + 1 == d);
+}

build/clang-plugin/tests/TestNANTestingExprC.c

@@ -0,0 +1,17 @@
+/* expected-no-diagnostics */
+void test(int x);
+void foo() {
+  float f, f2;
+  typedef double mydouble;
+  mydouble d;
+  double d2;
+  test(f == f);
+  test(d == d);
+  test(f != f);
+  test(d != d);
+  test(f != d);
+  test(d == (d - f));
+  test(f == f2);
+  test(d == d2);
+  test(d + 1 == d);
+}

build/clang-plugin/tests/moz.build

@@ -9,6 +9,8 @@ SOURCES += [
     'TestCustomHeap.cpp',
     'TestGlobalClass.cpp',
     'TestMustOverride.cpp',
+    'TestNANTestingExpr.cpp',
+    'TestNANTestingExprC.c',
     'TestNoArithmeticExprInArgument.cpp',
     'TestNonHeapClass.cpp',
     'TestStackClass.cpp',