[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added a comment.

In D66042#1655119 , @sylvestre.ledru 
wrote:

> @Charusso This probably should be added to the release notes:
>  https://clang.llvm.org/docs/ReleaseNotes.html#static-analyzer
>  and detailed in the doc.
>  Please let me know if you need help!


For the time being, we are keeping this as a developer only feature, but we're 
working on potentially promoting it to a regular frontend flag!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

@Charusso This probably should be added to the release notes:
https://clang.llvm.org/docs/ReleaseNotes.html#static-analyzer
and detailed in the doc.
Please let me know if you need help!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369078: [analyzer] Analysis: Silence checkers (authored by 
Charusso, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66042?vs=215518=215520#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66042

Files:
  cfe/trunk/include/clang/Basic/DiagnosticCommonKinds.td
  cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
  cfe/trunk/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  cfe/trunk/test/Analysis/analyzer-config.c
  cfe/trunk/test/Analysis/silence-checkers-and-packages-core-all.cpp
  cfe/trunk/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
  cfe/trunk/tools/scan-build/bin/scan-build
  cfe/trunk/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp

Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -324,18 +324,18 @@
   getLastArgIntValue(Args, OPT_analyzer_inline_max_stack_depth,
  Opts.InlineMaxStackDepth, Diags);
 
-  Opts.CheckersControlList.clear();
+  Opts.CheckersAndPackages.clear();
   for (const Arg *A :
Args.filtered(OPT_analyzer_checker, OPT_analyzer_disable_checker)) {
 A->claim();
-bool enable = (A->getOption().getID() == OPT_analyzer_checker);
+bool IsEnabled = A->getOption().getID() == OPT_analyzer_checker;
 // We can have a list of comma separated checker names, e.g:
 // '-analyzer-checker=cocoa,unix'
-StringRef checkerList = A->getValue();
-SmallVector checkers;
-checkerList.split(checkers, ",");
-for (auto checker : checkers)
-  Opts.CheckersControlList.emplace_back(checker, enable);
+StringRef CheckerAndPackageList = A->getValue();
+SmallVector CheckersAndPackages;
+CheckerAndPackageList.split(CheckersAndPackages, ",");
+for (const StringRef CheckerOrPackage : CheckersAndPackages)
+  Opts.CheckersAndPackages.emplace_back(CheckerOrPackage, IsEnabled);
   }
 
   // Go through the analyzer configuration options.
@@ -479,6 +479,32 @@
   !llvm::sys::fs::is_directory(AnOpts.ModelPath))
 Diags->Report(diag::err_analyzer_config_invalid_input) << "model-path"
<< "a filename";
+
+  // FIXME: Here we try to validate the silenced checkers or packages are valid.
+  // The current approach only validates the registered checkers which does not
+  // contain the runtime enabled checkers and optimally we would validate both.
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;
+AnOpts.RawSilencedCheckersAndPackages.split(CheckersAndPackages, ";");
+
+for (const StringRef CheckerOrPackage : CheckersAndPackages) {
+  bool IsChecker = CheckerOrPackage.contains('.');
+  bool IsValidName =
+  IsChecker ? llvm::find(Checkers, CheckerOrPackage) != Checkers.end()
+: llvm::find(Packages, CheckerOrPackage) != Packages.end();
+
+  if (!IsValidName)
+Diags->Report(diag::err_unknown_analyzer_checker_or_package)
+<< CheckerOrPackage;
+
+  AnOpts.SilencedCheckersAndPackages.emplace_back(CheckerOrPackage);
+}
+  }
 }
 
 static bool ParseMigratorArgs(MigratorOptions , ArgList ) {
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -1924,15 +1924,22 @@
 
 std::unique_ptr
 PathDiagnosticBuilder::generate(const PathDiagnosticConsumer *PDC) const {
-
-  if (!PDC->shouldGenerateDiagnostics())
-return generateEmptyDiagnosticForReport(R, getSourceManager());
-
   PathDiagnosticConstruct Construct(PDC, ErrorNode, R);
 
   const SourceManager  = getSourceManager();
   const BugReport *R = getBugReport();
   const AnalyzerOptions  = getAnalyzerOptions();
+  StringRef ErrorTag = ErrorNode->getLocation().getTag()->getTagDescription();
+
+  // See whether we need to silence the checker/package.
+  // FIXME: This will not work if the report was emitted with an incorrect tag.
+  for (const std::string  : Opts.SilencedCheckersAndPackages) {
+if 

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the reviews!




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

NoQ wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > NoQ wrote:
> > > > > Charusso wrote:
> > > > > > Szelethus wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > Charusso wrote:
> > > > > > > > > Szelethus wrote:
> > > > > > > > > > Szelethus wrote:
> > > > > > > > > > > Szelethus wrote:
> > > > > > > > > > > > The reason why I suggested validating this in 
> > > > > > > > > > > > CheckerRegistry is that CheckerRegistry is the only 
> > > > > > > > > > > > class knowing the actual list of checkers and packages, 
> > > > > > > > > > > > and is able to emit diagnostics before the analysis 
> > > > > > > > > > > > starts. This solution wouldn't work with plugin 
> > > > > > > > > > > > checkers/packages.
> > > > > > > > > > > I don't see this being addressed actually?
> > > > > > > > > > > 
> > > > > > > > > > > I think it would be totally fine to just omit the 
> > > > > > > > > > > validation part as I said earlier, the patch will be 
> > > > > > > > > > > leaner, and cases in which we're using the silencing of 
> > > > > > > > > > > checkers are very exotic anyways.
> > > > > > > > > > Also, we should probably compliment such validation by 
> > > > > > > > > > actually writing tests for plugins.
> > > > > > > > > > 
> > > > > > > > > > I've been through that process once. It isn't fun. 
> > > > > > > > > > Really-really isn't :^) Let's just collectively agree to 
> > > > > > > > > > "forget" this :)
> > > > > > > > > Checker validation is in `CheckerRegistry`, configuration 
> > > > > > > > > validation is in `parseAnalyzerConfigs()`. I have made a 
> > > > > > > > > configuration, rather than a checker flag, so that I have not 
> > > > > > > > > found more appropriate place and its does the job well.  If 
> > > > > > > > > it will be needed externally, I hope someone could do better.
> > > > > > > > Well isn't this checker validation?
> > > > > > > In any case, could just throw in a FIXME before commiting please? 
> > > > > > > :)
> > > > > > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain 
> > > > > > the plugins?
> > > > > //*/me doesn't know much about plugins*//
> > > > > 
> > > > > Normally enable-disable flags do work on plugins, so plugin checkers 
> > > > > must make it into the registry at some point.
> > > > So do we need a FIXME or most likely it working with plugins?
> > > Which doesn't happen here. CheckerRegistry is the only class supplied 
> > > with this information.
> > > 
> > > A primitive way to demonstrate this is the following: 
> > > `AnalyzerOptions::getRegisteredCheckers()` is a static function without 
> > > parameters that doesn't use global variables, so it's a hair away from 
> > > being `constexpr` as well. Plugins are an inherently runtime thing. A 
> > > more concrete proof is that it only works with the inclusion of 
> > > `Checkers.inc`, which is generated compile time by TableGen.
> > > 
> > > This was the primary reason behind us struggling really hard with checker 
> > > dependencies and checker options, because we can't solve this problem 
> > > with the use of TableGen only (relieving us of any runtime overhead). 
> > > This is also the main reason behind my caution whenever this part of the 
> > > project is touched -- we're in an okay spot now compared to when these 
> > > systems got implemented, but we're quite a distance away from perfect.
> > > 
> > > You can read more about plugins and checker registration here, that I'll 
> > > promise myself to finish after GSoC is wrapped up: D58065
> > I know for a fact that this wouldn't work with plugins, believe me :)
> Anyway, it's either "FIXME: This doesn't work with plugins" or "TODO: Does 
> this work with plugins?".
Okay, cool, thanks! I like the `FIXME` category:
```
  // FIXME: Here we try to validate the silenced checkers or packages are valid.
  // The current approach only validates the registered checkers which does not
  // contain the runtime enabled checkers and optimally we would validate both.
```


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 215518.
Charusso marked 10 inline comments as done.
Charusso added a comment.

- Rebased.
- Added the remaining FIXME.


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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
  clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckersAndPackages = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  SilenceCheckers => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers are disabled/silenced.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from that checker so we have to silence it.
+  if (index($Checker, "core") == 0) {
+$Options{SilenceCheckers}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+ keys %{$Options{SilenceCheckers}}) {
+  # Push checkers in order they were silenced.
+  push @AnalysesToRun, "-analyzer-config silence-checker", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers=core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
+
+void test_disable_null_deref(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // expected-warning@-1 {{Array access (from variable 'p') results in a null pointer dereference}}
+}
Index: clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers=core \
+// RUN:  -verify %s
+
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers="core.DivideZero;core.NullDereference" \
+// RUN:  -verify %s
+
+// RUN: not 

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Szelethus wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > Charusso wrote:
> > > > > Szelethus wrote:
> > > > > > Szelethus wrote:
> > > > > > > Charusso wrote:
> > > > > > > > Szelethus wrote:
> > > > > > > > > Szelethus wrote:
> > > > > > > > > > Szelethus wrote:
> > > > > > > > > > > The reason why I suggested validating this in 
> > > > > > > > > > > CheckerRegistry is that CheckerRegistry is the only class 
> > > > > > > > > > > knowing the actual list of checkers and packages, and is 
> > > > > > > > > > > able to emit diagnostics before the analysis starts. This 
> > > > > > > > > > > solution wouldn't work with plugin checkers/packages.
> > > > > > > > > > I don't see this being addressed actually?
> > > > > > > > > > 
> > > > > > > > > > I think it would be totally fine to just omit the 
> > > > > > > > > > validation part as I said earlier, the patch will be 
> > > > > > > > > > leaner, and cases in which we're using the silencing of 
> > > > > > > > > > checkers are very exotic anyways.
> > > > > > > > > Also, we should probably compliment such validation by 
> > > > > > > > > actually writing tests for plugins.
> > > > > > > > > 
> > > > > > > > > I've been through that process once. It isn't fun. 
> > > > > > > > > Really-really isn't :^) Let's just collectively agree to 
> > > > > > > > > "forget" this :)
> > > > > > > > Checker validation is in `CheckerRegistry`, configuration 
> > > > > > > > validation is in `parseAnalyzerConfigs()`. I have made a 
> > > > > > > > configuration, rather than a checker flag, so that I have not 
> > > > > > > > found more appropriate place and its does the job well.  If it 
> > > > > > > > will be needed externally, I hope someone could do better.
> > > > > > > Well isn't this checker validation?
> > > > > > In any case, could just throw in a FIXME before commiting please? :)
> > > > > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the 
> > > > > plugins?
> > > > //*/me doesn't know much about plugins*//
> > > > 
> > > > Normally enable-disable flags do work on plugins, so plugin checkers 
> > > > must make it into the registry at some point.
> > > So do we need a FIXME or most likely it working with plugins?
> > Which doesn't happen here. CheckerRegistry is the only class supplied with 
> > this information.
> > 
> > A primitive way to demonstrate this is the following: 
> > `AnalyzerOptions::getRegisteredCheckers()` is a static function without 
> > parameters that doesn't use global variables, so it's a hair away from 
> > being `constexpr` as well. Plugins are an inherently runtime thing. A more 
> > concrete proof is that it only works with the inclusion of `Checkers.inc`, 
> > which is generated compile time by TableGen.
> > 
> > This was the primary reason behind us struggling really hard with checker 
> > dependencies and checker options, because we can't solve this problem with 
> > the use of TableGen only (relieving us of any runtime overhead). This is 
> > also the main reason behind my caution whenever this part of the project is 
> > touched -- we're in an okay spot now compared to when these systems got 
> > implemented, but we're quite a distance away from perfect.
> > 
> > You can read more about plugins and checker registration here, that I'll 
> > promise myself to finish after GSoC is wrapped up: D58065
> I know for a fact that this wouldn't work with plugins, believe me :)
Anyway, it's either "FIXME: This doesn't work with plugins" or "TODO: Does this 
work with plugins?".


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > Szelethus wrote:
> > > > > Szelethus wrote:
> > > > > > Charusso wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > Szelethus wrote:
> > > > > > > > > Szelethus wrote:
> > > > > > > > > > The reason why I suggested validating this in 
> > > > > > > > > > CheckerRegistry is that CheckerRegistry is the only class 
> > > > > > > > > > knowing the actual list of checkers and packages, and is 
> > > > > > > > > > able to emit diagnostics before the analysis starts. This 
> > > > > > > > > > solution wouldn't work with plugin checkers/packages.
> > > > > > > > > I don't see this being addressed actually?
> > > > > > > > > 
> > > > > > > > > I think it would be totally fine to just omit the validation 
> > > > > > > > > part as I said earlier, the patch will be leaner, and cases 
> > > > > > > > > in which we're using the silencing of checkers are very 
> > > > > > > > > exotic anyways.
> > > > > > > > Also, we should probably compliment such validation by actually 
> > > > > > > > writing tests for plugins.
> > > > > > > > 
> > > > > > > > I've been through that process once. It isn't fun. 
> > > > > > > > Really-really isn't :^) Let's just collectively agree to 
> > > > > > > > "forget" this :)
> > > > > > > Checker validation is in `CheckerRegistry`, configuration 
> > > > > > > validation is in `parseAnalyzerConfigs()`. I have made a 
> > > > > > > configuration, rather than a checker flag, so that I have not 
> > > > > > > found more appropriate place and its does the job well.  If it 
> > > > > > > will be needed externally, I hope someone could do better.
> > > > > > Well isn't this checker validation?
> > > > > In any case, could just throw in a FIXME before commiting please? :)
> > > > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the 
> > > > plugins?
> > > //*/me doesn't know much about plugins*//
> > > 
> > > Normally enable-disable flags do work on plugins, so plugin checkers must 
> > > make it into the registry at some point.
> > So do we need a FIXME or most likely it working with plugins?
> Which doesn't happen here. CheckerRegistry is the only class supplied with 
> this information.
> 
> A primitive way to demonstrate this is the following: 
> `AnalyzerOptions::getRegisteredCheckers()` is a static function without 
> parameters that doesn't use global variables, so it's a hair away from being 
> `constexpr` as well. Plugins are an inherently runtime thing. A more concrete 
> proof is that it only works with the inclusion of `Checkers.inc`, which is 
> generated compile time by TableGen.
> 
> This was the primary reason behind us struggling really hard with checker 
> dependencies and checker options, because we can't solve this problem with 
> the use of TableGen only (relieving us of any runtime overhead). This is also 
> the main reason behind my caution whenever this part of the project is 
> touched -- we're in an okay spot now compared to when these systems got 
> implemented, but we're quite a distance away from perfect.
> 
> You can read more about plugins and checker registration here, that I'll 
> promise myself to finish after GSoC is wrapped up: D58065
I know for a fact that this wouldn't work with plugins, believe me :)


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > Charusso wrote:
> > > > > > Szelethus wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > Szelethus wrote:
> > > > > > > > > The reason why I suggested validating this in CheckerRegistry 
> > > > > > > > > is that CheckerRegistry is the only class knowing the actual 
> > > > > > > > > list of checkers and packages, and is able to emit 
> > > > > > > > > diagnostics before the analysis starts. This solution 
> > > > > > > > > wouldn't work with plugin checkers/packages.
> > > > > > > > I don't see this being addressed actually?
> > > > > > > > 
> > > > > > > > I think it would be totally fine to just omit the validation 
> > > > > > > > part as I said earlier, the patch will be leaner, and cases in 
> > > > > > > > which we're using the silencing of checkers are very exotic 
> > > > > > > > anyways.
> > > > > > > Also, we should probably compliment such validation by actually 
> > > > > > > writing tests for plugins.
> > > > > > > 
> > > > > > > I've been through that process once. It isn't fun. Really-really 
> > > > > > > isn't :^) Let's just collectively agree to "forget" this :)
> > > > > > Checker validation is in `CheckerRegistry`, configuration 
> > > > > > validation is in `parseAnalyzerConfigs()`. I have made a 
> > > > > > configuration, rather than a checker flag, so that I have not found 
> > > > > > more appropriate place and its does the job well.  If it will be 
> > > > > > needed externally, I hope someone could do better.
> > > > > Well isn't this checker validation?
> > > > In any case, could just throw in a FIXME before commiting please? :)
> > > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the 
> > > plugins?
> > //*/me doesn't know much about plugins*//
> > 
> > Normally enable-disable flags do work on plugins, so plugin checkers must 
> > make it into the registry at some point.
> So do we need a FIXME or most likely it working with plugins?
Which doesn't happen here. CheckerRegistry is the only class supplied with this 
information.

A primitive way to demonstrate this is the following: 
`AnalyzerOptions::getRegisteredCheckers()` is a static function without 
parameters that doesn't use global variables, so it's a hair away from being 
`constexpr` as well. Plugins are an inherently runtime thing. A more concrete 
proof is that it only works with the inclusion of `Checkers.inc`, which is 
generated compile time by TableGen.

This was the primary reason behind us struggling really hard with checker 
dependencies and checker options, because we can't solve this problem with the 
use of TableGen only (relieving us of any runtime overhead). This is also the 
main reason behind my caution whenever this part of the project is touched -- 
we're in an okay spot now compared to when these systems got implemented, but 
we're quite a distance away from perfect.

You can read more about plugins and checker registration here, that I'll 
promise myself to finish after GSoC is wrapped up: D58065


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

NoQ wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > Charusso wrote:
> > > > > Szelethus wrote:
> > > > > > Szelethus wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > The reason why I suggested validating this in CheckerRegistry 
> > > > > > > > is that CheckerRegistry is the only class knowing the actual 
> > > > > > > > list of checkers and packages, and is able to emit diagnostics 
> > > > > > > > before the analysis starts. This solution wouldn't work with 
> > > > > > > > plugin checkers/packages.
> > > > > > > I don't see this being addressed actually?
> > > > > > > 
> > > > > > > I think it would be totally fine to just omit the validation part 
> > > > > > > as I said earlier, the patch will be leaner, and cases in which 
> > > > > > > we're using the silencing of checkers are very exotic anyways.
> > > > > > Also, we should probably compliment such validation by actually 
> > > > > > writing tests for plugins.
> > > > > > 
> > > > > > I've been through that process once. It isn't fun. Really-really 
> > > > > > isn't :^) Let's just collectively agree to "forget" this :)
> > > > > Checker validation is in `CheckerRegistry`, configuration validation 
> > > > > is in `parseAnalyzerConfigs()`. I have made a configuration, rather 
> > > > > than a checker flag, so that I have not found more appropriate place 
> > > > > and its does the job well.  If it will be needed externally, I hope 
> > > > > someone could do better.
> > > > Well isn't this checker validation?
> > > In any case, could just throw in a FIXME before commiting please? :)
> > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the 
> > plugins?
> //*/me doesn't know much about plugins*//
> 
> Normally enable-disable flags do work on plugins, so plugin checkers must 
> make it into the registry at some point.
So do we need a FIXME or most likely it working with plugins?


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Charusso wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > Szelethus wrote:
> > > > > Szelethus wrote:
> > > > > > Szelethus wrote:
> > > > > > > The reason why I suggested validating this in CheckerRegistry is 
> > > > > > > that CheckerRegistry is the only class knowing the actual list of 
> > > > > > > checkers and packages, and is able to emit diagnostics before the 
> > > > > > > analysis starts. This solution wouldn't work with plugin 
> > > > > > > checkers/packages.
> > > > > > I don't see this being addressed actually?
> > > > > > 
> > > > > > I think it would be totally fine to just omit the validation part 
> > > > > > as I said earlier, the patch will be leaner, and cases in which 
> > > > > > we're using the silencing of checkers are very exotic anyways.
> > > > > Also, we should probably compliment such validation by actually 
> > > > > writing tests for plugins.
> > > > > 
> > > > > I've been through that process once. It isn't fun. Really-really 
> > > > > isn't :^) Let's just collectively agree to "forget" this :)
> > > > Checker validation is in `CheckerRegistry`, configuration validation is 
> > > > in `parseAnalyzerConfigs()`. I have made a configuration, rather than a 
> > > > checker flag, so that I have not found more appropriate place and its 
> > > > does the job well.  If it will be needed externally, I hope someone 
> > > > could do better.
> > > Well isn't this checker validation?
> > In any case, could just throw in a FIXME before commiting please? :)
> @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the plugins?
//*/me doesn't know much about plugins*//

Normally enable-disable flags do work on plugins, so plugin checkers must make 
it into the registry at some point.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Szelethus wrote:
> > Charusso wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > Szelethus wrote:
> > > > > > The reason why I suggested validating this in CheckerRegistry is 
> > > > > > that CheckerRegistry is the only class knowing the actual list of 
> > > > > > checkers and packages, and is able to emit diagnostics before the 
> > > > > > analysis starts. This solution wouldn't work with plugin 
> > > > > > checkers/packages.
> > > > > I don't see this being addressed actually?
> > > > > 
> > > > > I think it would be totally fine to just omit the validation part as 
> > > > > I said earlier, the patch will be leaner, and cases in which we're 
> > > > > using the silencing of checkers are very exotic anyways.
> > > > Also, we should probably compliment such validation by actually writing 
> > > > tests for plugins.
> > > > 
> > > > I've been through that process once. It isn't fun. Really-really isn't 
> > > > :^) Let's just collectively agree to "forget" this :)
> > > Checker validation is in `CheckerRegistry`, configuration validation is 
> > > in `parseAnalyzerConfigs()`. I have made a configuration, rather than a 
> > > checker flag, so that I have not found more appropriate place and its 
> > > does the job well.  If it will be needed externally, I hope someone could 
> > > do better.
> > Well isn't this checker validation?
> In any case, could just throw in a FIXME before commiting please? :)
@NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the plugins?


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Guys, thank you so much for working on establishing consensus on this change.
//*bows*//


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > The reason why I suggested validating this in CheckerRegistry is that 
> > > > > CheckerRegistry is the only class knowing the actual list of checkers 
> > > > > and packages, and is able to emit diagnostics before the analysis 
> > > > > starts. This solution wouldn't work with plugin checkers/packages.
> > > > I don't see this being addressed actually?
> > > > 
> > > > I think it would be totally fine to just omit the validation part as I 
> > > > said earlier, the patch will be leaner, and cases in which we're using 
> > > > the silencing of checkers are very exotic anyways.
> > > Also, we should probably compliment such validation by actually writing 
> > > tests for plugins.
> > > 
> > > I've been through that process once. It isn't fun. Really-really isn't 
> > > :^) Let's just collectively agree to "forget" this :)
> > Checker validation is in `CheckerRegistry`, configuration validation is in 
> > `parseAnalyzerConfigs()`. I have made a configuration, rather than a 
> > checker flag, so that I have not found more appropriate place and its does 
> > the job well.  If it will be needed externally, I hope someone could do 
> > better.
> Well isn't this checker validation?
In any case, could just throw in a FIXME before commiting please? :)


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence 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.

You know what, I argued that we should use configs because this flag is 
incomplete. So I shouldn't be all up and down that it isn't.

LGTM!


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Charusso wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > The reason why I suggested validating this in CheckerRegistry is that 
> > > > CheckerRegistry is the only class knowing the actual list of checkers 
> > > > and packages, and is able to emit diagnostics before the analysis 
> > > > starts. This solution wouldn't work with plugin checkers/packages.
> > > I don't see this being addressed actually?
> > > 
> > > I think it would be totally fine to just omit the validation part as I 
> > > said earlier, the patch will be leaner, and cases in which we're using 
> > > the silencing of checkers are very exotic anyways.
> > Also, we should probably compliment such validation by actually writing 
> > tests for plugins.
> > 
> > I've been through that process once. It isn't fun. Really-really isn't :^) 
> > Let's just collectively agree to "forget" this :)
> Checker validation is in `CheckerRegistry`, configuration validation is in 
> `parseAnalyzerConfigs()`. I have made a configuration, rather than a checker 
> flag, so that I have not found more appropriate place and its does the job 
> well.  If it will be needed externally, I hope someone could do better.
Well isn't this checker validation?


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > The reason why I suggested validating this in CheckerRegistry is that 
> > > CheckerRegistry is the only class knowing the actual list of checkers and 
> > > packages, and is able to emit diagnostics before the analysis starts. 
> > > This solution wouldn't work with plugin checkers/packages.
> > I don't see this being addressed actually?
> > 
> > I think it would be totally fine to just omit the validation part as I said 
> > earlier, the patch will be leaner, and cases in which we're using the 
> > silencing of checkers are very exotic anyways.
> Also, we should probably compliment such validation by actually writing tests 
> for plugins.
> 
> I've been through that process once. It isn't fun. Really-really isn't :^) 
> Let's just collectively agree to "forget" this :)
Checker validation is in `CheckerRegistry`, configuration validation is in 
`parseAnalyzerConfigs()`. I have made a configuration, rather than a checker 
flag, so that I have not found more appropriate place and its does the job 
well.  If it will be needed externally, I hope someone could do better.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> Szelethus wrote:
> > The reason why I suggested validating this in CheckerRegistry is that 
> > CheckerRegistry is the only class knowing the actual list of checkers and 
> > packages, and is able to emit diagnostics before the analysis starts. This 
> > solution wouldn't work with plugin checkers/packages.
> I don't see this being addressed actually?
> 
> I think it would be totally fine to just omit the validation part as I said 
> earlier, the patch will be leaner, and cases in which we're using the 
> silencing of checkers are very exotic anyways.
Also, we should probably compliment such validation by actually writing tests 
for plugins.

I've been through that process once. It isn't fun. Really-really isn't :^) 
Let's just collectively agree to "forget" this :)


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added a comment.

I swear this is my last objection :) As soon as this is settled, I'll accept.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

Szelethus wrote:
> The reason why I suggested validating this in CheckerRegistry is that 
> CheckerRegistry is the only class knowing the actual list of checkers and 
> packages, and is able to emit diagnostics before the analysis starts. This 
> solution wouldn't work with plugin checkers/packages.
I don't see this being addressed actually?

I think it would be totally fine to just omit the validation part as I said 
earlier, the patch will be leaner, and cases in which we're using the silencing 
of checkers are very exotic anyways.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added a comment.

Now that I had ever more time to think about this patch, I see a great 
potential in it for development use, for example, we could silence a checker 
before splitting it up to see whether we could disable it altogether or really 
having to go through the process of splitting it into a modeling and reporting 
portion.




Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:303
 // Static Analyzer Core
-def err_unknown_analyzer_checker : Error<
+def err_unknown_analyzer_checker_or_package : Error<
 "no analyzer checkers or packages are associated with '%0'">;

Cheers!



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:167
   static std::vector
-  getRegisteredCheckers(bool IncludeExperimental = false);
+  getRegisteredCheckers(bool IncludeExperimental = false) {
+static const StringRef StaticAnalyzerChecks[] = {

This has been bugging me for a while. Registered checkers are checkers that 
were, well, registered into the analyzer, that may also include plugins. This 
should rather be called `getBuiltinCheckers`, because it only retrieves the 
list of checkers declared in `Checkers.td`.

This is just thinking aloud, not related to your revision.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:177-178
+for (StringRef CheckerName : StaticAnalyzerChecks) {
+  if (!CheckerName.startswith("debug.") &&
+  (IncludeExperimental || !CheckerName.startswith("alpha.")))
+Checkers.push_back(CheckerName);

I wonder why the decision was made to hide debug checkers for clang-tidy. It so 
happened once that I wanted to enable one through clang-tidy's interface, but 
couldn't.

But this isn't the time to change it, just again, thinking aloud.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+  if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+std::vector Checkers =
+AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+std::vector Packages =
+AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+SmallVector CheckersAndPackages;

The reason why I suggested validating this in CheckerRegistry is that 
CheckerRegistry is the only class knowing the actual list of checkers and 
packages, and is able to emit diagnostics before the analysis starts. This 
solution wouldn't work with plugin checkers/packages.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 215159.
Charusso marked 4 inline comments as done.
Charusso added a comment.

- Rainbow Butterfly Unicorn Kitty 


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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
  clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,7 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
+Compiler.getAnalyzerOpts()->CheckersAndPackages = {
 {"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  SilenceCheckers => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers are disabled/silenced.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from that checker so we have to silence it.
+  if (index($Checker, "core") == 0) {
+$Options{SilenceCheckers}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+ keys %{$Options{SilenceCheckers}}) {
+  # Push checkers in order they were silenced.
+  push @AnalysesToRun, "-analyzer-config silence-checker", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checkers-and-packages-core-div-by-zero.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers=core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
+
+void test_disable_null_deref(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // expected-warning@-1 {{Array access (from variable 'p') results in a null pointer dereference}}
+}
Index: clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checkers-and-packages-core-all.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers=core \
+// RUN:  -verify %s
+
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core -analyzer-config \
+// RUN:   silence-checkers="core.DivideZero;core.NullDereference" \
+// RUN: 

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:468-502
 void CheckerRegistry::validateCheckerOptions() const {
   for (const auto  : AnOpts.Config) {
 
 StringRef SuppliedChecker;
 StringRef SuppliedOption;
 std::tie(SuppliedChecker, SuppliedOption) = Config.getKey().split(':');
 

Also, you can validate whether the user-provided checker/package names actually 
exist by modifying this function a bit.

Only if you feel like it, I don't insist :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:392
+StringRef, RawSilencedCheckers, "silence-checkers",
+"A semicolon separated list of checker and package names to silence.", "")
+

Hmm, maybe we could elaborate on what we mean under silencing.
```
Silenced checkers will remain enabled and work as usual during analysis, but 
bug reports originating from them are suppressed.
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

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

Szelethus added a comment.

Thanks!!! Please note that BugReporter.cpp (especially the parts you touched) 
just got refactored extensively, so you'll need to rebase on master.




Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192
   /// Pair of checker name and enable/disable.
-  std::vector> CheckersControlList;
+  std::vector> Checkers;
+

Hmm, this doesn't really describe what this field does, does it? This isn't 
even the entire list, only those explicitly specified by `-analyzer-checker` 
and `-analyzer-disable-checker`. Besides, package names are also in this 
container. How about any of these:

* `EnabledCheckersAndPackages`
* `EnabledCheckers`
* `CheckerAndPackageUserInput` (this may just be the name that describes whats 
happening here the best)
* `CheckerUserInput`



Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:195
+  /// Vector of checker names which will not emit warnings.
+  std::vector SilencedCheckers;
 

Same here. I realize the name of the field could get ridiculously long, how 
about we leave this as-is but mention it in the comments that packages are also 
included?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:1924
+
+  // See whether we need to silence the checker.
+  for (const std::string  : Opts.SilencedCheckers) {

Could you please add a `FIXME` about that this will not work if the report was 
emitted with the incorrect tag?


Repository:
  rC Clang

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I realized that it is meaningless what separator we use to list the checkers, 
so I have picked `;`. The `,` is limited to the `analyzer-config` list.




Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197
+  /// Pair of checker name and enable/disable to do analysis.
+  std::vector> CheckerAnalysisVector;
+
+  /// Vector of checker names to do not emit warnings.
+  std::vector CheckerSilenceVector;
 

Szelethus wrote:
> Charusso wrote:
> > NoQ wrote:
> > > `CheckerAnalysisVector` sounds like a vector of checkers that will be 
> > > subject to analysis. But in reality they are the ones that are analyzing 
> > > and nobody is analyzing them.
> > > 
> > > The old name isn't very expressive either, but at least it sounds cool >.<
> > > 
> > > Maybe `EnabledCheckers`, `SilencedCheckers`?
> > The problem with `EnabledCheckers` it is a lie. It would be just `Checkers` 
> > and then whether a given checker is enabled or disabled is determined 
> > later. `CheckerEnableVector` and `CheckerSilenceVector` may would be okay.
> Aye, I've come across this field multiple times and there isn't really a good 
> name for it. However, is this correct? Are these *checkers*, or checkers 
> *and* packages?
`SilencedCheckersAndPackagesAsCommaSeparatedListSplittedToStringRefVector` 
simplified to `SilencedCheckers` and `CheckersControlList` to `Checkers`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66042



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

[PATCH] D66042: [analyzer] Analysis: Silence checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 215103.
Charusso marked 4 inline comments as done.
Charusso retitled this revision from "[analyzer] Analysis: "Disable" core 
checkers" to "[analyzer] Analysis: Silence checkers".
Charusso edited the summary of this revision.
Charusso set the repository for this revision to rC Clang.
Charusso added a comment.

- Pink Fluffy Unicorns Dancing On Rainbows 



Repository:
  rC Clang

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

https://reviews.llvm.org/D66042

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/silence-checker-core-all.cpp
  clang/test/Analysis/silence-checker-core-div-by-zero.cpp
  clang/tools/scan-build/bin/scan-build
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -46,8 +46,7 @@
 std::unique_ptr AnalysisConsumer =
 CreateAnalysisConsumer(Compiler);
 AnalysisConsumer->AddDiagnosticConsumer(new DiagConsumer(DiagsOutput));
-Compiler.getAnalyzerOpts()->CheckersControlList = {
-{"custom.CustomChecker", true}};
+Compiler.getAnalyzerOpts()->Checkers = {{"custom.CustomChecker", true}};
 AnalysisConsumer->AddCheckerRegistrationFn([](CheckerRegistry ) {
   Registry.addChecker("custom.CustomChecker", "Description", "");
 });
Index: clang/tools/scan-build/bin/scan-build
===
--- clang/tools/scan-build/bin/scan-build
+++ clang/tools/scan-build/bin/scan-build
@@ -57,6 +57,7 @@
   KeepEmpty => 0,# Don't remove output directory even with 0 results.
   EnableCheckers => {},
   DisableCheckers => {},
+  SilenceCheckers => {},
   Excludes => [],
   UseCC => undef,# C compiler to use for compilation.
   UseCXX => undef,   # C++ compiler to use for compilation.
@@ -1742,9 +1743,15 @@
 if ($arg eq "-disable-checker") {
   shift @$Args;
   my $Checker = shift @$Args;
-  # Store $NumArgs to preserve the order the checkers were disabled.
-  $Options{DisableCheckers}{$Checker} = $NumArgs;
-  delete $Options{EnableCheckers}{$Checker};
+  # Store $NumArgs to preserve the order the checkers are disabled/silenced.
+  # See whether it is a core checker to disable. That means we do not want
+  # to emit a report from that checker so we have to silence it.
+  if (index($Checker, "core") == 0) {
+$Options{SilenceCheckers}{$Checker} = $NumArgs;
+  } else {
+$Options{DisableCheckers}{$Checker} = $NumArgs;
+delete $Options{EnableCheckers}{$Checker};
+  }
   next;
 }
 
@@ -1882,6 +1889,11 @@
   # Push checkers in order they were disabled.
   push @AnalysesToRun, "-analyzer-disable-checker", $_;
 }
+foreach (sort { $Options{SilenceCheckers}{$a} <=> $Options{SilenceCheckers}{$b} }
+ keys %{$Options{SilenceCheckers}}) {
+  # Push checkers in order they were silenced.
+  push @AnalysesToRun, "-analyzer-config silence-checker", $_;
+}
 if ($Options{AnalyzeHeaders}) { push @AnalysesToRun, "-analyzer-opt-analyze-headers"; }
 if ($Options{AnalyzerStats}) { push @AnalysesToRun, '-analyzer-checker=debug.Stats'; }
 if ($Options{MaxLoop} > 0) { push @AnalysesToRun, "-analyzer-max-loop $Options{MaxLoop}"; }
Index: clang/test/Analysis/silence-checker-core-div-by-zero.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-div-by-zero.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-config silence-checkers=core.DivideZero \
+// RUN:  -verify %s
+
+void test_disable_core_div_by_zero() {
+  (void)(1 / 0);
+  // expected-warning@-1 {{division by zero is undefined}}
+  // no-warning: 'Division by zero'
+}
+
+void test_disable_null_deref(int *p) {
+  if (p)
+return;
+
+  int x = p[0];
+  // expected-warning@-1 {{Array access (from variable 'p') results in a null pointer dereference}}
+}
Index: clang/test/Analysis/silence-checker-core-all.cpp
===
--- /dev/null
+++ clang/test/Analysis/silence-checker-core-all.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core \
+// RUN:  -analyzer-config silence-checkers=core \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN: