[Analyzer] Bugfix for CheckerRegistry

[baloghadamsoftware]

Summary

CheckerRegistry registers a checker either if it is excplicitly enabled or it is a dependency of an explicitly enabled checker and is not explicitly disabled. In both cases it is also important that the checker should be registered (shoudRegisterXXX() returns true).

Currently there is a bug here: if the dependenct checker is not explicitly disabled it is registered regardless of whether it should be registered. This patch fixes this bug.

[xazax.hun]

If we disable the dependency of a checker explicitly I think we should at least emit a warning.

[baloghadamsoftware]

In D75842#1912246, @xazax.hun wrote:

If we disable the dependency of a checker explicitly I think we should at least emit a warning.

I agree, but I do not thik that this should be part of this patch. This is only a small bugfix.

[baloghadamsoftware]

I looked in the unittests directory but could not find tests for dependency handling. @Szelethus, feel free to commandeer this patch if you know how to test it.

[Szelethus]

In D75842#1912249, @baloghadamsoftware wrote:

In D75842#1912246, @xazax.hun wrote:

If we disable the dependency of a checker explicitly I think we should at least emit a warning.

I agree, but I do not thik that this should be part of this patch. This is only a small bugfix.

Oof. We wanted to do that for a long time but there were compatibility concerns -- in any case, we totally could emit an error at least when -analyzer-config-compatibility-mode is false and for newer checkers and a warning for older ones.

I distinctively remember a lot of concerns being raised around this. @dcoughlin, @NoQ, how does what I suggested here sound like?

In D75842#1912250, @baloghadamsoftware wrote:

I looked in the unittests directory but could not find tests for dependency handling. @Szelethus, feel free to commandeer this patch if you know how to test it.

This should be the thing you're looking for: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp. It might be a good idea to wait for me to land D67335 first, though :)

[xazax.hun]

I am not sure if I would call this a bugfix. Enabling a checker without one of its dependency will potentially cause the checker to misbehave. I am uncomfortable changing the current behavior to a more dangerous one without any diagnostics. Including the diagnostic with this patch should not make this too big.

[baloghadamsoftware]

In D75842#1912293, @xazax.hun wrote:

I am not sure if I would call this a bugfix. Enabling a checker without one of its dependency will potentially cause the checker to misbehave. I am uncomfortable changing the current behavior to a more dangerous one without any diagnostics. Including the diagnostic with this patch should not make this too big.

This is a bugfix. The current behavior is wrong due to a mistake. A checker is registered which should not and this may cause crashes.

[xazax.hun]

In D75842#1912364, @baloghadamsoftware wrote:

In D75842#1912293, @xazax.hun wrote:

I am not sure if I would call this a bugfix. Enabling a checker without one of its dependency will potentially cause the checker to misbehave. I am uncomfortable changing the current behavior to a more dangerous one without any diagnostics. Including the diagnostic with this patch should not make this too big.

This is a bugfix. The current behavior is wrong due to a mistake. A checker is registered which should not and this may cause crashes.

Oh, sorry. I misunderstood the description. My bad. In this case I am ok with having the diagnostic in a separate patch.

[Szelethus]

The thing to note here is checkers that are isDisabled() won't get enabled, only if they are isEnabled() as well :)

Please add a unit test, otherwise LGTM. I just landed D67335, so it should be easy to do.

[baloghadamsoftware]

Test added.

[Szelethus]

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

85

How about Unsatisfied checker dependency.?

92

Isn't this overkill? The test above test only the output, but don't outright cause a crash.

111–112

Same thing.

143

I don't think this is checking what you're looking for -- the test should be whether Diag is an empty string, while runCheckerOnCode returns true when the tool (the static analyzer, in this case) terminates successfully, even if it doesn't work the way we expect it to.

[baloghadamsoftware]

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

143

There could be hundreds of alternative approaches, but this test exactly simulates the real-world problem: the checker crashes because it should not be registered. Of course, I tried the test without the bugfix and it fails as it should because the tool terminates unsuccessfully if the prerequisite checker is registered.

[Szelethus]

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

143

This is still confusing. Please check the string, that should contain what you need and nothing else, and the asserts could be removed as a result -- it shouldn't be more then 5 lines.

[baloghadamsoftware]

Updated according to the comments.

[Szelethus]

LGTM, thanks!

clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h

struct CheckerInfo {
 
     ConstCheckerInfoList Dependencies;
 
     bool isEnabled(const LangOptions &LO) const {
       return State == StateFromCmdLine::State_Enabled && ShouldRegister(LO);
     }
 
     bool isDisabled(const LangOptions &LO) const {
-      return State == StateFromCmdLine::State_Disabled && ShouldRegister(LO);
+      return State == StateFromCmdLine::State_Disabled || !ShouldRegister(LO);
     }
 
     // Since each checker must have a different full name, we can identify
     // CheckerInfo objects by them.
     bool operator==(const CheckerInfo &Rhs) const {
       return FullName == Rhs.FullName;
     }
 

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

void addLocIncDecChecker(AnalysisASTConsumer &AnalysisConsumer,
   });
 }
 
 TEST(RegisterCustomCheckers, CheckLocationIncDec) {
   EXPECT_TRUE(
       runCheckerOnCode<addLocIncDecChecker>("void f() { int *p; (*p)++; }"));
 }
 
+//===----------------------------------------------------------------------===//
+// Unsatisfied checker dependency
+//===----------------------------------------------------------------------===//
+
+class PrerequisiteChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const {
+    BR.EmitBasicReport(D, this, "Prerequisite", categories::LogicError,
+                       "This is the prerequisite checker",
+                       PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+  }
+};
+
+void registerPrerequisiteChecker(CheckerManager &mgr) {
+  mgr.registerChecker<PrerequisiteChecker>();
+}
+
+bool shouldRegisterPrerequisiteChecker(const LangOptions &LO) {
+  return false;
+}
+
+class DependentChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const {
+    BR.EmitBasicReport(D, this, "Dependent", categories::LogicError,
+                       "This is the Dependent Checker",
+                       PathDiagnosticLocation(D, Mgr.getSourceManager()), {});
+  }
+};
+
+void registerDependentChecker(CheckerManager &mgr) {
+  mgr.registerChecker<DependentChecker>();
+}
+
+bool shouldRegisterDependentChecker(const LangOptions &LO) {
+  return true;
+}
+
+void addDependentChecker(AnalysisASTConsumer &AnalysisConsumer,
+                         AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"custom.Dependent", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+     Registry.addChecker(registerPrerequisiteChecker,
+                         shouldRegisterPrerequisiteChecker,
+                         "custom.Prerequisite", "Description", "", false);
+     Registry.addChecker(registerDependentChecker,
+                         shouldRegisterDependentChecker,
+                         "custom.Dependent", "Description", "", false);
+     Registry.addDependency("custom.Dependent", "custom.Prerequisite");
+    });
+}
+
+TEST(RegisterDependentCheckers, RegisterChecker) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addDependentChecker>("void f() {;}", Diags));
+  EXPECT_EQ(Diags, "");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang