[clang] 9b5c9c4 - [analyzer] Dump checker name if multiple checkers evaluate the same call

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2021-11-02T14:42:14+01:00
New Revision: 9b5c9c469d90227251d9da9108ee7985ba415f2b

URL: https://github.com/llvm/llvm-project/commit/9b5c9c469d90227251d9da9108ee7985ba415f2b
DIFF: https://github.com/llvm/llvm-project/commit/9b5c9c469d90227251d9da9108ee7985ba415f2b.diff

LOG: [analyzer] Dump checker name if multiple checkers evaluate the same call

Previously, if accidentally multiple checkers `eval::Call`-ed the same
`CallEvent`, in debug builds the analyzer detected this and crashed
with the message stating this. Unfortunately, the message did not state
the offending checkers violating this invariant.
This revision addresses this by printing a more descriptive message
before aborting.

Reviewed By: martong

Differential Revision: https://reviews.llvm.org/D112889

Added: 
    clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp

Modified: 
    clang/lib/StaticAnalyzer/Core/CallEvent.cpp
    clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
    clang/unittests/StaticAnalyzer/CMakeLists.txt

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 180fa970a3543..43ffcc8f13174 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -411,7 +411,6 @@ void CallEvent::dump(raw_ostream &Out) const {
   ASTContext &Ctx = getState()->getStateManager().getContext();
   if (const Expr *E = getOriginExpr()) {
     E->printPretty(Out, nullptr, Ctx.getPrintingPolicy());
-    Out << "\n";
     return;
   }
 

diff  --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index e09399a83589e..94287b7992dd9 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -26,6 +26,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include <cassert>
 #include <vector>
 
@@ -655,7 +656,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
                                             ExprEngine &Eng,
                                             const EvalCallOptions &CallOpts) {
   for (auto *const Pred : Src) {
-    bool anyEvaluated = false;
+    Optional<CheckerNameRef> evaluatorChecker;
 
     ExplodedNodeSet checkDst;
     NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());
@@ -674,10 +675,26 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
         CheckerContext C(B, Eng, Pred, L);
         evaluated = EvalCallChecker(Call, C);
       }
-      assert(!(evaluated && anyEvaluated)
-             && "There are more than one checkers evaluating the call");
+#ifndef NDEBUG
+      if (evaluated && evaluatorChecker) {
+        const auto toString = [](const CallEvent &Call) -> std::string {
+          std::string Buf;
+          llvm::raw_string_ostream OS(Buf);
+          Call.dump(OS);
+          OS.flush();
+          return Buf;
+        };
+        std::string AssertionMessage = llvm::formatv(
+            "The '{0}' call has been already evaluated by the {1} checker, "
+            "while the {2} checker also tried to evaluate the same call. At "
+            "most one checker supposed to evaluate a call.",
+            toString(Call), evaluatorChecker->getName(),
+            EvalCallChecker.Checker->getCheckerName());
+        llvm_unreachable(AssertionMessage.c_str());
+      }
+#endif
       if (evaluated) {
-        anyEvaluated = true;
+        evaluatorChecker = EvalCallChecker.Checker->getCheckerName();
         Dst.insert(checkDst);
 #ifdef NDEBUG
         break; // on release don't check that no other checker also evals.
@@ -686,7 +703,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
     }
 
     // If none of the checkers evaluated the call, ask ExprEngine to handle it.
-    if (!anyEvaluated) {
+    if (!evaluatorChecker) {
       NodeBuilder B(Pred, Dst, Eng.getBuilderContext());
       Eng.defaultEvalCall(B, Pred, Call, CallOpts);
     }

diff  --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 985edf4db3408..810cf75400d79 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -8,6 +8,7 @@ add_clang_unittest(StaticAnalysisTests
   BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
+  ConflictingEvalCallsTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
   NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp

diff  --git a/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp b/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
new file mode 100644
index 0000000000000..405a59ffab1b3
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp
@@ -0,0 +1,58 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class EvalCallBase : public Checker<eval::Call> {
+  const CallDescription Foo = {"foo", 0};
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+    return Call.isCalled(Foo);
+  }
+};
+
+class EvalCallFoo1 : public EvalCallBase {};
+class EvalCallFoo2 : public EvalCallBase {};
+void addEvalFooCheckers(AnalysisASTConsumer &AnalysisConsumer,
+                        AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.EvalFoo1", true},
+                                {"test.EvalFoo2", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<EvalCallFoo1>("test.EvalFoo1", "EmptyDescription",
+                                      "EmptyDocsUri");
+    Registry.addChecker<EvalCallFoo2>("test.EvalFoo2", "EmptyDescription",
+                                      "EmptyDocsUri");
+  });
+}
+} // namespace
+
+TEST(EvalCall, DetectConflictingEvalCalls) {
+#ifdef NDEBUG
+  GTEST_SKIP() << "This test is only available for debug builds.";
+#endif
+  const std::string Code = R"code(
+    void foo();
+    void top() {
+      foo(); // crash
+    }
+  )code";
+  constexpr auto Regex =
+      "The 'foo\\(\\)' call has been already evaluated by the test\\.EvalFoo1 "
+      "checker, while the test\\.EvalFoo2 checker also tried to evaluate the "
+      "same call\\. At most one checker supposed to evaluate a call\\.";
+  ASSERT_DEATH(runCheckerOnCode<addEvalFooCheckers>(Code), Regex);
+}