RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
decreasing the gap between Clang-Tidy users and Static Analyzer users
and make sure it's always possible for our users to take the best of
both worlds. In particular, i'd like to make Clang-Tidy easily
integratable into all UIs that already integrate the Static Analyzer.
The opposite is already true because Clang-Tidy links to the Static
Analyzer. However, existing Static Analyzer users (eg., users that rely
on Scan-Build for their build system interception or use the Static
Analyzer through Xcode) cannot easily take advantage of Clang-Tidy.
Also, as far as i understand, Ericsson CodeChecker currently performs
ad-hoc integration of Clang-Tidy by parsing text output, which most
likely works just fine, but definitely contributes to anxiety. On the
other hand, the BugReporter facility of the Static Analyzer has a
variety of output modes, including fancy html reports and
machine-readable plist output, so if we pump Clang-Tidy reports through
the same BugReporter it'll open up some new possibilities.

Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
us: make the Static Analyzer load it as a library and expose Clang-Tidy
checks as its own, maybe in a separate package. This is harder to do
though, because Clang-Tidy lives in a separate repo and also it's a hard
sell to build it into the Clang binary. I'm open to suggestions here: we
can keep such integration under an off-by-default CMake flag (which
requires enabling compilation of clang-tools-extra) or we may even use
it as a run-time plugin (i mean, clang plugins are super wonky, but this
time it's actually fairly easy to avoid version conflicts, as they all
get compiled from the same sources simultaneously).

But regardless of how far do i end up going with such integration, first
thing first: i'll anyway need to teach Clang-Tidy how to route its
diagnostics through our diagnostic engine. This is also the only thing
that's absolutely necessary; the rest can always be hacked up on the UI
side.

It's a great question why didn't we extend Clang's DiagnosticEngine to
begin with, but instead made our own facility. I don't know the answer
to that; this happened way before i even learned what an AST is :) We
generally needed a much more sophisticated diagnostic engine because our
reports are much more complicated. While we could try to unify these
diagnostic engines completely, i don't think it's worth it.

So i think it's more feasible to make Clang-Tidy's diag() method return
a proxy object that mimics the DiagnosticBuilder interface but may also
pump the diagnostics to an instance of the Static Analyzer's
BugReporter. I hope this would avoid API breakages for clang-tidy checks.

It is going to be a bit annoying that DiagnosticBuilder treats warnings
and their respective notes as completely unrelated diagnostics, while
for the purposes of the BugReporter they need to be on the same
BugReport object. I should be able to take care of this on the
BugReporter side as long as clang-tidy checkers promise to always emit
notes immediately after their respective warnings (which seems to be the
case, but i didn't look at *all* the checks).

Finally, one feature that BugReporter was lacking until recently was the
fix-it hint support. However, this is mostly out of the way since
https://reviews.llvm.org/D65182.

Does any of that sound reasonable? Should i proceed with this?
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
Hi Artem,

On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>
> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> decreasing the gap between Clang-Tidy users and Static Analyzer users
> and make sure it's always possible for our users to take the best of
> both worlds. In particular, i'd like to make Clang-Tidy easily
> integratable into all UIs that already integrate the Static Analyzer.

I like the idea of integrating the two tools more closely. From the
user's point of view, I think, ClangTidy and Clang Static Analyzer are
doing more or less the same thing, but somehow have completely
different workflows and integrations.

> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> us: make the Static Analyzer load it as a library and expose Clang-Tidy
> checks as its own, maybe in a separate package. This is harder to do
> though, because Clang-Tidy lives in a separate repo and also it's a hard
> sell to build it into the Clang binary. I'm open to suggestions here: we
> can keep such integration under an off-by-default CMake flag (which
> requires enabling compilation of clang-tools-extra) or we may even use
> it as a run-time plugin (i mean, clang plugins are super wonky, but this
> time it's actually fairly easy to avoid version conflicts, as they all
> get compiled from the same sources simultaneously).

I would like to suggest something different: move Clang Static
Analyzer to clang-tools-extra. Build it either as a separate binary or
compile it into the clang-tidy binary. Then let `clang -analyze`
delegate to that binary for backwards compatibility.

Clang Static Analyzer is being developed in the clang repository for
historical reasons -- we didn't have clang-tools-extra at the time the
project was started. The "non-core" nature of Clang Static Analyzer is
also validated by the fact that it is an optional part of Clang -- we
have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.

Long-term, my preference would be to have all static analysis tooling
(Clang Static Analyzer, ClangTidy, any prospective dataflow-based
analysis) to have a consistent UI -- including invocation, integration
with build systems, configuration strategy etc. While the
implementation of these tools is very different, I think the audience
is largely the same.

> But regardless of how far do i end up going with such integration, first
> thing first: i'll anyway need to teach Clang-Tidy how to route its
> diagnostics through our diagnostic engine. This is also the only thing
> that's absolutely necessary; the rest can always be hacked up on the UI
> side.
>
> It's a great question why didn't we extend Clang's DiagnosticEngine to
> begin with, but instead made our own facility. I don't know the answer
> to that; this happened way before i even learned what an AST is :) We
> generally needed a much more sophisticated diagnostic engine because our
> reports are much more complicated. While we could try to unify these
> diagnostic engines completely, i don't think it's worth it.
>
> So i think it's more feasible to make Clang-Tidy's diag() method return
> a proxy object that mimics the DiagnosticBuilder interface but may also
> pump the diagnostics to an instance of the Static Analyzer's
> BugReporter. I hope this would avoid API breakages for clang-tidy checks.

I'm very concerned about introducing lookalike APIs that don't behave
exactly the same way as original ones. In LLVM, we are generally not
concerned with backwards compatibility for C++ APIs. Instead, we
should be looking to keep our code maintainable in the long run.

With that in mind, my suggestion is to refactor ClangTidy to use
BugReporter instead of DiagnosticBuilder.

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
I completely agree with what Dmitri has written. A couple of additional comments from me below.

On Wed, Aug 14, 2019 at 9:24 AM Dmitri Gribenko <[hidden email]> wrote:
Hi Artem,

On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>
> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> decreasing the gap between Clang-Tidy users and Static Analyzer users
> and make sure it's always possible for our users to take the best of
> both worlds. In particular, i'd like to make Clang-Tidy easily
> integratable into all UIs that already integrate the Static Analyzer.

I like the idea of integrating the two tools more closely. From the
user's point of view, I think, ClangTidy and Clang Static Analyzer are
doing more or less the same thing, but somehow have completely
different workflows and integrations. 

There are some distinct features (like FixIt hints or path-sensitive diagnostics) and use cases (e.g. the automated refactorings), that can be better or worse supported by different integrations and workflows. It should still be possible to take the best of two worlds and unify the user experience.


> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> us: make the Static Analyzer load it as a library and expose Clang-Tidy
> checks as its own, maybe in a separate package. This is harder to do
> though, because Clang-Tidy lives in a separate repo and also it's a hard
> sell to build it into the Clang binary. I'm open to suggestions here: we
> can keep such integration under an off-by-default CMake flag (which
> requires enabling compilation of clang-tools-extra) or we may even use
> it as a run-time plugin (i mean, clang plugins are super wonky, but this
> time it's actually fairly easy to avoid version conflicts, as they all
> get compiled from the same sources simultaneously).

I would like to suggest something different: move Clang Static
Analyzer to clang-tools-extra. Build it either as a separate binary or
compile it into the clang-tidy binary. Then let `clang -analyze`
delegate to that binary for backwards compatibility.

This would be good for a few more reasons:
1. Clear separation of functionality: clang's main purpose is to compile code rather than perform static analysis. Clang and static analyzer need partially overlapping, but different sets of options.
2. Clang binary size growth is a concern for maintainers of toolchains. Static analyzer (especially with AST matchers-based checkers) is contributing a lot to that growth. Building clang without CLANG_ENABLE_STATIC_ANALYZER somewhat solves this part of the problem, but then you need another clang binary (possibly in a separate package) to support static analyzer - I believe, that's clearly suboptimal. Thus, having a completely different binary for the static analyzer (or teach clang-tidy do everything static analyzer needs to do - including all output formats and options) would be a much better solution.


Clang Static Analyzer is being developed in the clang repository for
historical reasons -- we didn't have clang-tools-extra at the time the
project was started. The "non-core" nature of Clang Static Analyzer is
also validated by the fact that it is an optional part of Clang -- we
have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.

Long-term, my preference would be to have all static analysis tooling
(Clang Static Analyzer, ClangTidy, any prospective dataflow-based
analysis) to have a consistent UI -- including invocation, integration
with build systems, configuration strategy etc. While the
implementation of these tools is very different, I think the audience
is largely the same. 
 

> But regardless of how far do i end up going with such integration, first
> thing first: i'll anyway need to teach Clang-Tidy how to route its
> diagnostics through our diagnostic engine. This is also the only thing
> that's absolutely necessary; the rest can always be hacked up on the UI
> side.
>
> It's a great question why didn't we extend Clang's DiagnosticEngine to
> begin with, but instead made our own facility. I don't know the answer
> to that; this happened way before i even learned what an AST is :) We
> generally needed a much more sophisticated diagnostic engine because our
> reports are much more complicated. While we could try to unify these
> diagnostic engines completely, i don't think it's worth it.
>
> So i think it's more feasible to make Clang-Tidy's diag() method return
> a proxy object that mimics the DiagnosticBuilder interface but may also
> pump the diagnostics to an instance of the Static Analyzer's
> BugReporter. I hope this would avoid API breakages for clang-tidy checks.

I'm very concerned about introducing lookalike APIs that don't behave
exactly the same way as original ones. In LLVM, we are generally not
concerned with backwards compatibility for C++ APIs. Instead, we
should be looking to keep our code maintainable in the long run.

With that in mind, my suggestion is to refactor ClangTidy to use
BugReporter instead of DiagnosticBuilder.

No concerns here, as long as we ensure that BugReporter has support for FixItHints and other stuff DiagnosticBuilder provides.

 

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
<[hidden email]> wrote:

>
> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> >
> > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > and make sure it's always possible for our users to take the best of
> > both worlds. In particular, i'd like to make Clang-Tidy easily
> > integratable into all UIs that already integrate the Static Analyzer.
>
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.

Strong +1 from me!

> > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > checks as its own, maybe in a separate package. This is harder to do
> > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > sell to build it into the Clang binary. I'm open to suggestions here: we
> > can keep such integration under an off-by-default CMake flag (which
> > requires enabling compilation of clang-tools-extra) or we may even use
> > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > time it's actually fairly easy to avoid version conflicts, as they all
> > get compiled from the same sources simultaneously).
>
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.

I am not keen on this approach. A stand-alone tool is harder for users
to integrate into their workflows compared to the compiler itself,
which is already (generally) a part of their existing workflow. It's
far easier for a user to add a flag like -tidy to an existing clang
execution than it is to insert clang-tidy into a complex build system.
Personally, I would much rather see libClangTidy integrated into Clang
and exposed via a driver flag, similar to how the static analyzer
already works.

> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.

FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
and have always considered that a deficiency that has hindered
adoption. For instance, running clang-tidy required separate effort to
get it up on godbolt, whereas --analyze has worked there since the
first time I tried it.

> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.

I think this is a good goal.

~Aaron

> > But regardless of how far do i end up going with such integration, first
> > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > diagnostics through our diagnostic engine. This is also the only thing
> > that's absolutely necessary; the rest can always be hacked up on the UI
> > side.
> >
> > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > begin with, but instead made our own facility. I don't know the answer
> > to that; this happened way before i even learned what an AST is :) We
> > generally needed a much more sophisticated diagnostic engine because our
> > reports are much more complicated. While we could try to unify these
> > diagnostic engines completely, i don't think it's worth it.
> >
> > So i think it's more feasible to make Clang-Tidy's diag() method return
> > a proxy object that mimics the DiagnosticBuilder interface but may also
> > pump the diagnostics to an instance of the Static Analyzer's
> > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
>
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.
>
> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> <[hidden email]> wrote:
> >
> > Hi Artem,
> >
> > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > >
> > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > and make sure it's always possible for our users to take the best of
> > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > integratable into all UIs that already integrate the Static Analyzer.
> >
> > I like the idea of integrating the two tools more closely. From the
> > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > doing more or less the same thing, but somehow have completely
> > different workflows and integrations.
>
> Strong +1 from me!
>
> > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > checks as its own, maybe in a separate package. This is harder to do
> > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > can keep such integration under an off-by-default CMake flag (which
> > > requires enabling compilation of clang-tools-extra) or we may even use
> > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > time it's actually fairly easy to avoid version conflicts, as they all
> > > get compiled from the same sources simultaneously).
> >
> > I would like to suggest something different: move Clang Static
> > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > compile it into the clang-tidy binary. Then let `clang -analyze`
> > delegate to that binary for backwards compatibility.
>
> I am not keen on this approach. A stand-alone tool is harder for users
> to integrate into their workflows compared to the compiler itself,
> which is already (generally) a part of their existing workflow. It's
> far easier for a user to add a flag like -tidy to an existing clang
> execution than it is to insert clang-tidy into a complex build system.
> Personally, I would much rather see libClangTidy integrated into Clang
> and exposed via a driver flag, similar to how the static analyzer
> already works.

Integrating by adding a flag to the build is only one possibility, and
it does not work for all users. Not everyone is willing to pay the
analysis slowdown during compilation (otherwise we would just expose
static analyzer and clang-tidy as -W flags in the compiler as the only
integration option). Running a separate build just to analyze forces
the compiler to run code generation and produce object files and
binaries, which are not necessary for static analysis.

Dmitri

> > Clang Static Analyzer is being developed in the clang repository for
> > historical reasons -- we didn't have clang-tools-extra at the time the
> > project was started. The "non-core" nature of Clang Static Analyzer is
> > also validated by the fact that it is an optional part of Clang -- we
> > have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
> and have always considered that a deficiency that has hindered
> adoption. For instance, running clang-tidy required separate effort to
> get it up on godbolt, whereas --analyze has worked there since the
> first time I tried it.
>
> > Long-term, my preference would be to have all static analysis tooling
> > (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> > analysis) to have a consistent UI -- including invocation, integration
> > with build systems, configuration strategy etc. While the
> > implementation of these tools is very different, I think the audience
> > is largely the same.
>
> I think this is a good goal.
>
> ~Aaron
>
> > > But regardless of how far do i end up going with such integration, first
> > > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > > diagnostics through our diagnostic engine. This is also the only thing
> > > that's absolutely necessary; the rest can always be hacked up on the UI
> > > side.
> > >
> > > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > > begin with, but instead made our own facility. I don't know the answer
> > > to that; this happened way before i even learned what an AST is :) We
> > > generally needed a much more sophisticated diagnostic engine because our
> > > reports are much more complicated. While we could try to unify these
> > > diagnostic engines completely, i don't think it's worth it.
> > >
> > > So i think it's more feasible to make Clang-Tidy's diag() method return
> > > a proxy object that mimics the DiagnosticBuilder interface but may also
> > > pump the diagnostics to an instance of the Static Analyzer's
> > > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> >
> > I'm very concerned about introducing lookalike APIs that don't behave
> > exactly the same way as original ones. In LLVM, we are generally not
> > concerned with backwards compatibility for C++ APIs. Instead, we
> > should be looking to keep our code maintainable in the long run.
> >
> > With that in mind, my suggestion is to refactor ClangTidy to use
> > BugReporter instead of DiagnosticBuilder.
> >
> > Dmitri
> >
> > --
> > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
> > _______________________________________________
> > cfe-dev mailing list
> > [hidden email]
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> >
> > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > <[hidden email]> wrote:
> > >
> > > Hi Artem,
> > >
> > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > >
> > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > and make sure it's always possible for our users to take the best of
> > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > integratable into all UIs that already integrate the Static Analyzer.
> > >
> > > I like the idea of integrating the two tools more closely. From the
> > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > doing more or less the same thing, but somehow have completely
> > > different workflows and integrations.
> >
> > Strong +1 from me!
> >
> > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > checks as its own, maybe in a separate package. This is harder to do
> > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > can keep such integration under an off-by-default CMake flag (which
> > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > get compiled from the same sources simultaneously).
> > >
> > > I would like to suggest something different: move Clang Static
> > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > delegate to that binary for backwards compatibility.
> >
> > I am not keen on this approach. A stand-alone tool is harder for users
> > to integrate into their workflows compared to the compiler itself,
> > which is already (generally) a part of their existing workflow. It's
> > far easier for a user to add a flag like -tidy to an existing clang
> > execution than it is to insert clang-tidy into a complex build system.
> > Personally, I would much rather see libClangTidy integrated into Clang
> > and exposed via a driver flag, similar to how the static analyzer
> > already works.
>
> Integrating by adding a flag to the build is only one possibility, and
> it does not work for all users.

I agree, my point was that a stand-alone tool doesn't work for all
users either. For my needs, moving the static analyzer into clang-tidy
makes things harder, not easier, whereas moving clang-tidy into the
compiler proper makes things easier rather than harder.

> Not everyone is willing to pay the
> analysis slowdown during compilation (otherwise we would just expose
> static analyzer and clang-tidy as -W flags in the compiler as the only
> integration option).

This is why analysis is controlled by a separate flag. I see no reason
why clang-tidy checks would not behave similarly.

> Running a separate build just to analyze forces
> the compiler to run code generation and produce object files and
> binaries, which are not necessary for static analysis.

I don't see how that follows; -fsyntax-only exists to support that
sort of use case.

~Aaron

>
> Dmitri
>
> > > Clang Static Analyzer is being developed in the clang repository for
> > > historical reasons -- we didn't have clang-tools-extra at the time the
> > > project was started. The "non-core" nature of Clang Static Analyzer is
> > > also validated by the fact that it is an optional part of Clang -- we
> > > have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
> >
> > FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
> > and have always considered that a deficiency that has hindered
> > adoption. For instance, running clang-tidy required separate effort to
> > get it up on godbolt, whereas --analyze has worked there since the
> > first time I tried it.
> >
> > > Long-term, my preference would be to have all static analysis tooling
> > > (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> > > analysis) to have a consistent UI -- including invocation, integration
> > > with build systems, configuration strategy etc. While the
> > > implementation of these tools is very different, I think the audience
> > > is largely the same.
> >
> > I think this is a good goal.
> >
> > ~Aaron
> >
> > > > But regardless of how far do i end up going with such integration, first
> > > > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > > > diagnostics through our diagnostic engine. This is also the only thing
> > > > that's absolutely necessary; the rest can always be hacked up on the UI
> > > > side.
> > > >
> > > > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > > > begin with, but instead made our own facility. I don't know the answer
> > > > to that; this happened way before i even learned what an AST is :) We
> > > > generally needed a much more sophisticated diagnostic engine because our
> > > > reports are much more complicated. While we could try to unify these
> > > > diagnostic engines completely, i don't think it's worth it.
> > > >
> > > > So i think it's more feasible to make Clang-Tidy's diag() method return
> > > > a proxy object that mimics the DiagnosticBuilder interface but may also
> > > > pump the diagnostics to an instance of the Static Analyzer's
> > > > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> > >
> > > I'm very concerned about introducing lookalike APIs that don't behave
> > > exactly the same way as original ones. In LLVM, we are generally not
> > > concerned with backwards compatibility for C++ APIs. Instead, we
> > > should be looking to keep our code maintainable in the long run.
> > >
> > > With that in mind, my suggestion is to refactor ClangTidy to use
> > > BugReporter instead of DiagnosticBuilder.
> > >
> > > Dmitri
> > >
> > > --
> > > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
> > > _______________________________________________
> > > cfe-dev mailing list
> > > [hidden email]
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
> >
> > On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > > <[hidden email]> wrote:
> > > >
> > > > Hi Artem,
> > > >
> > > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > > >
> > > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > > and make sure it's always possible for our users to take the best of
> > > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > > integratable into all UIs that already integrate the Static Analyzer.
> > > >
> > > > I like the idea of integrating the two tools more closely. From the
> > > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > > doing more or less the same thing, but somehow have completely
> > > > different workflows and integrations.
> > >
> > > Strong +1 from me!
> > >
> > > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > > checks as its own, maybe in a separate package. This is harder to do
> > > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > > can keep such integration under an off-by-default CMake flag (which
> > > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > > get compiled from the same sources simultaneously).
> > > >
> > > > I would like to suggest something different: move Clang Static
> > > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > > delegate to that binary for backwards compatibility.
> > >
> > > I am not keen on this approach. A stand-alone tool is harder for users
> > > to integrate into their workflows compared to the compiler itself,
> > > which is already (generally) a part of their existing workflow. It's
> > > far easier for a user to add a flag like -tidy to an existing clang
> > > execution than it is to insert clang-tidy into a complex build system.
> > > Personally, I would much rather see libClangTidy integrated into Clang
> > > and exposed via a driver flag, similar to how the static analyzer
> > > already works.
> >
> > Integrating by adding a flag to the build is only one possibility, and
> > it does not work for all users.
>
> I agree, my point was that a stand-alone tool doesn't work for all
> users either. For my needs, moving the static analyzer into clang-tidy
> makes things harder, not easier, whereas moving clang-tidy into the
> compiler proper makes things easier rather than harder.
>
> > Not everyone is willing to pay the
> > analysis slowdown during compilation (otherwise we would just expose
> > static analyzer and clang-tidy as -W flags in the compiler as the only
> > integration option).
>
> This is why analysis is controlled by a separate flag. I see no reason
> why clang-tidy checks would not behave similarly.

Agreed, users who *can* integrate by analyzing during the build,
should be able to do so.

> > Running a separate build just to analyze forces
> > the compiler to run code generation and produce object files and
> > binaries, which are not necessary for static analysis.
>
> I don't see how that follows; -fsyntax-only exists to support that
> sort of use case.

The build system won't proceed with the build if .o files and code
generation binaries are not produced.

Dmitri

>
> ~Aaron
>
> >
> > Dmitri
> >
> > > > Clang Static Analyzer is being developed in the clang repository for
> > > > historical reasons -- we didn't have clang-tools-extra at the time the
> > > > project was started. The "non-core" nature of Clang Static Analyzer is
> > > > also validated by the fact that it is an optional part of Clang -- we
> > > > have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
> > >
> > > FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
> > > and have always considered that a deficiency that has hindered
> > > adoption. For instance, running clang-tidy required separate effort to
> > > get it up on godbolt, whereas --analyze has worked there since the
> > > first time I tried it.
> > >
> > > > Long-term, my preference would be to have all static analysis tooling
> > > > (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> > > > analysis) to have a consistent UI -- including invocation, integration
> > > > with build systems, configuration strategy etc. While the
> > > > implementation of these tools is very different, I think the audience
> > > > is largely the same.
> > >
> > > I think this is a good goal.
> > >
> > > ~Aaron
> > >
> > > > > But regardless of how far do i end up going with such integration, first
> > > > > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > > > > diagnostics through our diagnostic engine. This is also the only thing
> > > > > that's absolutely necessary; the rest can always be hacked up on the UI
> > > > > side.
> > > > >
> > > > > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > > > > begin with, but instead made our own facility. I don't know the answer
> > > > > to that; this happened way before i even learned what an AST is :) We
> > > > > generally needed a much more sophisticated diagnostic engine because our
> > > > > reports are much more complicated. While we could try to unify these
> > > > > diagnostic engines completely, i don't think it's worth it.
> > > > >
> > > > > So i think it's more feasible to make Clang-Tidy's diag() method return
> > > > > a proxy object that mimics the DiagnosticBuilder interface but may also
> > > > > pump the diagnostics to an instance of the Static Analyzer's
> > > > > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> > > >
> > > > I'm very concerned about introducing lookalike APIs that don't behave
> > > > exactly the same way as original ones. In LLVM, we are generally not
> > > > concerned with backwards compatibility for C++ APIs. Instead, we
> > > > should be looking to keep our code maintainable in the long run.
> > > >
> > > > With that in mind, my suggestion is to refactor ClangTidy to use
> > > > BugReporter instead of DiagnosticBuilder.
> > > >
> > > > Dmitri
> > > >
> > > > --
> > > > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > > > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
> > > > _______________________________________________
> > > > cfe-dev mailing list
> > > > [hidden email]
> > > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >
> >
> >
> > --
> > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/



--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 8:38 AM Dmitri Gribenko <[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:
> >
> > On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> > > >
> > > > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > > > <[hidden email]> wrote:
> > > > >
> > > > > Hi Artem,
> > > > >
> > > > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > > > >
> > > > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > > > and make sure it's always possible for our users to take the best of
> > > > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > > > integratable into all UIs that already integrate the Static Analyzer.
> > > > >
> > > > > I like the idea of integrating the two tools more closely. From the
> > > > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > > > doing more or less the same thing, but somehow have completely
> > > > > different workflows and integrations.
> > > >
> > > > Strong +1 from me!
> > > >
> > > > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > > > checks as its own, maybe in a separate package. This is harder to do
> > > > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > > > can keep such integration under an off-by-default CMake flag (which
> > > > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > > > get compiled from the same sources simultaneously).
> > > > >
> > > > > I would like to suggest something different: move Clang Static
> > > > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > > > delegate to that binary for backwards compatibility.
> > > >
> > > > I am not keen on this approach. A stand-alone tool is harder for users
> > > > to integrate into their workflows compared to the compiler itself,
> > > > which is already (generally) a part of their existing workflow. It's
> > > > far easier for a user to add a flag like -tidy to an existing clang
> > > > execution than it is to insert clang-tidy into a complex build system.
> > > > Personally, I would much rather see libClangTidy integrated into Clang
> > > > and exposed via a driver flag, similar to how the static analyzer
> > > > already works.
> > >
> > > Integrating by adding a flag to the build is only one possibility, and
> > > it does not work for all users.
> >
> > I agree, my point was that a stand-alone tool doesn't work for all
> > users either. For my needs, moving the static analyzer into clang-tidy
> > makes things harder, not easier, whereas moving clang-tidy into the
> > compiler proper makes things easier rather than harder.
> >
> > > Not everyone is willing to pay the
> > > analysis slowdown during compilation (otherwise we would just expose
> > > static analyzer and clang-tidy as -W flags in the compiler as the only
> > > integration option).
> >
> > This is why analysis is controlled by a separate flag. I see no reason
> > why clang-tidy checks would not behave similarly.
>
> Agreed, users who *can* integrate by analyzing during the build,
> should be able to do so.

Awesome; I also agree that there may be users who cannot go with this
approach and we need to support them as well.

> > > Running a separate build just to analyze forces
> > > the compiler to run code generation and produce object files and
> > > binaries, which are not necessary for static analysis.
> >
> > I don't see how that follows; -fsyntax-only exists to support that
> > sort of use case.
>
> The build system won't proceed with the build if .o files and code
> generation binaries are not produced.

I think that depends on the build system and the project, but you're
right that some build systems won't handle this well.

~Aaron

>
> Dmitri
>
> >
> > ~Aaron
> >
> > >
> > > Dmitri
> > >
> > > > > Clang Static Analyzer is being developed in the clang repository for
> > > > > historical reasons -- we didn't have clang-tools-extra at the time the
> > > > > project was started. The "non-core" nature of Clang Static Analyzer is
> > > > > also validated by the fact that it is an optional part of Clang -- we
> > > > > have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
> > > >
> > > > FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
> > > > and have always considered that a deficiency that has hindered
> > > > adoption. For instance, running clang-tidy required separate effort to
> > > > get it up on godbolt, whereas --analyze has worked there since the
> > > > first time I tried it.
> > > >
> > > > > Long-term, my preference would be to have all static analysis tooling
> > > > > (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> > > > > analysis) to have a consistent UI -- including invocation, integration
> > > > > with build systems, configuration strategy etc. While the
> > > > > implementation of these tools is very different, I think the audience
> > > > > is largely the same.
> > > >
> > > > I think this is a good goal.
> > > >
> > > > ~Aaron
> > > >
> > > > > > But regardless of how far do i end up going with such integration, first
> > > > > > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > > > > > diagnostics through our diagnostic engine. This is also the only thing
> > > > > > that's absolutely necessary; the rest can always be hacked up on the UI
> > > > > > side.
> > > > > >
> > > > > > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > > > > > begin with, but instead made our own facility. I don't know the answer
> > > > > > to that; this happened way before i even learned what an AST is :) We
> > > > > > generally needed a much more sophisticated diagnostic engine because our
> > > > > > reports are much more complicated. While we could try to unify these
> > > > > > diagnostic engines completely, i don't think it's worth it.
> > > > > >
> > > > > > So i think it's more feasible to make Clang-Tidy's diag() method return
> > > > > > a proxy object that mimics the DiagnosticBuilder interface but may also
> > > > > > pump the diagnostics to an instance of the Static Analyzer's
> > > > > > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> > > > >
> > > > > I'm very concerned about introducing lookalike APIs that don't behave
> > > > > exactly the same way as original ones. In LLVM, we are generally not
> > > > > concerned with backwards compatibility for C++ APIs. Instead, we
> > > > > should be looking to keep our code maintainable in the long run.
> > > > >
> > > > > With that in mind, my suggestion is to refactor ClangTidy to use
> > > > > BugReporter instead of DiagnosticBuilder.
> > > > >
> > > > > Dmitri
> > > > >
> > > > > --
> > > > > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > > > > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
> > > > > _______________________________________________
> > > > > cfe-dev mailing list
> > > > > [hidden email]
> > > > > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> > >
> > >
> > >
> > > --
> > > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> > > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
>
>
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On Wed, 14 Aug 2019 at 14:16, Aaron Ballman via cfe-dev
<[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> <[hidden email]> wrote:
> >
> > Hi Artem,
> >
> > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > >
> > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > and make sure it's always possible for our users to take the best of
> > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > integratable into all UIs that already integrate the Static Analyzer.
> >
> > I like the idea of integrating the two tools more closely. From the
> > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > doing more or less the same thing, but somehow have completely
> > different workflows and integrations.
>
> Strong +1 from me!

Same here, there are other tools around that might benefit from that,
eg. clazy, and any other custom tools.

> > Long-term, my preference would be to have all static analysis tooling
> > (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> > analysis) to have a consistent UI -- including invocation, integration
> > with build systems, configuration strategy etc. While the
> > implementation of these tools is very different, I think the audience
> > is largely the same.
>
> I think this is a good goal.

Same comment again, would  be nice if the command line, and
configuration file handling could be shared and consistent b/w similar
tooling (clazy, custom, ...)

>
> ~Aaron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On Wed, 14 Aug 2019 at 14:39, Dmitri Gribenko via cfe-dev
<[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:
> >
> > On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> > > >
> > > > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > > > <[hidden email]> wrote:
> > > > >
> > > > > Hi Artem,
> > > > >
> > > > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > > > >
> > > > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > > > and make sure it's always possible for our users to take the best of
> > > > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > > > integratable into all UIs that already integrate the Static Analyzer.
> > > > >
> > > > > I like the idea of integrating the two tools more closely. From the
> > > > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > > > doing more or less the same thing, but somehow have completely
> > > > > different workflows and integrations.
> > > >
> > > > Strong +1 from me!
> > > >
> > > > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > > > checks as its own, maybe in a separate package. This is harder to do
> > > > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > > > can keep such integration under an off-by-default CMake flag (which
> > > > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > > > get compiled from the same sources simultaneously).
> > > > >
> > > > > I would like to suggest something different: move Clang Static
> > > > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > > > delegate to that binary for backwards compatibility.
> > > >
> > > > I am not keen on this approach. A stand-alone tool is harder for users
> > > > to integrate into their workflows compared to the compiler itself,
> > > > which is already (generally) a part of their existing workflow. It's
> > > > far easier for a user to add a flag like -tidy to an existing clang
> > > > execution than it is to insert clang-tidy into a complex build system.
> > > > Personally, I would much rather see libClangTidy integrated into Clang
> > > > and exposed via a driver flag, similar to how the static analyzer
> > > > already works.
> > >
> > > Integrating by adding a flag to the build is only one possibility, and
> > > it does not work for all users.
> >
> > I agree, my point was that a stand-alone tool doesn't work for all
> > users either. For my needs, moving the static analyzer into clang-tidy
> > makes things harder, not easier, whereas moving clang-tidy into the
> > compiler proper makes things easier rather than harder.
> >
> > > Not everyone is willing to pay the
> > > analysis slowdown during compilation (otherwise we would just expose
> > > static analyzer and clang-tidy as -W flags in the compiler as the only
> > > integration option).
> >
> > This is why analysis is controlled by a separate flag. I see no reason
> > why clang-tidy checks would not behave similarly.
>
> Agreed, users who *can* integrate by analyzing during the build,
> should be able to do so.
>
> > > Running a separate build just to analyze forces
> > > the compiler to run code generation and produce object files and
> > > binaries, which are not necessary for static analysis.
> >
> > I don't see how that follows; -fsyntax-only exists to support that
> > sort of use case.
>
> The build system won't proceed with the build if .o files and code
> generation binaries are not produced.

Does it makes sense to keep support for compiler wrapper?
The compilation database is a much better approach.

Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 2:46 PM Christian Gagneraud <[hidden email]> wrote:

>
> On Wed, 14 Aug 2019 at 14:39, Dmitri Gribenko via cfe-dev
> <[hidden email]> wrote:
> >
> > On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
> > > >
> > > > On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> > > > >
> > > > > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > > > > <[hidden email]> wrote:
> > > > > >
> > > > > > Hi Artem,
> > > > > >
> > > > > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > > > > >
> > > > > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > > > > and make sure it's always possible for our users to take the best of
> > > > > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > > > > integratable into all UIs that already integrate the Static Analyzer.
> > > > > >
> > > > > > I like the idea of integrating the two tools more closely. From the
> > > > > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > > > > doing more or less the same thing, but somehow have completely
> > > > > > different workflows and integrations.
> > > > >
> > > > > Strong +1 from me!
> > > > >
> > > > > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > > > > checks as its own, maybe in a separate package. This is harder to do
> > > > > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > > > > can keep such integration under an off-by-default CMake flag (which
> > > > > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > > > > get compiled from the same sources simultaneously).
> > > > > >
> > > > > > I would like to suggest something different: move Clang Static
> > > > > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > > > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > > > > delegate to that binary for backwards compatibility.
> > > > >
> > > > > I am not keen on this approach. A stand-alone tool is harder for users
> > > > > to integrate into their workflows compared to the compiler itself,
> > > > > which is already (generally) a part of their existing workflow. It's
> > > > > far easier for a user to add a flag like -tidy to an existing clang
> > > > > execution than it is to insert clang-tidy into a complex build system.
> > > > > Personally, I would much rather see libClangTidy integrated into Clang
> > > > > and exposed via a driver flag, similar to how the static analyzer
> > > > > already works.
> > > >
> > > > Integrating by adding a flag to the build is only one possibility, and
> > > > it does not work for all users.
> > >
> > > I agree, my point was that a stand-alone tool doesn't work for all
> > > users either. For my needs, moving the static analyzer into clang-tidy
> > > makes things harder, not easier, whereas moving clang-tidy into the
> > > compiler proper makes things easier rather than harder.
> > >
> > > > Not everyone is willing to pay the
> > > > analysis slowdown during compilation (otherwise we would just expose
> > > > static analyzer and clang-tidy as -W flags in the compiler as the only
> > > > integration option).
> > >
> > > This is why analysis is controlled by a separate flag. I see no reason
> > > why clang-tidy checks would not behave similarly.
> >
> > Agreed, users who *can* integrate by analyzing during the build,
> > should be able to do so.
> >
> > > > Running a separate build just to analyze forces
> > > > the compiler to run code generation and produce object files and
> > > > binaries, which are not necessary for static analysis.
> > >
> > > I don't see how that follows; -fsyntax-only exists to support that
> > > sort of use case.
> >
> > The build system won't proceed with the build if .o files and code
> > generation binaries are not produced.
>
> Does it makes sense to keep support for compiler wrapper?
> The compilation database is a much better approach.

It depends whom we are optimizing for.

I think that generally, small projects will prefer direct compiler
integration (`clang -c -tidy -analyze`) or the compiler wrapper
approach.

Bigger projects with more complex build processes, a CI, pre-commit
testing etc. will probably prefer an approach based on the compilation
database.

If we want to cover everyone, I guess we have to maintain both approaches.

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 9:05 AM Dmitri Gribenko <[hidden email]> wrote:

>
> On Wed, Aug 14, 2019 at 2:46 PM Christian Gagneraud <[hidden email]> wrote:
> >
> > On Wed, 14 Aug 2019 at 14:39, Dmitri Gribenko via cfe-dev
> > <[hidden email]> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:
> > > >
> > > > On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
> > > > >
> > > > > On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
> > > > > >
> > > > > > On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> > > > > > <[hidden email]> wrote:
> > > > > > >
> > > > > > > Hi Artem,
> > > > > > >
> > > > > > > On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
> > > > > > > >
> > > > > > > > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > > > > > > > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > > > > > > > and make sure it's always possible for our users to take the best of
> > > > > > > > both worlds. In particular, i'd like to make Clang-Tidy easily
> > > > > > > > integratable into all UIs that already integrate the Static Analyzer.
> > > > > > >
> > > > > > > I like the idea of integrating the two tools more closely. From the
> > > > > > > user's point of view, I think, ClangTidy and Clang Static Analyzer are
> > > > > > > doing more or less the same thing, but somehow have completely
> > > > > > > different workflows and integrations.
> > > > > >
> > > > > > Strong +1 from me!
> > > > > >
> > > > > > > > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > > > > > > > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > > > > > > > checks as its own, maybe in a separate package. This is harder to do
> > > > > > > > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > > > > > > > sell to build it into the Clang binary. I'm open to suggestions here: we
> > > > > > > > can keep such integration under an off-by-default CMake flag (which
> > > > > > > > requires enabling compilation of clang-tools-extra) or we may even use
> > > > > > > > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > > > > > > > time it's actually fairly easy to avoid version conflicts, as they all
> > > > > > > > get compiled from the same sources simultaneously).
> > > > > > >
> > > > > > > I would like to suggest something different: move Clang Static
> > > > > > > Analyzer to clang-tools-extra. Build it either as a separate binary or
> > > > > > > compile it into the clang-tidy binary. Then let `clang -analyze`
> > > > > > > delegate to that binary for backwards compatibility.
> > > > > >
> > > > > > I am not keen on this approach. A stand-alone tool is harder for users
> > > > > > to integrate into their workflows compared to the compiler itself,
> > > > > > which is already (generally) a part of their existing workflow. It's
> > > > > > far easier for a user to add a flag like -tidy to an existing clang
> > > > > > execution than it is to insert clang-tidy into a complex build system.
> > > > > > Personally, I would much rather see libClangTidy integrated into Clang
> > > > > > and exposed via a driver flag, similar to how the static analyzer
> > > > > > already works.
> > > > >
> > > > > Integrating by adding a flag to the build is only one possibility, and
> > > > > it does not work for all users.
> > > >
> > > > I agree, my point was that a stand-alone tool doesn't work for all
> > > > users either. For my needs, moving the static analyzer into clang-tidy
> > > > makes things harder, not easier, whereas moving clang-tidy into the
> > > > compiler proper makes things easier rather than harder.
> > > >
> > > > > Not everyone is willing to pay the
> > > > > analysis slowdown during compilation (otherwise we would just expose
> > > > > static analyzer and clang-tidy as -W flags in the compiler as the only
> > > > > integration option).
> > > >
> > > > This is why analysis is controlled by a separate flag. I see no reason
> > > > why clang-tidy checks would not behave similarly.
> > >
> > > Agreed, users who *can* integrate by analyzing during the build,
> > > should be able to do so.
> > >
> > > > > Running a separate build just to analyze forces
> > > > > the compiler to run code generation and produce object files and
> > > > > binaries, which are not necessary for static analysis.
> > > >
> > > > I don't see how that follows; -fsyntax-only exists to support that
> > > > sort of use case.
> > >
> > > The build system won't proceed with the build if .o files and code
> > > generation binaries are not produced.
> >
> > Does it makes sense to keep support for compiler wrapper?
> > The compilation database is a much better approach.
>
> It depends whom we are optimizing for.
>
> I think that generally, small projects will prefer direct compiler
> integration (`clang -c -tidy -analyze`) or the compiler wrapper
> approach.
>
> Bigger projects with more complex build processes, a CI, pre-commit
> testing etc. will probably prefer an approach based on the compilation
> database.
>
> If we want to cover everyone, I guess we have to maintain both approaches.

I think you may be correct.

~Aaron

>
> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On Wed, Aug 14, 2019 at 12:24 AM Dmitri Gribenko via cfe-dev <[hidden email]> wrote:
I would like to suggest something different: move Clang Static
Analyzer to clang-tools-extra. Build it either as a separate binary or
compile it into the clang-tidy binary. Then let `clang -analyze`
delegate to that binary for backwards compatibility.

Speaking as an outsider in the peanut gallery who does very little static analysis, this is very appealing to me. It would save binary size in clang and make building and testing clang faster, if the developer isn't working in the static analyzer space.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev


On 8/14/19 5:43 AM, Aaron Ballman wrote:

> On Wed, Aug 14, 2019 at 8:38 AM Dmitri Gribenko <[hidden email]> wrote:
>> On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <[hidden email]> wrote:
>>> On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <[hidden email]> wrote:
>>>> On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <[hidden email]> wrote:
>>>>> On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
>>>>> <[hidden email]> wrote:
>>>>>> Hi Artem,
>>>>>>
>>>>>> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>>>>>>> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
>>>>>>> decreasing the gap between Clang-Tidy users and Static Analyzer users
>>>>>>> and make sure it's always possible for our users to take the best of
>>>>>>> both worlds. In particular, i'd like to make Clang-Tidy easily
>>>>>>> integratable into all UIs that already integrate the Static Analyzer.
>>>>>> I like the idea of integrating the two tools more closely. From the
>>>>>> user's point of view, I think, ClangTidy and Clang Static Analyzer are
>>>>>> doing more or less the same thing, but somehow have completely
>>>>>> different workflows and integrations.
>>>>> Strong +1 from me!
>>>>>
>>>>>>> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
>>>>>>> us: make the Static Analyzer load it as a library and expose Clang-Tidy
>>>>>>> checks as its own, maybe in a separate package. This is harder to do
>>>>>>> though, because Clang-Tidy lives in a separate repo and also it's a hard
>>>>>>> sell to build it into the Clang binary. I'm open to suggestions here: we
>>>>>>> can keep such integration under an off-by-default CMake flag (which
>>>>>>> requires enabling compilation of clang-tools-extra) or we may even use
>>>>>>> it as a run-time plugin (i mean, clang plugins are super wonky, but this
>>>>>>> time it's actually fairly easy to avoid version conflicts, as they all
>>>>>>> get compiled from the same sources simultaneously).
>>>>>> I would like to suggest something different: move Clang Static
>>>>>> Analyzer to clang-tools-extra. Build it either as a separate binary or
>>>>>> compile it into the clang-tidy binary. Then let `clang -analyze`
>>>>>> delegate to that binary for backwards compatibility.
>>>>> I am not keen on this approach. A stand-alone tool is harder for users
>>>>> to integrate into their workflows compared to the compiler itself,
>>>>> which is already (generally) a part of their existing workflow. It's
>>>>> far easier for a user to add a flag like -tidy to an existing clang
>>>>> execution than it is to insert clang-tidy into a complex build system.
>>>>> Personally, I would much rather see libClangTidy integrated into Clang
>>>>> and exposed via a driver flag, similar to how the static analyzer
>>>>> already works.
>>>> Integrating by adding a flag to the build is only one possibility, and
>>>> it does not work for all users.
>>> I agree, my point was that a stand-alone tool doesn't work for all
>>> users either. For my needs, moving the static analyzer into clang-tidy
>>> makes things harder, not easier, whereas moving clang-tidy into the
>>> compiler proper makes things easier rather than harder.
>>>
>>>> Not everyone is willing to pay the
>>>> analysis slowdown during compilation (otherwise we would just expose
>>>> static analyzer and clang-tidy as -W flags in the compiler as the only
>>>> integration option).
>>> This is why analysis is controlled by a separate flag. I see no reason
>>> why clang-tidy checks would not behave similarly.
>> Agreed, users who *can* integrate by analyzing during the build,
>> should be able to do so.
> Awesome; I also agree that there may be users who cannot go with this
> approach and we need to support them as well.

This is much more about delivery than it is about integration.

When it comes to integration, "clang file.cpp --analyze $FLAGS" isn't
anyhow easier to integrate than "clang-tidy file.cpp -- $FLAGS". It
needs to be a separate invocation anyway, because Clang cannot do normal
compilation when it's in its "--analyze" mode. So "clang --analyze" is
basically indistinguishable from a separate tool in this aspect.

But when it comes to delivering the tool to the users, there are a lot
more people who already have Clang delivered to them than people who
already have Clang-Tidy delivered to them. Not needing to ship
additional packages to our users but instead having the tool always
available at their fingertips is a nice advantage that we currently enjoy.


>>>> Running a separate build just to analyze forces
>>>> the compiler to run code generation and produce object files and
>>>> binaries, which are not necessary for static analysis.
>>> I don't see how that follows; -fsyntax-only exists to support that
>>> sort of use case.
>> The build system won't proceed with the build if .o files and code
>> generation binaries are not produced.
> I think that depends on the build system and the project, but you're
> right that some build systems won't handle this well.
>
> ~Aaron
>
>> Dmitri
>>
>>> ~Aaron
>>>
>>>> Dmitri
>>>>
>>>>>> Clang Static Analyzer is being developed in the clang repository for
>>>>>> historical reasons -- we didn't have clang-tools-extra at the time the
>>>>>> project was started. The "non-core" nature of Clang Static Analyzer is
>>>>>> also validated by the fact that it is an optional part of Clang -- we
>>>>>> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>>>>> FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
>>>>> and have always considered that a deficiency that has hindered
>>>>> adoption. For instance, running clang-tidy required separate effort to
>>>>> get it up on godbolt, whereas --analyze has worked there since the
>>>>> first time I tried it.
>>>>>
>>>>>> Long-term, my preference would be to have all static analysis tooling
>>>>>> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
>>>>>> analysis) to have a consistent UI -- including invocation, integration
>>>>>> with build systems, configuration strategy etc. While the
>>>>>> implementation of these tools is very different, I think the audience
>>>>>> is largely the same.
>>>>> I think this is a good goal.
>>>>>
>>>>> ~Aaron
>>>>>
>>>>>>> But regardless of how far do i end up going with such integration, first
>>>>>>> thing first: i'll anyway need to teach Clang-Tidy how to route its
>>>>>>> diagnostics through our diagnostic engine. This is also the only thing
>>>>>>> that's absolutely necessary; the rest can always be hacked up on the UI
>>>>>>> side.
>>>>>>>
>>>>>>> It's a great question why didn't we extend Clang's DiagnosticEngine to
>>>>>>> begin with, but instead made our own facility. I don't know the answer
>>>>>>> to that; this happened way before i even learned what an AST is :) We
>>>>>>> generally needed a much more sophisticated diagnostic engine because our
>>>>>>> reports are much more complicated. While we could try to unify these
>>>>>>> diagnostic engines completely, i don't think it's worth it.
>>>>>>>
>>>>>>> So i think it's more feasible to make Clang-Tidy's diag() method return
>>>>>>> a proxy object that mimics the DiagnosticBuilder interface but may also
>>>>>>> pump the diagnostics to an instance of the Static Analyzer's
>>>>>>> BugReporter. I hope this would avoid API breakages for clang-tidy checks.
>>>>>> I'm very concerned about introducing lookalike APIs that don't behave
>>>>>> exactly the same way as original ones. In LLVM, we are generally not
>>>>>> concerned with backwards compatibility for C++ APIs. Instead, we
>>>>>> should be looking to keep our code maintainable in the long run.
>>>>>>
>>>>>> With that in mind, my suggestion is to refactor ClangTidy to use
>>>>>> BugReporter instead of DiagnosticBuilder.
>>>>>>
>>>>>> Dmitri
>>>>>>
>>>>>> --
>>>>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>>>>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
>>>>>> _______________________________________________
>>>>>> cfe-dev mailing list
>>>>>> [hidden email]
>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>>>
>>>> --
>>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
>>
>>
>> --
>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On 8/14/19 9:20 AM, Reid Kleckner wrote:
On Wed, Aug 14, 2019 at 12:24 AM Dmitri Gribenko via cfe-dev <[hidden email]> wrote:
I would like to suggest something different: move Clang Static
Analyzer to clang-tools-extra. Build it either as a separate binary or
compile it into the clang-tidy binary. Then let `clang -analyze`
delegate to that binary for backwards compatibility.

Speaking as an outsider in the peanut gallery who does very little static analysis, this is very appealing to me. It would save binary size in clang and make building and testing clang faster, if the developer isn't working in the static analyzer space.

Generally we already have a cmake flag for this, -DCLANG_ENABLE_STATIC_ANALYZER=OFF (defaults to ON). This flag is definitely a must-have for people who try to make their clang binary as tiny as possible. But i understand that this flag is not exceptionally discoverable and a lot of people probably suffer unnecessarily long compile times simply because they didn't discover it.

That said, i suspect that unless you work exclusively on CodeGen, you probably do not want to turn off the Static Analyzer, because the Static Analyzer has some interesting tests that you can accidentally break when you work on anything else (Preprocessor, Lexer, AST, Sema...). In particular, Richard Smith regularly touches our code :)

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
Ugh, i just realized this.

Clang-Tidy doesn't really need BugReporter; all it needs is
PathDiagnosticConsumer. (the word "Path" is rudimentary)

It should be perfectly comfortable to build PathDiagnostics by hand - it
maps nicely to how you already build your warnings and notes. There's no
need to force you to do the job of packing your notes into an auxiliary
BugReport object and then use BugReporter to unwrap it back into a
PathDiagnostic. It makes a huge lot of sense for path-sensitive reports
to de-duplicate the work of turning a report into a PathDiagnostic, but
it doesn't really mean much for path-insensitive reports.

Change of plans, i guess.

On 8/14/19 12:23 AM, Dmitri Gribenko wrote:

> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
>> decreasing the gap between Clang-Tidy users and Static Analyzer users
>> and make sure it's always possible for our users to take the best of
>> both worlds. In particular, i'd like to make Clang-Tidy easily
>> integratable into all UIs that already integrate the Static Analyzer.
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.
>
>> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
>> us: make the Static Analyzer load it as a library and expose Clang-Tidy
>> checks as its own, maybe in a separate package. This is harder to do
>> though, because Clang-Tidy lives in a separate repo and also it's a hard
>> sell to build it into the Clang binary. I'm open to suggestions here: we
>> can keep such integration under an off-by-default CMake flag (which
>> requires enabling compilation of clang-tools-extra) or we may even use
>> it as a run-time plugin (i mean, clang plugins are super wonky, but this
>> time it's actually fairly easy to avoid version conflicts, as they all
>> get compiled from the same sources simultaneously).
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.
>
> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.
>
>> But regardless of how far do i end up going with such integration, first
>> thing first: i'll anyway need to teach Clang-Tidy how to route its
>> diagnostics through our diagnostic engine. This is also the only thing
>> that's absolutely necessary; the rest can always be hacked up on the UI
>> side.
>>
>> It's a great question why didn't we extend Clang's DiagnosticEngine to
>> begin with, but instead made our own facility. I don't know the answer
>> to that; this happened way before i even learned what an AST is :) We
>> generally needed a much more sophisticated diagnostic engine because our
>> reports are much more complicated. While we could try to unify these
>> diagnostic engines completely, i don't think it's worth it.
>>
>> So i think it's more feasible to make Clang-Tidy's diag() method return
>> a proxy object that mimics the DiagnosticBuilder interface but may also
>> pump the diagnostics to an instance of the Static Analyzer's
>> BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.
>
> Dmitri
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev


On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <[hidden email]> wrote:
Ugh, i just realized this.

Clang-Tidy doesn't really need BugReporter; all it needs is
PathDiagnosticConsumer. (the word "Path" is rudimentary)

Maybe its time to rename it! :)
 
It should be perfectly comfortable to build PathDiagnostics by hand - it
maps nicely to how you already build your warnings and notes. There's no
need to force you to do the job of packing your notes into an auxiliary
BugReport object and then use BugReporter to unwrap it back into a
PathDiagnostic. It makes a huge lot of sense for path-sensitive reports
to de-duplicate the work of turning a report into a PathDiagnostic, but
it doesn't really mean much for path-insensitive reports.
 
A bit of an unrelated question, does clang-tidy house CFG-based checks, or only AST based ones? For our upcoming reaching definitions analysis, we may even want a CFGBugReport class.
 
Change of plans, i guess.

In any case, your changes to BugReport gave much needed clarity to the code! :)
 
On 8/14/19 12:23 AM, Dmitri Gribenko wrote:
> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
>> decreasing the gap between Clang-Tidy users and Static Analyzer users
>> and make sure it's always possible for our users to take the best of
>> both worlds. In particular, i'd like to make Clang-Tidy easily
>> integratable into all UIs that already integrate the Static Analyzer.
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.
>
>> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
>> us: make the Static Analyzer load it as a library and expose Clang-Tidy
>> checks as its own, maybe in a separate package. This is harder to do
>> though, because Clang-Tidy lives in a separate repo and also it's a hard
>> sell to build it into the Clang binary. I'm open to suggestions here: we
>> can keep such integration under an off-by-default CMake flag (which
>> requires enabling compilation of clang-tools-extra) or we may even use
>> it as a run-time plugin (i mean, clang plugins are super wonky, but this
>> time it's actually fairly easy to avoid version conflicts, as they all
>> get compiled from the same sources simultaneously).
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.
>
> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.
>
>> But regardless of how far do i end up going with such integration, first
>> thing first: i'll anyway need to teach Clang-Tidy how to route its
>> diagnostics through our diagnostic engine. This is also the only thing
>> that's absolutely necessary; the rest can always be hacked up on the UI
>> side.
>>
>> It's a great question why didn't we extend Clang's DiagnosticEngine to
>> begin with, but instead made our own facility. I don't know the answer
>> to that; this happened way before i even learned what an AST is :) We
>> generally needed a much more sophisticated diagnostic engine because our
>> reports are much more complicated. While we could try to unify these
>> diagnostic engines completely, i don't think it's worth it.
>>
>> So i think it's more feasible to make Clang-Tidy's diag() method return
>> a proxy object that mimics the DiagnosticBuilder interface but may also
>> pump the diagnostics to an instance of the Static Analyzer's
>> BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.
>
> Dmitri
>


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev

On 9/3/19 5:31 PM, Kristóf Umann wrote:
On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <[hidden email]> wrote:
Ugh, i just realized this.

Clang-Tidy doesn't really need BugReporter; all it needs is
PathDiagnosticConsumer. (the word "Path" is rudimentary)

Maybe its time to rename it! :)
 
It should be perfectly comfortable to build PathDiagnostics by hand - it
maps nicely to how you already build your warnings and notes. There's no
need to force you to do the job of packing your notes into an auxiliary
BugReport object and then use BugReporter to unwrap it back into a
PathDiagnostic. It makes a huge lot of sense for path-sensitive reports
to de-duplicate the work of turning a report into a PathDiagnostic, but
it doesn't really mean much for path-insensitive reports.
 
A bit of an unrelated question, does clang-tidy house CFG-based checks, or only AST based ones? For our upcoming reaching definitions analysis, we may even want a CFGBugReport class.

For must-problems i suspect that the really good way to present them to the user would be to present all reports simultaneously (eg., as part of a single HTML report file). For example: "this variable is never changed and may be declared as const (1) because this if-branch in which it's assigned is in fact dead (2) because its condition always evaluates to true (3)". These may be three bug reports emitted by three different CFG-based checkers, however this whole logical chain of "(1) because (2) because (3)" may be impossible to comprehend unless we show all three reports together. That could be fun to write a BugReporter for.

Change of plans, i guess.

In any case, your changes to BugReport gave much needed clarity to the code! :)

Sure, i still plan to land it.

 
On 8/14/19 12:23 AM, Dmitri Gribenko wrote:
> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
>> decreasing the gap between Clang-Tidy users and Static Analyzer users
>> and make sure it's always possible for our users to take the best of
>> both worlds. In particular, i'd like to make Clang-Tidy easily
>> integratable into all UIs that already integrate the Static Analyzer.
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.
>
>> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
>> us: make the Static Analyzer load it as a library and expose Clang-Tidy
>> checks as its own, maybe in a separate package. This is harder to do
>> though, because Clang-Tidy lives in a separate repo and also it's a hard
>> sell to build it into the Clang binary. I'm open to suggestions here: we
>> can keep such integration under an off-by-default CMake flag (which
>> requires enabling compilation of clang-tools-extra) or we may even use
>> it as a run-time plugin (i mean, clang plugins are super wonky, but this
>> time it's actually fairly easy to avoid version conflicts, as they all
>> get compiled from the same sources simultaneously).
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.
>
> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.
>
>> But regardless of how far do i end up going with such integration, first
>> thing first: i'll anyway need to teach Clang-Tidy how to route its
>> diagnostics through our diagnostic engine. This is also the only thing
>> that's absolutely necessary; the rest can always be hacked up on the UI
>> side.
>>
>> It's a great question why didn't we extend Clang's DiagnosticEngine to
>> begin with, but instead made our own facility. I don't know the answer
>> to that; this happened way before i even learned what an AST is :) We
>> generally needed a much more sophisticated diagnostic engine because our
>> reports are much more complicated. While we could try to unify these
>> diagnostic engines completely, i don't think it's worth it.
>>
>> So i think it's more feasible to make Clang-Tidy's diag() method return
>> a proxy object that mimics the DiagnosticBuilder interface but may also
>> pump the diagnostics to an instance of the Static Analyzer's
>> BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.
>
> Dmitri
>



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
On Wed, Sep 4, 2019 at 4:03 AM Artem Dergachev <[hidden email]> wrote:

On 9/3/19 5:31 PM, Kristóf Umann wrote:
On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <[hidden email]> wrote:
Ugh, i just realized this.

Clang-Tidy doesn't really need BugReporter; all it needs is
PathDiagnosticConsumer. (the word "Path" is rudimentary)

Maybe its time to rename it! :)
 
It should be perfectly comfortable to build PathDiagnostics by hand - it
maps nicely to how you already build your warnings and notes. There's no
need to force you to do the job of packing your notes into an auxiliary
BugReport object and then use BugReporter to unwrap it back into a
PathDiagnostic. It makes a huge lot of sense for path-sensitive reports
to de-duplicate the work of turning a report into a PathDiagnostic, but
it doesn't really mean much for path-insensitive reports.
 
A bit of an unrelated question, does clang-tidy house CFG-based checks, or only AST based ones?
There are a few CFG-based checks. Some of them would benefit from a way to represent execution path in the same diagnostic. The way it can currently be done is using diagnostic notes (CSA diagnostics are represented the same way, when converted from PathDiagnostic in clang-tidy (the code is around clang-tidy/ClangTidy.cpp:70). 
 
For our upcoming reaching definitions analysis, we may even want a CFGBugReport class.

For must-problems i suspect that the really good way to present them to the user would be to present all reports simultaneously (eg., as part of a single HTML report file). For example: "this variable is never changed and may be declared as const (1) because this if-branch in which it's assigned is in fact dead (2) because its condition always evaluates to true (3)". These may be three bug reports emitted by three different CFG-based checkers, however this whole logical chain of "(1) because (2) because (3)" may be impossible to comprehend unless we show all three reports together. That could be fun to write a BugReporter for.

Change of plans, i guess.

In any case, your changes to BugReport gave much needed clarity to the code! :)

Sure, i still plan to land it.

 
On 8/14/19 12:23 AM, Dmitri Gribenko wrote:
> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <[hidden email]> wrote:
>> As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
>> decreasing the gap between Clang-Tidy users and Static Analyzer users
>> and make sure it's always possible for our users to take the best of
>> both worlds. In particular, i'd like to make Clang-Tidy easily
>> integratable into all UIs that already integrate the Static Analyzer.
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.
>
>> Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
>> us: make the Static Analyzer load it as a library and expose Clang-Tidy
>> checks as its own, maybe in a separate package. This is harder to do
>> though, because Clang-Tidy lives in a separate repo and also it's a hard
>> sell to build it into the Clang binary. I'm open to suggestions here: we
>> can keep such integration under an off-by-default CMake flag (which
>> requires enabling compilation of clang-tools-extra) or we may even use
>> it as a run-time plugin (i mean, clang plugins are super wonky, but this
>> time it's actually fairly easy to avoid version conflicts, as they all
>> get compiled from the same sources simultaneously).
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.
>
> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.
>
>> But regardless of how far do i end up going with such integration, first
>> thing first: i'll anyway need to teach Clang-Tidy how to route its
>> diagnostics through our diagnostic engine. This is also the only thing
>> that's absolutely necessary; the rest can always be hacked up on the UI
>> side.
>>
>> It's a great question why didn't we extend Clang's DiagnosticEngine to
>> begin with, but instead made our own facility. I don't know the answer
>> to that; this happened way before i even learned what an AST is :) We
>> generally needed a much more sophisticated diagnostic engine because our
>> reports are much more complicated. While we could try to unify these
>> diagnostic engines completely, i don't think it's worth it.
>>
>> So i think it's more feasible to make Clang-Tidy's diag() method return
>> a proxy object that mimics the DiagnosticBuilder interface but may also
>> pump the diagnostics to an instance of the Static Analyzer's
>> BugReporter. I hope this would avoid API breakages for clang-tidy checks.
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.
>
> Dmitri
>



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
On Wed, Sep 4, 2019 at 1:17 AM Artem Dergachev <[hidden email]> wrote:
> Clang-Tidy doesn't really need BugReporter; all it needs is
> PathDiagnosticConsumer. (the word "Path" is rudimentary)
>
> It should be perfectly comfortable to build PathDiagnostics by hand - it
> maps nicely to how you already build your warnings and notes.

I like this approach better, however, I also think that
PathDiagnostic's API is a lot more complex than the current `diag()
<<` in ClangTidy. For example, it requires the caller to specify the
checker name. It also requires to specify a ton of locations and
decls, whereas we can probably infer most of the locations and decls
from just one location.

Again, something I would really like to see written down (and us agree
upon) is the data model for a diagnostic. When we know what data we
need to store, we can figure out how to store it, and a convenient API
to populate it.

Dmitri

--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <[hidden email]>*/
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12