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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.
Herald added a subscriber: manas.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh, Ádám via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGafcb77cc88a2: [Analyzer] Fix for incorrect use of container 
and iterator checkers (authored by baloghadamsoftware).
Herald added a subscriber: ASDenysPetrov.

Changed prior to commit:
  https://reviews.llvm.org/D75171?vs=249352=253506#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
  
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
  clang/test/Analysis/loop-widening-notes.cpp


Index: clang/test/Analysis/loop-widening-notes.cpp
===
--- clang/test/Analysis/loop-widening-notes.cpp
+++ clang/test/Analysis/loop-widening-notes.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha -analyzer-max-loop 2 
-analyzer-config widen-loops=true -analyzer-output=text -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core 
-analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text 
-verify -analyzer-config eagerly-assume=false %s
 
 int *p_a;
 int bar();
Index: 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection\
+// RUN: %s 2>&1 | FileCheck %s
+
+// XFAIL: *
+
+// CHECK: checker cannot be enabled with analyzer option 
'aggressive-binary-operation-simplification' == false
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+
+void comparison(std::vector ) {
+  clang_analyzer_eval(V.begin() == V.end()); // no-crash
+}
Index: 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,alpha.cplusplus.ContainerModeling\
+// RUN: %s 2>&1 | FileCheck %s
+
+// XFAIL: *
+
+// CHECK: checker cannot be enabled with analyzer option 
'aggressive-binary-operation-simplification' == false
Index: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ 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"
@@ -1068,5 +1069,15 @@
 }
 
 bool ento::shouldRegisterContainerModeling(const CheckerManager ) {
+  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;
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -344,6 +344,8 @@
   "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">;


Index: clang/test/Analysis/loop-widening-notes.cpp
===
--- clang/test/Analysis/loop-widening-notes.cpp
+++ clang/test/Analysis/loop-widening-notes.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha -analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Yay!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 249352.
baloghadamsoftware added a comment.

Tests reduced.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
  
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
  clang/test/Analysis/loop-widening-notes.cpp


Index: clang/test/Analysis/loop-widening-notes.cpp
===
--- clang/test/Analysis/loop-widening-notes.cpp
+++ clang/test/Analysis/loop-widening-notes.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha -analyzer-max-loop 2 
-analyzer-config widen-loops=true -analyzer-output=text -verify 
-analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core 
-analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text 
-verify -analyzer-config eagerly-assume=false %s
 
 int *p_a;
 int bar();
Index: 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection\
+// RUN: %s 2>&1 | FileCheck %s
+
+// CHECK: checker cannot be enabled with analyzer option 
'aggressive-binary-operation-simplification' == false
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+
+void comparison(std::vector ) {
+  clang_analyzer_eval(V.begin() == V.end()); // no-crash
+}
Index: 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,alpha.cplusplus.ContainerModeling\
+// RUN: %s 2>&1 | FileCheck %s
+
+// CHECK: checker cannot be enabled with analyzer option 
'aggressive-binary-operation-simplification' == false
Index: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ 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 @@
 }
 
 bool ento::shouldRegisterContainerModeling(const CheckerManager ) {
+  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;
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -344,6 +344,8 @@
   "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">;


Index: clang/test/Analysis/loop-widening-notes.cpp
===
--- clang/test/Analysis/loop-widening-notes.cpp
+++ clang/test/Analysis/loop-widening-notes.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha -analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text -verify -analyzer-config eagerly-assume=false %s
 
 int *p_a;
 int bar();
Index: clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Yay, this is very nice!




Comment at: 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp:1-10
+// 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 |\

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



Comment at: 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp:38-39
+  if (V.end() != first) {
+clang_analyzer_eval(clang_analyzer_container_end(V) ==
+clang_analyzer_iterator_position(first));
+  }

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 249043.
baloghadamsoftware added a comment.

Correct diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
  
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
  clang/test/Analysis/loop-widening-notes.cpp

Index: clang/test/Analysis/loop-widening-notes.cpp
===
--- clang/test/Analysis/loop-widening-notes.cpp
+++ clang/test/Analysis/loop-widening-notes.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha -analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-max-loop 2 -analyzer-config widen-loops=true -analyzer-output=text -verify -analyzer-config eagerly-assume=false %s
 
 int *p_a;
 int bar();
Index: clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
@@ -0,0 +1,41 @@
+// 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: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+template 
+long clang_analyzer_container_end(const Container&);
+template 
+long clang_analyzer_iterator_position(const Iterator&);
+
+void clang_analyzer_eval(bool);
+
+
+template 
+InputIterator nonStdFind(InputIterator first, InputIterator last,
+ const T ) {
+  for (auto i = first; i != last; ++i) {
+if (*i == val) {
+  return i;
+}
+  }
+  return last;
+}
+
+void non_std_find(std::vector , 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));
+  }
+}
Index: clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
===
--- /dev/null
+++ clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
@@ -0,0 +1,10 @@
+// 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: checker cannot be enabled with analyzer option 'aggressive-binary-operation-simplification' == false
Index: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ 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 @@
 }
 
 bool ento::shouldRegisterContainerModeling(const CheckerManager ) {
+  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;
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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 () const { return LangOpts; }
  -  AnalyzerOptions () { return AOptions; }
  -  ASTContext () {
  +  AnalyzerOptions () const { return AOptions; }
  +  ASTContext () 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 
) {
   }
   
   bool ento::shouldRegisterContainerModeling(const CheckerManager ) {
  +  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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

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 `shouldRegister`//XXX//`()` 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

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.




Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:1037-1038
+llvm::errs() << "[ERROR] Checker " << mgr.getCurrentCheckerName().getName()
+ << " cannot be enabled without analyzer option aggressive-"
+"binary-operation-simplification.\n";
+return false;

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

'aggressive-binary-operation-simplification'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75171/new/

https://reviews.llvm.org/D75171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus.
baloghadamsoftware added a project: clang.
Herald added subscribers: martong, steakhal, Charusso, gamesh411, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75171

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
  
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp


Index: 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
@@ -0,0 +1,41 @@
+// 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 
+long clang_analyzer_container_end(const Container&);
+template 
+long clang_analyzer_iterator_position(const Iterator&);
+
+void clang_analyzer_eval(bool);
+
+
+template 
+InputIterator nonStdFind(InputIterator first, InputIterator last,
+ const T ) {
+  for (auto i = first; i != last; ++i) {
+if (*i == val) {
+  return i;
+}
+  }
+  return last;
+}
+
+void non_std_find(std::vector , 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));
+  }
+}
Index: 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
===
--- /dev/null
+++ 
clang/test/Analysis/container-modeling-no-aggressive-binary-operation-simplification-warn.cpp
@@ -0,0 +1,10 @@
+// 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.
Index: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
@@ -1031,10 +1031,23 @@
 
 } // namespace
 
+static bool checkAggressiveSimplificationEnabled(CheckerManager ) {
+  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 ) {
+  if (!checkAggressiveSimplificationEnabled(mgr))
+return;
+
   mgr.registerChecker();
 }
 
 bool ento::shouldRegisterContainerModeling(const LangOptions ) {
-  return true;
+  return LO.CPlusPlus;
 }


Index: clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/iterator-modeling-no-aggressive-binary-operation-simplification-no-crash.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_analyze_cc1 -std=c++11\
+// RUN: -analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection\
+// RUN: -analyzer-config c++-container-inlining=false