[Analyzer] Fix for incorrect use of container and iterator checkers

[baloghadamsoftware]

Summary

Iterator checkers (and planned container checkers) need the option aggressive-binary-operation-simplification to be enabled. Without this option they may cause assertions. To prevent such misuse, this patch adds a preventive check which issues a warning and denies the registartion of the checker if this option is disabled.

[whisperity]

Do you have access to the Driver somehow? Instead of a cerr (-ish) output, something that is formatted like a "true" error diagnostic (or warning), such as "no such file or directory" would be much better, I fear this [ERROR] will simply be flooded away with the usual diagnostic output on screen, especially if multiple files are concerned.

Are you trying to land this before 10.0 goes out? In this case, it could be okay, but for the long term, this seems a very crude solution.

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

1037–1038

In Clang diagnostics, "identifiers" are put between apostrophes:

'aggressive-binary-operation-simplification'

[whisperity]

Also, errors should conceptually break the operation at hand, so this thing should be a warning instead?

[Szelethus]

Im sorry, i though we talked about internal code and we're stuck with the checker interface -- this should definitely be solved by changing the parameter of the shouldRegister function to CheckerManager. If you want to emit errors for incorrect file, there is an example in UninitializedObjectChecker and maybe in GenericTaintChecker.

[baloghadamsoftware]

In D75171#1893811, @Szelethus wrote:

Im sorry, i though we talked about internal code and we're stuck with the checker interface -- this should definitely be solved by changing the parameter of the shouldRegister function to CheckerManager. If you want to emit errors for incorrect file, there is an example in UninitializedObjectChecker and maybe in GenericTaintChecker.

Sorry, but it does not work. It is impossible to use CheckerManager as parameter for shouldRegisterXXX() because CheckerRegistry does not have it. If I add CheckerManager to CheckerRegistry then the printXXX() functions will not work because they do not have CheckerManager at all. I tried to add AnalyzerOptions as a second parameter (see D75271), but it does not help in printing error message. I need a way to solve 44998 by rejecting the checker if the option is disabled and printing an error message.

[baloghadamsoftware]

This is the so called "correct" solution. However, it does not even compile because getAnalyzerOptions() and getASTContext() are non-const functions of CheckerManager, but shouldRegisterXXX() functions get it as const reference. Even if I use const_cast (not in this patch, just for testing) it does not work, it does not prevent the crash: when trying to register IteratorModeling which depends on ContainerModeling both checkers are registered after shouldRegisterContainerModeling() function returning false.

[Szelethus]

You seem to have uploaded the wrong diff :)

In D75171#1908259, @baloghadamsoftware wrote:

This is the so called "correct" solution. However, it does not even compile because getAnalyzerOptions() and getASTContext() are non-const functions of CheckerManager, but shouldRegisterXXX() functions get it as const reference.

That doesn't sound too drastic, why don't you make those methods const?

Even if I use const_cast (not in this patch, just for testing) it does not work, it does not prevent the crash: when trying to register IteratorModeling which depends on ContainerModeling both checkers are registered after shouldRegisterContainerModeling() function returning false.

Huh, that sounds interesting. I just tried it locally and it works like a charm. Would you mind me commandeering this patch to demonstrate?

[Szelethus]

Actually, this is the diff:

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index ecd871e36ee..1f2c8c50a01 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -341,6 +341,8 @@ def err_analyzer_checker_option_unknown : Error<
   "checker '%0' has no option called '%1'">;
 def err_analyzer_checker_option_invalid_input : Error<
   "invalid input for checker option '%0', that expects %1">;
+def err_analyzer_checker_incompatible_analyzer_option : Error<
+  "checker cannot be enabled with analyzer option '%0' == %1">;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index a3a85bfac13..680cb92e1ff 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -169,8 +169,8 @@ public:
   void finishedCheckerRegistration();
 
   const LangOptions &getLangOpts() const { return LangOpts; }
-  AnalyzerOptions &getAnalyzerOptions() { return AOptions; }
-  ASTContext &getASTContext() {
+  AnalyzerOptions &getAnalyzerOptions() const { return AOptions; }
+  ASTContext &getASTContext() const {
     assert(Context);
     return *Context;
   }
diff --git a/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
index 16a913e761c..f739417a37b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -1036,5 +1037,15 @@ void ento::registerContainerModeling(CheckerManager &mgr) {
 }
 
 bool ento::shouldRegisterContainerModeling(const CheckerManager &mgr) {
+  if (!mgr.getLangOpts().CPlusPlus)
+    return false;
+
+  if (!mgr.getAnalyzerOptions().ShouldAggressivelySimplifyBinaryOperation) {
+    mgr.getASTContext().getDiagnostics().Report(
+        diag::err_analyzer_checker_incompatible_analyzer_option)
+      << "aggressive-binary-operation-simplification" << "false";
+    return false;
+  }
+
   return true;
 }
diff --git a/clang/test/Analysis/checker-dependencies.c b/clang/test/Analysis/checker-dependencies.c
index 6c8583adb35..54d585561d1 100644
--- a/clang/test/Analysis/checker-dependencies.c
+++ b/clang/test/Analysis/checker-dependencies.c
@@ -18,3 +18,11 @@
 
 // CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCountBase
 // CHECK-IMPLICITLY-DISABLED-NOT: osx.cocoa.RetainCount
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=cplusplus.IteratorModeling \
+// RUN:   -analyzer-config aggressive-binary-operation-simplification=false \
+// RUN:   -analyzer-list-enabled-checkers \
+// RUN:   2>&1 | FileCheck %s -check-prefix=DEPENDENCY-SHOULD-NOT-REGISTER
+
+// DEPENDENCY-SHOULD-NOT-REGISTER: 'aggressive-binary-operation-simplification' == false

[baloghadamsoftware]

Correct diff.

[Szelethus]

Yay, this is very nice!

clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp

2–11

What does this file do that clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp doesn't?

clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp

39–40

Is this where the crash supposed to happen? Can you mark it with // no-crash?

[baloghadamsoftware]

Tests reduced.

[Szelethus]

Yay!

[sylvestre.ledru]

@baloghadamsoftware @Szelethus it would be great to have the name of the checkers in the error message
The error is "error: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false"
and I had to look at this patch to understand that it is about iterator

[Szelethus]

In D75171#1954279, @sylvestre.ledru wrote:

@baloghadamsoftware @Szelethus it would be great to have the name of the checkers in the error message
The error is "error: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false"
and I had to look at this patch to understand that it is about iterator

Huh, that is a fair point -- Adam, can you patch it in please?

[steakhal]

In D75171#1954405, @Szelethus wrote:

In D75171#1954279, @sylvestre.ledru wrote:

@baloghadamsoftware @Szelethus it would be great to have the name of the checkers in the error message
The error is "error: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false"
and I had to look at this patch to understand that it is about iterator

Huh, that is a fair point -- Adam, can you patch it in please?

IMO enabling the checker should imply enabling all the required options as well. So, I see very little value in this error message, not to mention that we won't know which checkers to blame.
It's also really bad for git bisecting, when e.g. someone is simply enabling the alpha checkers. Suddenly, sometimes in the process, you have to pass this checker option, and in some other bisect cases, you must not pass them, because those are not yet recognized.
This happened a couple of times for example reducing crashes.

So, I think we should reiterate this issue.
What's your take on this @Szelethus?

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp

if (Sym.second.getContainer() == Cont)
       return true;
   }
 
   return false;
 }
 
 } // namespace
 
+static bool checkAggressiveSimplificationEnabled(CheckerManager &mgr) {
+  if (!mgr.getAnalyzerOptions().ShouldAggressivelySimplifyBinaryOperation) {
+    llvm::errs() << "[ERROR] Checker " << mgr.getCurrentCheckerName().getName()
+                 << " cannot be enabled without analyzer option aggressive-"
+                    "binary-operation-simplification.\n";
+    return false;
+  }
+  return true;
+}
+
 void ento::registerContainerModeling(CheckerManager &mgr) {
+  if (!checkAggressiveSimplificationEnabled(mgr))
+    return;
+
   mgr.registerChecker<ContainerModeling>();
 }
 
 bool ento::shouldRegisterContainerModeling(const LangOptions &LO) {
-  return true;
+  return LO.CPlusPlus;
 }

clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp

+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,alpha.cplusplus.ContainerModeling\
+// RUN: -analyzer-config c++-container-inlining=false %s 2>&1 | FileCheck %s
+
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,alpha.cplusplus.ContainerModeling\
+// RUN: -analyzer-config c++-container-inlining=true -DINLINE=1 %s 2>&1 |\
+// RUN: FileCheck %s
+
+// CHECK: [ERROR] Checker alpha.cplusplus.ContainerModeling cannot be enabled without analyzer option aggressive-binary-operation-simplification.

clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp

+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection\
+// RUN: -analyzer-config c++-container-inlining=false %s 2>&1 | FileCheck %s
+
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection\
+// RUN: -analyzer-config c++-container-inlining=true -DINLINE=1 %s 2>&1 |\
+// RUN: FileCheck %s
+
+// CHECK: [ERROR] Checker alpha.cplusplus.ContainerModeling cannot be enabled without analyzer option aggressive-binary-operation-simplification.
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+template <typename Container>
+long clang_analyzer_container_end(const Container&);
+template <typename Iterator>
+long clang_analyzer_iterator_position(const Iterator&);
+
+void clang_analyzer_eval(bool);
+
+
+template <typename InputIterator, typename T>
+InputIterator nonStdFind(InputIterator first, InputIterator last,
+                         const T &val) {
+  for (auto i = first; i != last; ++i) {
+    if (*i == val) {
+      return i;
+    }
+  }
+  return last;
+}
+
+void non_std_find(std::vector<int> &V, int e) {
+  auto first = nonStdFind(V.begin(), V.end(), e);
+  clang_analyzer_eval(clang_analyzer_container_end(V) ==
+                      clang_analyzer_iterator_position(first));
+  if (V.end() != first) {
+    clang_analyzer_eval(clang_analyzer_container_end(V) ==
+                        clang_analyzer_iterator_position(first));
+  }
+}