[analyzer] Remove overzealous "No dispatcher registered" assertion

[vabridgers]

Random testing revealed it's possible to crash the analyzer with the command line invocation:

clang -cc1 -analyze -analyzer-checker=nullability empty.c

where the source file, empty.c is an empty source file.

clang: <root>/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:56:
   void clang::ento::CheckerManager::finishedCheckerRegistration():
     Assertion `Event.second.HasDispatcher && "No dispatcher registered for an event"' failed.

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/

Stack dump:
0.      Program arguments: clang -cc1 -analyze -analyzer-checker=nullability nullability-nocrash.c
 #0 ...
 ...
 #7 <addr> clang::ento::CheckerManager::finishedCheckerRegistration()
 #8 <addr> clang::ento::CheckerManager::CheckerManager(clang::ASTContext&,
             clang::AnalyzerOptions&, clang::Preprocessor const&,
             llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>,
             std::allocator<char>>>, llvm::ArrayRef<std::function<void (clang::ento::CheckerRegistry&)>>)

This commit removes the assertion which failed here, because it was logically incorrect: it required that if an Event is handled by some (enabled) checker, then there must be an enabled checker which can emit that kind of Event. It should be OK to disable the event-producing checkers but enable an event-consuming checker which has different responsibilities in addition to handling the events.

Note that this assertion was in an #ifndef NDEBUG block, so this change does not impact the non-debug builds.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: None (vabridgers)

Changes

Random testing revealed it's possible to crash the analyzer through a rare command line invocation:

clang -cc1 -analyze -analyzer-checker=nullability empty.c

where the source file, empty.c is an empty source file. This change simply registers the ImplictNullDeref Event Dispatcher as is done in other similar checks to avoid the crash.

clang: <root>/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:56:
void clang::ento::CheckerManager::finishedCheckerRegistration():
Assertion `Event.second.HasDispatcher && "No dispatcher registered for an event"' failed.

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/

Stack dump:
0. Program arguments: clang -cc1 -analyze -analyzer-checker=nullability nullability-nocrash.c
#0 ...
...
#7 <addr> clang::ento::CheckerManager::finishedCheckerRegistration()
#8 <addr> clang::ento::CheckerManager::CheckerManager(clang::ASTContext&,
clang::AnalyzerOptions&, clang::Preprocessor const&,
llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char>>>, llvm::ArrayRef<std::function<void (clang::ento::CheckerRegistry&)>>)

---

Full diff: https://github.com/llvm/llvm-project/pull/107294.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (+1)
• (added) clang/test/Analysis/nullability-nocrash.c (+4)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 04472bb3895a78..d75dcb5a8cd644 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -84,6 +84,7 @@ class NullabilityChecker
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
                      check::PostObjCMessage, check::DeadSymbols, eval::Assume,
                      check::Location, check::Event<ImplicitNullDerefEvent>,
+                     /*EventDispatcher<ImplicitNullDerefEvent>,*/
                      check::BeginFunction> {
 
 public:
diff --git a/clang/test/Analysis/nullability-nocrash.c b/clang/test/Analysis/nullability-nocrash.c
new file mode 100644
index 00000000000000..4102a4fd3a846f
--- /dev/null
+++ b/clang/test/Analysis/nullability-nocrash.c
@@ -0,0 +1,4 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=nullability \
+// RUN:                       -analyzer-output=text -verify %s
+//
+// expected-no-diagnostics

[NagyDonat]

I investigated this situation and I found that this crash is not limited to empty source files -- I'd guess that the analyzer would crash on any input if it's executed as

// RUN: %clang_analyze_cc1 -w -analyzer-checker=nullability \
// RUN:                       -analyzer-output=text -verify %s

The real reason why this crash is rare is that two core checkers (core.NullDereference and core.NonNullParamChecker) are derived from EventDispatcher<ImplicitNullDerefEvent> so the assertion is not triggered in the "normal" case when the core checkers are enabled.

Note that the documentation of core checkers says that "These checkers must be always switched on as other checker rely on them."; however we should still eliminate this assertion failure because it's ugly.

Registering NullabilityChecker as an EventDispatcher (which happens to never dispatch any events) definitely works, but I think it would be more elegant to simply remove the assertion that caused the crash. Registering a handler for an event which cannot be emitted (under the current unusual config) is not an error, it should not trigger an assertion failure.

[vabridgers]

Thanks @NagyDonat , in that case I'll just remove the assert. I don't think we need a backing LIT test case if we choose that solution. What do you think?

[NagyDonat]

Hmm, I see that this assertion was hidden behind an #ifndef NDEBUG so it is only active within debug builds of the analyzer.

Nevertheless, I still think that it's better to remove it, because

1. it does not offer significant protection against introducing bugs (it's unlikely that somebody would develop an event handler without writing a corresponding event dispatcher);
2. it can cause trouble in test files where it's often good to enable a very minimal set of checkers.

[NagyDonat]

@vabridgers I rewrote the title and description of this PR to describe the current approach. Feel free to adjust (or partially restore) it if you'd prefer a different phrasing.

[vabridgers]

Thanks @NagyDonat I'll update.

[NagyDonat]

Looks good to me :)