Page MenuHomePhabricator

[analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.
ClosedPublic

Authored by NoQ on Sep 10 2019, 4:59 PM.

Details

Summary

We're finally more or less ready to allow users outside of the Static Analyzer to take advantage of path diagnostic consumers for emitting their warnings in different formats. I didn't really try to do that in practice, but most of the necessary APIs should be at least available now.

I'm not planning to convert Clang-Tidy to use these APIs directly because i understand that they're more complicated than Clang-Tidy really needs them to be. I'll either simplify these APIs (if they indeed can be simplified) or (more likely) introduce a convenient wrapper for easily building path diagnostics.

I'm not sure about renaming these classes to get rid of the "Path" prefix. These diagnostics aren't necessarily displaying the path (and they never did), but they're flexible enough for displaying paths and that's still the primary difference between our path diagnostics and ordinary diagnostics. Also if we just call them "Diagnostics" it'll be too generic and hard to distinguish from the Clang diagnostic engine. Suggestions are welcome :)

Diff Detail

Event Timeline

NoQ created this revision.Sep 10 2019, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 4:59 PM
gribozavr requested changes to this revision.Sep 11 2019, 6:04 AM
gribozavr added inline comments.
clang/include/clang/Analysis/PathDiagnosticConsumers.h
39

Does this code declare e.g., createPlistDiagnosticConsumer? That function is defined in libStaticAnalyzer. Declaring it libAnalysis is not appropriate, and effectively introduces a layering violation. Users who link against libAnalysis will get link errors if they don't link against libStaticAnalyzer.

This revision now requires changes to proceed.Sep 11 2019, 6:04 AM
NoQ marked an inline comment as done.Sep 11 2019, 11:23 AM
NoQ added inline comments.
clang/include/clang/Analysis/PathDiagnosticConsumers.h
39

Oh crap, i forgot to move them.

NoQ added a comment.Sep 11 2019, 11:53 AM

Hmm, does anybody want me to write an example tool that emits path diagnostics but doesn't link to libStaticAnalyzer*?

NoQ updated this revision to Diff 219829.Sep 11 2019, 4:19 PM

Unforget to actually move the consumer implementations to libAnalysis, not just their headers.

Casually rename ClangDiagPathDiagConsumer to TextDiagnostics according to the local naming tradition.

gribozavr added inline comments.Sep 12 2019, 2:33 AM
clang/include/clang/Analysis/PathDiagnosticConsumers.def
23

80 columns?

clang/include/clang/Analysis/PathDiagnosticConsumers.h
38

Maybe not quite an appropriate comment for this patch, but I just realized that this factory function returns void, which confused me -- where does the new consumer go? Only after reading an implementation of one of these functions I understood that they add the new consumer to C, which is a vector (whose type is conveniently hidden by a typedef -- why?)

I'd suggest to make these factories more like factories, and make them return a unique_ptr that the caller can put wherever they need. Of course, in a separate patch.

I'd also suggest to remove the PathDiagnosticConsumers typedef altogether. It is used just a couple of times in the code, and obfuscates code more than it helps.

clang/lib/Analysis/PlistHTMLDiagnostics.cpp
27 ↗(On Diff #219829)

Passing it by value would have made it less subtle, and probably as efficient.

clang/lib/Analysis/TextDiagnostics.cpp
23 ↗(On Diff #219829)

"TextDiagnosticsConsumer"

Or even better, "TextPathDiagnosticsConsumer".

Same for the file name. Same for other files that define diagnostics consumers.

Clarity is really important here. Very few places in the code will mention this type by name, however, it is really important to distinguish this diagnostics infrastructure from the rest of Clang's diagnostics.

I'm also starting to wonder whether this diagnostics infrastructure should be in its own library, not in libAnalysis -- what do you think?

In D67422#1667079, @NoQ wrote:

Casually rename ClangDiagPathDiagConsumer to TextDiagnostics according to the local naming tradition.

Yoink. You could be facing a rebasing nightmare here :)

NoQ updated this revision to Diff 282798.Aug 3 2020, 9:20 PM
NoQ marked an inline comment as done.
NoQ added a reviewer: vsavchenko.

Aaaaand rebase. Addressed comments. With @Szelethus's refactoring work the patch actually looks much cleaner than it used to. One bit that's not directly related but i decided to keep it from the original patch was moving the plist-html diagnostic builder function into its own file.

NoQ added inline comments.Aug 3 2020, 9:20 PM
clang/include/clang/Analysis/PathDiagnosticConsumers.h
38

Will do!

clang/lib/Analysis/TextDiagnostics.cpp
23 ↗(On Diff #219829)

Renamed all the stuff.

I'm also starting to wonder whether this diagnostics infrastructure should be in its own library, not in libAnalysis -- what do you think?

Maybe! libAnalysis is currently a mess; it's basically a collection of all static-analyzer-ish things that are also used outside of the static analyzer. That's indeed not a good definition for a library. It incorporates a number of completely unrelated things.

In D67422#2192407, @NoQ wrote:

One bit that's not directly related but i decided to keep it from the original patch was moving the plist-html diagnostic builder function into its own file.

That is a great call indeed.

I'm also starting to wonder whether this diagnostics infrastructure should be in its own library, not in libAnalysis -- what do you think?

Maybe! libAnalysis is currently a mess; it's basically a collection of all static-analyzer-ish things that are also used outside of the static analyzer. That's indeed not a good definition for a library. It incorporates a number of completely unrelated things.

Not to mention my new plugins were knocked around all over the place, and eventually they also landed in it. I think libAnalysis is indeed well overdue for a revision, maybe sooner rather then later. There aren't too many other changes in this patch, maybe its worth doing this right away? I don't insist.

Other than parameter names, it looks totally reasonable to me.

clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
69

Maybe while we are here, we can change the names to match the code style (aka capitalize)?
It seems like all other functions have similar problems. It's no biggie, but still...

NOTE: I really wanted to try this new feature of Phabricator 😊
clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
26

As long as it is a new function, I guess we should definitely keep names according to the Coding Standards

NoQ updated this revision to Diff 283461.Aug 5 2020, 5:55 PM
NoQ marked 2 inline comments as done.

Fxd.

clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
26

It's not new but sure!

vsavchenko added inline comments.Aug 6 2020, 1:57 AM
clang/lib/Analysis/TextPathDiagnosticConsumer.cpp
1

TextPathDiagnosticConsumer.cpp?

35

TextPathDiagnosticConsumer?

NoQ updated this revision to Diff 283993.Aug 7 2020, 12:33 PM
NoQ marked 2 inline comments as done.

Whoops fxd!

NoQ added inline comments.Aug 13 2020, 12:06 AM
clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
28–32

I just realized that this code creates 5 consumers: 1 html consumer, 1 plist consumer, 3 text consumers. This isn't a regression due to this patch... but damn!

The patch looks great. I guess the only remaining discussion remains as to whether this should be in libAnalysis, or somewhere else. Here is my take: Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized variable warnings) are static analyzers within the same infrastructure, it makes sense for them to have a common library. I see that diagnostics aren't really analyses. libFrontend or libDriver, which, as I understand it contains most the diagnostics machinery for clang aren't viable alternatives because of the library dependencies. So, I think if libAnalysis was called libStaticAnalysisCommon, we would be golden, but that would screw over downstream developers for negligible gain. While I'm not terribly experiences in library layout design, for the time being, the move to libAnalysis seems appropriate.

I see that we're still heavily tied to StaticAnalyzer headers. Doesn't that violate the module system, as libAnalysis is a dependency of libStaticAnalyzerCore? AnalyzerOptions and AnalysisConsumer (that last one in particular) seem deeply integrated into these consumers.

clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
28–32

Oh wow. That is definitely my mistake. The issue is that detailed diagnostics (-analyzer-output=text) and the minimal one (analyzer-output=text-minimal) should be mutually exclusive. I'll try to think about a solution. Maybe some Profile thing might work.

vsavchenko accepted this revision.Aug 13 2020, 9:58 AM

Awesome, looks great!

NoQ updated this revision to Diff 285417.Aug 13 2020, 10:07 AM

The patch looks great. I guess the only remaining discussion remains as to whether this should be in libAnalysis, or somewhere else. Here is my take: Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized variable warnings) are static analyzers within the same infrastructure, it makes sense for them to have a common library. I see that diagnostics aren't really analyses. libFrontend or libDriver, which, as I understand it contains most the diagnostics machinery for clang aren't viable alternatives because of the library dependencies. So, I think if libAnalysis was called libStaticAnalysisCommon, we would be golden, but that would screw over downstream developers for negligible gain. While I'm not terribly experiences in library layout design, for the time being, the move to libAnalysis seems appropriate.

I agree that there's something here and also that it's not that urgent/critical to rename things at this point. It's not that people suffer horribly from having to link to only some of these things.

I see that we're still heavily tied to StaticAnalyzer headers. Doesn't that violate the module system, as libAnalysis is a dependency of libStaticAnalyzerCore? AnalyzerOptions and AnalysisConsumer (that last one in particular) seem deeply integrated into these consumers.

Mmm, these are dead includes now. Reliance on AnalyzerOptions was supposed to be eliminated since D67420 and these are in fact rebase conflicts. Fxd.

NoQ added inline comments.Aug 13 2020, 10:08 AM
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310

This thing still worries me but this definitely isn't a link-time dependency.

Szelethus added inline comments.Aug 13 2020, 5:02 PM
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310
Szelethus accepted this revision.Aug 14 2020, 11:36 AM

Unless anyone else objects, I'm happy to see this landed! Nice work! ^-^

In D67422#2216137, @NoQ wrote:

I agree that there's something here and also that it's not that urgent/critical to rename things at this point. It's not that people suffer horribly from having to link to only some of these things.

In the long run, if we end up piling more of the common stuff in libAnalysis, it might be worth to create a new library.

NoQ added inline comments.Oct 12 2020, 1:25 PM
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310

Ok, what's the proper solution here? This is clearly a layering violation now; this generic diagnostic consumer shouldn't know anything about the Static Analyzer specifically. I guess we could separate it into an independent polymorphic object ("DescriptionGetter" or something like that) that the Static Analyzer would instantiate manually. Or ideally we could ship this information with the bug report itself.

I'll add a FIXME and try to reproduce potential modules problems locally.

Szelethus added inline comments.
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310

I am puzzled myself :/ Maybe we could ask @aaron.ballman, since he landed most of these changes back in the day?

This revision was not accepted when it landed; it landed in state Needs Review.Oct 13 2020, 10:53 AM
This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Oct 13 2020, 10:56 AM
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310

I fixed a couple other bugs that caused modules build to fail before pushing but this one wasn't a problem.

aaron.ballman added inline comments.Oct 13 2020, 10:58 AM
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
310

The original code was added because it's needed for the required SARIF output. At the time, it didn't seem like it was a layering violation because the SARIF output was limited to just the clang static analyzer diagnostics -- however, I agree that work needs to be done here now. For instance, one of the issues with the SARIF output is that it only captures output from the static analyzer and doesn't report diagnostics from Clang itself, IIRC. We'll need to solve this for clang-tidy diagnostics as well.