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

[steakhal]

Summary

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.

[martong]

Thanks, LGTM! (with nits).

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

679

Optional implicitly converts to bool and IMHO this way it is easier to read the code.

680

clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp

58

Let's hope this will not break windows and other exotic build bots.

[steakhal]

Committed with the requested changes.

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

679

I guess it's up to personal taste.
I'm okay either way, but I more frequently stick to the explicit way of checking this.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

 }
 
 LLVM_DUMP_METHOD void CallEvent::dump() const { dump(llvm::errs()); }
 
 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;
   }
 
   if (const Decl *D = getDecl()) {
     Out << "Call to ";
     D->print(Out, Ctx.getPrintingPolicy());
     return;
   }

clang/lib/StaticAnalyzer/Core/CheckerManager.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include <cassert>
 #include <vector>
 
 using namespace clang;
 using namespace ento;
 
 bool CheckerManager::hasPathSensitiveCheckers() const {
   const auto IfAnyAreNonEmpty = [](const auto &... Callbacks) -> bool {

 /// Run checkers for evaluating a call.
 /// Only one checker will evaluate the call.
 void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst,
                                             const ExplodedNodeSet &Src,
                                             const CallEvent &Call,
                                             ExprEngine &Eng,
                                             const EvalCallOptions &CallOpts) {
   for (auto *const Pred : Src) {
-    bool anyEvaluated = false;
+    Optional<CheckerNameRef> evaluatorChecker;
 
     ExplodedNodeSet checkDst;
     NodeBuilder B(Pred, checkDst, Eng.getBuilderContext());
 
     // Check if any of the EvalCall callbacks can evaluate the call.
     for (const auto &EvalCallChecker : EvalCallCheckers) {
       // TODO: Support the situation when the call doesn't correspond
       // to any Expr.
       ProgramPoint L = ProgramPoint::getProgramPoint(
           Call.getOriginExpr(), ProgramPoint::PostStmtKind,
           Pred->getLocationContext(), EvalCallChecker.Checker);
       bool evaluated = false;
       { // CheckerContext generates transitions(populates checkDest) on
         // destruction, so introduce the scope to make sure it gets properly
         // populated.
         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.
 #endif
       }
     }
 
     // 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);
     }
   }
 }
 
 /// Run checkers for the entire Translation Unit.
 void CheckerManager::runCheckersOnEndOfTranslationUnit(

clang/unittests/StaticAnalyzer/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
   BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
+  ConflictingEvalCallsTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
   NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp
   StoreTest.cpp
   SymbolReaperTest.cpp
   SValTest.cpp

clang/unittests/StaticAnalyzer/ConflictingEvalCallsTest.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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);
+}