This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Move the text output type to its own file, move code to PathDiagnosticConsumer creator functions
ClosedPublic

Authored by Szelethus on Mar 20 2020, 9:19 AM.

Details

Summary

TableGen and .def files (which are meant to be used with the preprocessor) come with obvious downsides. One of those issues is that generated switch-case branches have to be identical. This pushes corner cases either to an outer code block, or into the generated code.

Inspect the removed code in AnalysisConsumer::DigestAnalyzerOptions. You can see how corner cases like a not existing output file, the analysis output type being set to PD_NONE, or whether to complement the output with additional diagnostics on stderr lay around the preprocessor generated code. This is a bit problematic, as to how to deal with such errors is not in the hands of the users of this interface (those implementing output types, like PlistDiagnostics etc).

This patch changes this by moving these corner cases into the generated code, more specifically, into the called functions. In addition, I introduced a new output type for convenience purposes, PD_TEXT_MINIMAL, which always existed conceptually, but never in the actual Analyses.def file. This refactoring allowed me to move TextDiagnostics (renamed from ClangDiagPathDiagConsumer) to its own file, which it really deserved.

Also, those that had the misfortune to gaze upon Analyses.def will probably enjoy the sight that a clang-format did on it.

Diff Detail

Event Timeline

Szelethus created this revision.Mar 20 2020, 9:19 AM
Szelethus marked an inline comment as done.Mar 20 2020, 9:49 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
152–154

Will need to handle PD_NONE, or clang-tidy will crash all over the place.

Szelethus updated this revision to Diff 251973.Mar 23 2020, 2:59 AM

Unbreak clang-tidy.

martong added inline comments.Mar 23 2020, 6:33 AM
clang/include/clang/StaticAnalyzer/Core/Analyses.def
17–18

Yay! I am a big fun of the 80 col limit! :)

61

Just out of curiosity: is there a way to know which one is the default?

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
140

Actually, now I wonder where should we document the default output type ... I am pretty sure it should be done somewhere where it is obvious, maybe in Analyses.def?

142

Man, I understand you pain now, this is really muddled, do you plan to sort this out in an upcoming patch?

148

Would be an assert better here?

148
// if the output
 // directory isn't specified, it acts like if it was in the minimal text
 // output mode.

So, why don't we do this check before calling createTextMinimalPathDiagnosticConsumer ?

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
2

Is this just a blind copy (and rename) from AnalysisConsumer, or should I take a deeper look into anything here?

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
160–161

Some build bots will not compile if you handle all cases in the swtich and you still have a default, beware.

352

Either the ; is redundant or this is not the end of a namespace.

Szelethus marked 15 inline comments as done.Mar 23 2020, 7:50 AM

Thanks, @martong!

clang/include/clang/StaticAnalyzer/Core/Analyses.def
61

Not from this file, unfortunately. You need to go to AnalyzerOptions.h. It would be great if we were able to list the possible options as well as the default value (similarly to -analyzer-configs), but I don't have plant to do it myself anytime soon.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
140

AnalyzerOptions.h.

142

Allow me to indulge you with the relieving fix found in D76510 :)

148
if (OutputDir.empty())
  return;

createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));

Otherwise this would be a functional and breaking change. Mind that in this patch, HTML is still our default output type. If I were to move the text diagnostics below this line, we would no longer emit any diagnostics to stderr. I actually had the same idea, got ~670 breaking test :^)

if (OutputDir.empty()) {
  createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
  return;
}

C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));

With this code, we would no longer emit to stderr, only to HTML, should an output directory be specified.

148

Would be an assert better here?

No, the point of the patch is to handle user errors (such as setting the output type to html but forgetting to supply the output location) in these factory functions. An assert would've been appropriate if I left AnalysisConsumer::DigestAnalyzerOptions untouched, as previously, that is where we checked such inconsistencies.

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp
2

Well, almost:

  • Moved the initialization of the boolean fields into the constructor (these were previously done with setters in AnalysisConsumer::DigestAnalyzerOptions)
  • Renamed the DiagnosticsEngine & field from Diag to DiagEng (because this entire file is about diagnostics, duh)
  • Instead of IncludePath being set according to the value of Opts->AnalysisDiagOpt == PD_TEXT, it depends on whether createTextMinimalPathDiagnosticConsumer or createTextPathDiagnosticConsumer is called. But as to which of these is called, of course, depends on Opts->AnalysisDiagOpt == PD_TEXT.

This is such an essential part of the analyzer, almost every test file implicitly checks this code as well, so I don't think there's much to worry about here :)

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
160–161

Oh, thanks!

352

clangd >.<

martong accepted this revision.Mar 23 2020, 8:29 AM

OK, LGTM.

This revision is now accepted and ready to land.Mar 23 2020, 8:29 AM

Much more logical than the existing setup. LGTM.

This revision was automatically updated to reflect the committed changes.
Szelethus marked 6 inline comments as done.