[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 removed a reviewer: jansvoboda11.
jansvoboda11 added inline comments.
Herald added a subscriber: jansvoboda11.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:640
+StringRef CheckList = A->getValue();
+Opts.TidyChecks.emplace_back(CheckList);
+  }

Can you please use the option marshalling infrastructure instead? 
https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95403

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

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Artem, could you set the repository to `rG LLVM Github Monorepo` when uploading 
patches as mentioned in 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
 ? This way you'll allow pre-merge checks to run.

In D95403#2534714 , @aaron.ballman 
wrote:

>> This patch introduces a frontend flag -analyzer-tidy-checker=...
>
> FWIW, the usual clang-tidy nomenclature is to call these "checks" rather than 
> "checkers", so it might make sense to expose `-analyzer-tidy-checks=...` to 
> be analogous to `clang-tidy -checks=...`

I vaguely remember discussing (most probably, with @gribozavr2) this difference 
and coming to a conclusion that there's no strong reason to keep the 
nomenclature difference between the static analyzer and clang-tidy. This didn't 
go much farther beyond creating the clang-tools-extra/test/clang-tidy/checkers/ 
directory, but we could revisit this, since we're looking into more interaction 
between the tools.




Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:54-60
+#include "../../clang-tools-extra/clang-tidy/ClangTidyCheck.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyForceLinker.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModule.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyOptions.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyProfiling.h"

NoQ wrote:
> alexfh wrote:
> > Isn't this a layering violation, since clang-tidy depends on 
> > clangStaticAnalyzerCore and clangStaticAnalyzerFrontend?
> Yes, absolutely.
> 
> That said, the only purpose of clang-tidy's dependency on libStaticAnalyzer* 
> is integration of static analyzer into clang tidy which is definitely not 
> something we want to enable when we're baking clang-tidy back into clang. It 
> never makes sense to run static analyzer through clang-tidy integration into 
> static analyzer.
> 
> So ideally these two dependencies are temporally separated. I could make 
> these dependencies mutually exclusive by making the upcoming option of baking 
> clang-tidy into clang explicitly incompatible with 
> `CLANG_TIDY_ENABLE_STATIC_ANALYZER`.
> 
> But if we want to support building both clang-tidy with static analyzer and 
> static analyzer with clang-tidy from the same sources into the same build 
> directory, that'll probably involve either building two variants of 
> clang-tidy (one with static analyzer for standalone clang-tidy binary and one 
> without to use inside clang binary only) or two variants of static analyzer 
> (one with clang-tidy for the clang binary and one without to use inside 
> clang-tidy binary only).
> 
> Do you have any preference on how should i untangle this?
I hope we could break the dependency cycle without "flipping" the direction of 
dependencies based on a CMake switch. This way it would be easier to replicate 
the dependency structure in other BUILD systems.

Now the dependencies look very roughly like this:
```
clangStaticAnalyzerCore:
clangStaticAnalyzerCheckers: clangStaticAnalyzerCore
clangStaticAnalyzerFrontend: clangStaticAnalyzerCheckers clangStaticAnalyzerCore
clangFrontendTool: clangStaticAnalyzerFrontend

clangTidy: clangStaticAnalyzerCore clangStaticAnalyzerFrontend
clang-tidy check libraries gathered under $ALL_CLANG_TIDY_CHECKS
clangTidyMain: clangTidy
clang-tidy: clangTidyMain clangTidy $ALL_CLANG_TIDY_CHECKS
```

Untangled version could look like this:
```
clangStaticAnalyzerCore:
clangStaticAnalyzerCheckers: clangStaticAnalyzerCore
clangStaticAnalyzerFrontend: clangStaticAnalyzerCheckers 
clangStaticAnalyzerCore clangTidyInterface
clangStaticAnalyzerFrontendWithClangTidy: clangTidyImpl $ALL_CLANG_TIDY_CHECKS
clangFrontendTool: clangStaticAnalyzerFrontendWithClangTidy

clangTidyInterface: (with ClangTidy.h, ClangTidyOptions.h, 
ClangTidyDiagnosticConsumer.h and ClangTidyProfiling.h, for example)
clangTidyImpl: clangTidyInterface clangStaticAnalyzerCore 
clangStaticAnalyzerFrontend
clang-tidy check libraries gathered under $ALL_CLANG_TIDY_CHECKS
clangTidyMain: clangTidyImpl
clang-tidy: clangTidyMain clangTidyImpl $ALL_CLANG_TIDY_CHECKS
```

What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95403

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

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I think it's fantastic effort!

> I'll make sure to implement (and hopefully test) a reasonable behavior for 
> all various combinations of CMake flags (eg., clang-tidy enabled/disabled, 
> static analyzer enabled/disabled, static-analyzer-into-clang-tidy integration 
> enabled/disabled, etc.).

Will we be able to stand up a buildbot to explicitly test this configuration?

> This patch introduces a frontend flag -analyzer-tidy-checker=...

FWIW, the usual clang-tidy nomenclature is to call these "checks" rather than 
"checkers", so it might make sense to expose `-analyzer-tidy-checks=...` to be 
analogous to `clang-tidy -checks=...`

> As long as at least one such flag is supplied, ClangTidyContext is 
> instantiated with the given checker list and a clang-tidy AST consumer gets 
> multiplexed with AnalysisConsumer so that to run clang-tidy checkers.

clang-tidy currently runs the clang static analyzer, so I worry a little bit 
about duplicated diagnostics. I assume that we'll disable running the static 
analyzer checks a second time through clang-tidy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95403

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

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:54-60
+#include "../../clang-tools-extra/clang-tidy/ClangTidyCheck.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyForceLinker.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModule.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyOptions.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyProfiling.h"

alexfh wrote:
> Isn't this a layering violation, since clang-tidy depends on 
> clangStaticAnalyzerCore and clangStaticAnalyzerFrontend?
Yes, absolutely.

That said, the only purpose of clang-tidy's dependency on libStaticAnalyzer* is 
integration of static analyzer into clang tidy which is definitely not 
something we want to enable when we're baking clang-tidy back into clang. It 
never makes sense to run static analyzer through clang-tidy integration into 
static analyzer.

So ideally these two dependencies are temporally separated. I could make these 
dependencies mutually exclusive by making the upcoming option of baking 
clang-tidy into clang explicitly incompatible with 
`CLANG_TIDY_ENABLE_STATIC_ANALYZER`.

But if we want to support building both clang-tidy with static analyzer and 
static analyzer with clang-tidy from the same sources into the same build 
directory, that'll probably involve either building two variants of clang-tidy 
(one with static analyzer for standalone clang-tidy binary and one without to 
use inside clang binary only) or two variants of static analyzer (one with 
clang-tidy for the clang binary and one without to use inside clang-tidy binary 
only).

Do you have any preference on how should i untangle this?


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

https://reviews.llvm.org/D95403

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

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:54-60
+#include "../../clang-tools-extra/clang-tidy/ClangTidyCheck.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyForceLinker.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModule.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyModuleRegistry.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyOptions.h"
+#include "../../clang-tools-extra/clang-tidy/ClangTidyProfiling.h"

Isn't this a layering violation, since clang-tidy depends on 
clangStaticAnalyzerCore and clangStaticAnalyzerFrontend?


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

https://reviews.llvm.org/D95403

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

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 319514.
NoQ added a comment.

Unforget to `git add` tests.


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

https://reviews.llvm.org/D95403

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Frontend/AnalysisConsumer.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
  clang/test/Analysis/tidy-integration.c

Index: clang/test/Analysis/tidy-integration.c
===
--- /dev/null
+++ clang/test/Analysis/tidy-integration.c
@@ -0,0 +1,38 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-tidy-checker=bugprone-assert-side-effect \
+// RUN: -verify=bugprone-assert-side-effect
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-tidy-checker=bugprone-infinite-loop \
+// RUN: -verify=bugprone-infinite-loop
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-tidy-checker=cppcoreguidelines-init-variables \
+// RUN: -verify=cppcoreguidelines-init-variables
+
+// Test multiple conflicting -analyzer-tidy-checker flags.
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-tidy-checker=bugprone-infinite-loop \
+// RUN: -analyzer-tidy-checker=-bugprone-* \
+// RUN: -analyzer-tidy-checker=cppcoreguidelines-init-variables \
+// RUN: -verify=cppcoreguidelines-init-variables
+
+// Test enabling checkers by default.
+// RUN: %clang --analyze %s \
+// RUN: -Xclang -verify=bugprone-assert-side-effect,bugprone-infinite-loop
+
+void test_bugprone_assert_side_effect() {
+  void exit(int);
+  #define assert(x) if (!(x)) exit(1);
+  int x = 0;
+  assert((++x) == 1); // bugprone-assert-side-effect-warning-re^}}Found assert() with side effect [bugprone-assert-side-effect]{{$
+}
+
+void test_bugprone_infinite_loop(int y) {
+  for (int x = 0; x < 10; ++y) { // bugprone-infinite-loop-warning-re^}}This loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]{{$
+  }
+}
+
+void test_cppcoreguidelines_init_variables() {
+  int x; // cppcoreguidelines-init-variables-warning-re^}}Variable 'x' is not initialized [cppcoreguidelines-init-variables]{{$
+}
Index: clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
@@ -1,3 +1,7 @@
+configure_file(
+  ${CMAKE_SOURCE_DIR}/../clang-tools-extra/clang-tidy/clang-tidy-config.h.cmake
+  ${CMAKE_CURRENT_BINARY_DIR}/clang-tidy-config.h)
+
 include_directories( ${CMAKE_CURRENT_BINARY_DIR}/../Checkers )
 
 set(LLVM_LINK_COMPONENTS
@@ -23,6 +27,31 @@
   clangLex
   clangStaticAnalyzerCheckers
   clangStaticAnalyzerCore
+  clangTidy
+  clangTidyAbseilModule
+  clangTidyAndroidModule
+  clangTidyAlteraModule
+  clangTidyBoostModule
+  clangTidyBugproneModule
+  clangTidyCERTModule
+  clangTidyConcurrencyModule
+  clangTidyCppCoreGuidelinesModule
+  clangTidyDarwinModule
+  clangTidyFuchsiaModule
+  clangTidyGoogleModule
+  clangTidyHICPPModule
+  clangTidyLinuxKernelModule
+  clangTidyLLVMModule
+  clangTidyLLVMLibcModule
+  clangTidyMiscModule
+  clangTidyModernizeModule
+  clangTidyMPIModule
+  clangTidyObjCModule
+  clangTidyOpenMPModule
+  clangTidyPerformanceModule
+  clangTidyPortabilityModule
+  clangTidyReadabilityModule
+  clangTidyZirconModule
 
   DEPENDS
   omp_gen
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -16,15 +16,20 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Analysis/PathDiagnosticConsumers.h"
+#include "clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/MultiplexConsumer.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/StaticAnalyzer/Checkers/LocalCheckers.h"
@@ -38,21 +43,35 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
+#include "llvm/Support/Registry.h"

[PATCH] D95403: [clang-tidy][analyzer][WIP] Clang-tidy reverse integration into Static Analyzer.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: vsavchenko, xazax.hun, martong, Szelethus, 
baloghadamsoftware, Charusso, steakhal, gribozavr2, alexfh, aaron.ballman.
Herald added subscribers: dang, ASDenysPetrov, phosek, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, mgorny.
Herald added a reviewer: jansvoboda11.
NoQ requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

This patch allows building a clang binary that has clang-tidy checks embedded 
into the static analyzer. I discussed these plans briefly in 
https://lists.llvm.org/pipermail/cfe-dev/2020-October/067002.html.

WIP because i haven't handled any build-time settings yet. This new facility 
//will be// under a CMake flag and most likely off by default (unless everybody 
suddenly loves this option, but even then, I still have to turn it into an 
option). I'll make sure to implement (and hopefully test) a reasonable behavior 
for all various combinations of CMake flags (eg., clang-tidy enabled/disabled, 
static analyzer enabled/disabled, static-analyzer-into-clang-tidy integration 
enabled/disabled, etc.).

This patch introduces a frontend flag `-analyzer-tidy-checker=...` that accepts 
clang-tidy checker strings (eg., 
`-analyzer-tidy-checker=bugprone-*,-cert-*,cert-env33-c`). As long as at least 
one such flag is supplied, `ClangTidyContext` is instantiated with the given 
checker list and a clang-tidy AST consumer gets multiplexed with 
`AnalysisConsumer` so that to run clang-tidy checkers. Diagnostics from these 
checks are pumped into a `PathDiagnosticConverterDiagnosticConsumer` (D94476 
) and displayed similarly to static analyzer 
diagnostics, eg. as HTML or Plist reports:

F15183921: Screen Shot 2021-01-25 at 2.59.30 PM.png 


This patch suggests enabling a few clang-tidy checks by default in Clang Driver 
(under the --analyze flag, obviously, and the new reverse integration should be 
enabled). Requirements for enabling a clang-tidy check by default would be 
similar to requirements of enabling a static analyzer check by default (finds 
real bugs by design, understandable diagnostics, etc.). Additionally, 
on-by-default clang-tidy checks should have static analyzer bug categories 
assigned to them (eg., "Logic error", "Memory error", etc.) in the diagnostic 
converter (a relatively convenient way to do so is provided).

If bug categories aren't assigned, the default bug category for clang-tidy 
check `x-y-z` is `Clang-Tidy [x]` and the default bug type is `Clang-Tidy 
[x-y-z]`. This looks pretty ok in scan-build's index.html:

F15183900: Screen Shot 2021-01-25 at 2.56.47 PM.png 


That said, we should probably provide an option to disable bug category 
conversion and use it whenever an arbitrary clang-tidy config is expected. As 
usual, we're optimizing for a reasonable default rather than for 
configurability.

I've considered a different approach to enabling/disabling clang-tidy checks 
(but didn't implement it in this patch) that's more static analyzer-like: 
`-analyzer-checker tidy.bugprone.assert-side-effect` etc. Here we treat `x` in 
`x-y-z` as a static analyzer package, hence the dot. This approach is less 
flexible because it doesn't allow arbitrary wildcards, so we still have to have 
the current approach but we should probably implement both. One thing to think 
about here would be to figure out whether each individual off-by-default 
clang-tidy check is treated as "alpha" ("we're not supporting it, don't use 
it") or opt-in ("we're supporting it but it's still of by default"). Probably 
we could additionally separate them into `alpha.tidy.*` and `optin.tidy.*`.

Clang-tidy check-specific options are not implemented yet. Again, we could 
provide a separate frontend flag, or we could interface them through 
`AnalyzerOptions` like this: `-analyzer-config 
tidy.bugprone.assert-side-effect:AssertMacros=assert,Assert,ASSERT`. And, 
again, we should probably do both.


https://reviews.llvm.org/D95403

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/include/clang/StaticAnalyzer/Frontend/AnalysisConsumer.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt

Index: clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
@@ -1,3 +1,7 @@
+configure_file(
+  ${CMAKE_SOURCE_DIR}/../clang-tools-extra/clang-tidy/clang-tidy-config.h.cmake
+  ${CMAKE_CURRENT_BINARY_DIR}/clang-tidy-config.h)
+
 include_directories( ${CMAKE_CURRENT_BINARY_DIR}/../Checkers )
 
 set(LLVM_LINK_COMPONENTS
@@ -23,6