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
|

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

Kristóf Umann via cfe-dev
Sorry about the delayed reply, I only saw this thread when it popped up again today.

On Wed, Aug 14, 2019 at 7:37 PM Artem Dergachev via cfe-dev <[hidden email]> wrote:
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.
A bit +1 to this from me as well. 

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.

I'm aware of this flag, but it doesn't quite do what I want: I want clang-tidy to have the static analyzer, but I don't want my compiler to have it. It'd be nice if there was a setting that allowed putting the analyzer in clang-tidy but not in clang, and to make `clang -analyze` shell out to clang-tidy for backwards compat.

I agree that the analyzer is in the compiler mainly for historical reasons, and that we wouldn't move it form clang-tidy into the compiler if it was only in clang-tidy. This suggests that that's the future we should work towards.
 

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

_______________________________________________
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
Maybe i should do something similar to SemaDiagnosticBuilder, i.e.
sub-class DiagnosticBuilder and steal diagnostics from it in my
superclass destructor, so that to convert them to our path diagnostics,
WDYT?

That's not going to break the source compatibility (apart from the few
checks where people put their DiagnosticBuilder into a local variable,
they'll have to change the type of that variable), but neither would it
introduce a look-alike interface because it's still literally the same
interface and it's already kind of expected to be polymorphic.

On 9/6/19 6:44 AM, Dmitri Gribenko wrote:

> 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
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
P.S. We could provide such diagnostic builder to our own AST-based
checkers as well. That way we can see if it's indeed a better API for
emitting path-insensitive warnings than BugReporter::EmitBasicReport(),
which is pretty likely. And if so, i'll readily recommend it for use in
any path-insensitive tool that emits warnings. This would also be an
indication that our API is the best in long term.

P.P.S. Such API would in its out-of-the-box shape not allow Clang-Tidy
to add the typical path-sensitive pieces (event piece, control flow
piece, etc.) to the report. But if you ever want to do this sort of
stuff, it should be a matter of simply extending our superclass a little
bit.

On 10.09.2019 18:17, Artem Dergachev wrote:

> Maybe i should do something similar to SemaDiagnosticBuilder, i.e.
> sub-class DiagnosticBuilder and steal diagnostics from it in my
> superclass destructor, so that to convert them to our path
> diagnostics, WDYT?
>
> That's not going to break the source compatibility (apart from the few
> checks where people put their DiagnosticBuilder into a local variable,
> they'll have to change the type of that variable), but neither would
> it introduce a look-alike interface because it's still literally the
> same interface and it's already kind of expected to be polymorphic.
>
> On 9/6/19 6:44 AM, Dmitri Gribenko wrote:
>> 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
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 10.09.2019 18:17, Artem Dergachev wrote:
> > Maybe i should do something similar to SemaDiagnosticBuilder, i.e.
> > sub-class DiagnosticBuilder and steal diagnostics from it in my
> > superclass destructor, so that to convert them to our path
> > diagnostics, WDYT?

That sounds like abuse of inheritance :) Subclassing DiagnosticBuilder
to hijack its output (which is stored in DiagnosticsEngine BTW!) is
too clever. The current Clang's diagnostics subsystem has a lot of
quirks already. I think diagnostics that start from DiagnosticBuilder
should be reported by DiagnosticsEngine to the DiagnosticConsumer.

If we want to reuse DiagnosticBuilder, WDYT about a custom
DiagnosticConsumer instead?

On Wed, Sep 11, 2019 at 6:26 AM Artem Dergachev <[hidden email]> wrote:

>
> P.S. We could provide such diagnostic builder to our own AST-based
> checkers as well. That way we can see if it's indeed a better API for
> emitting path-insensitive warnings than BugReporter::EmitBasicReport(),
> which is pretty likely. And if so, i'll readily recommend it for use in
> any path-insensitive tool that emits warnings. This would also be an
> indication that our API is the best in long term.
>
> P.P.S. Such API would in its out-of-the-box shape not allow Clang-Tidy
> to add the typical path-sensitive pieces (event piece, control flow
> piece, etc.) to the report. But if you ever want to do this sort of
> stuff, it should be a matter of simply extending our superclass a little
> bit.

That does sound interesting, however, I'd prefer to try to figure out
ways to converge the two APIs (Clang's diagnostic subsystem and
BugReporter) instead of building more ways of doing the same stuff.
However, I'm not sure about the best way to do it. These APIs are
definitely built with different thoughts towards performance. Clang's
diagnostic subsystem is super-optimized to the point of trying hard to
not even dynamically allocate memory; all this while trying to present
an OOP interface, which, however, leaks the underlying implementation
by exposing the concepts of "in-flight diagnostic", "delayed
diagnostic" etc. Static Analyzer diagnostic subsystem is more OOP-like
in its implementation as well, with various path pieces for example.

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 9/12/19 4:55 AM, Dmitri Gribenko wrote:

>> On 10.09.2019 18:17, Artem Dergachev wrote:
>>> Maybe i should do something similar to SemaDiagnosticBuilder, i.e.
>>> sub-class DiagnosticBuilder and steal diagnostics from it in my
>>> superclass destructor, so that to convert them to our path
>>> diagnostics, WDYT?
> That sounds like abuse of inheritance :) Subclassing DiagnosticBuilder
> to hijack its output (which is stored in DiagnosticsEngine BTW!) is
> too clever. The current Clang's diagnostics subsystem has a lot of
> quirks already. I think diagnostics that start from DiagnosticBuilder
> should be reported by DiagnosticsEngine to the DiagnosticConsumer.
>
> If we want to reuse DiagnosticBuilder, WDYT about a custom
> DiagnosticConsumer instead?

Wait, we already have that? Nice! Will go that way then.

>
> On Wed, Sep 11, 2019 at 6:26 AM Artem Dergachev <[hidden email]> wrote:
>> P.S. We could provide such diagnostic builder to our own AST-based
>> checkers as well. That way we can see if it's indeed a better API for
>> emitting path-insensitive warnings than BugReporter::EmitBasicReport(),
>> which is pretty likely. And if so, i'll readily recommend it for use in
>> any path-insensitive tool that emits warnings. This would also be an
>> indication that our API is the best in long term.
>>
>> P.P.S. Such API would in its out-of-the-box shape not allow Clang-Tidy
>> to add the typical path-sensitive pieces (event piece, control flow
>> piece, etc.) to the report. But if you ever want to do this sort of
>> stuff, it should be a matter of simply extending our superclass a little
>> bit.
> That does sound interesting, however, I'd prefer to try to figure out
> ways to converge the two APIs (Clang's diagnostic subsystem and
> BugReporter) instead of building more ways of doing the same stuff.
> However, I'm not sure about the best way to do it. These APIs are
> definitely built with different thoughts towards performance. Clang's
> diagnostic subsystem is super-optimized to the point of trying hard to
> not even dynamically allocate memory; all this while trying to present
> an OOP interface, which, however, leaks the underlying implementation
> by exposing the concepts of "in-flight diagnostic", "delayed
> diagnostic" etc. Static Analyzer diagnostic subsystem is more OOP-like
> in its implementation as well, with various path pieces for example.
>
> Dmitri
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12