Page MenuHomePhabricator

[analyzer] Change the default output type to PD_TEXT_MINIMAL in the frontend, error if an output loc is missing for PathDiagConsumers that need it
AcceptedPublic

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

Details

Summary

The title and the included test file sums everything up -- the only thing I'm mildly afraid of is whether anyone actually depends on the weird behavior of HTMLDiagnostics pretending to be TextDiagnostics if an output directory is not supplied. If it is, I guess we would need to resort to tiptoeing around the compatibility flag.

Diff Detail

Event Timeline

Szelethus created this revision.Mar 20 2020, 9:24 AM

Basically LGTM, but please wait for @NoQ or someone for the compatibility questions.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
26

Why?

This revision is now accepted and ready to land.Mar 20 2020, 11:45 AM
Szelethus marked an inline comment as done.Mar 23 2020, 7:58 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
26

Woops. I rebased this on a patch that moves TextDiagnostics to its own file and forgot about this.

NoQ added inline comments.Mar 23 2020, 8:33 PM
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
209

This patch most likely changes almost nothing because the default output mode of clang --analyze has always been Plist:

lib/Driver/ToolChains/Clang.cpp
2938:  // Set the output format. The default is plist, for (lame) historical reasons.
2939-  CmdArgs.push_back("-analyzer-output");
2940-  if (Arg *A = Args.getLastArg(options::OPT__analyzer_output))
2941-    CmdArgs.push_back(A->getValue());
2942-  else
2943-    CmdArgs.push_back("plist");

I'll double check on historical reasons; i suspect we should be able to transition out of it eventually because this is indeed the worst possible output mode for --analyze.

That said, i wonder how/why do our tests currently work without flooding everything with html reports.

Szelethus retitled this revision from [analyzer] Change the default output type to PD_TEXT_MINIMAL, error if an output loc is missing for PathDiagConsumers that need it to [analyzer] Change the default output type to PD_TEXT_MINIMAL in the frontend, error if an output loc is missing for PathDiagConsumers that need it.Apr 2 2020, 4:42 AM
Szelethus marked an inline comment as done.Apr 2 2020, 4:49 AM

Lets highlight then that this change affects the analyzer in frontend mode, not in the driver configuration. I might make a patch for that too, btw.

@NoQ, were you able to check whether this change could break anything? :)

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
209

Because the HTML report acts like the text if no output file is supplied.

I'm committing this now as-is, if something breaks, I'll revert.

This revision was automatically updated to reflect the committed changes.
vsavchenko added a subscriber: vsavchenko.EditedFri, May 22, 9:53 AM

Hi @Szelethus , it looks like I am a bearer of bad news again :(
This patch seems to crash scan-build on pretty much every project that I tested.

One way to reproduce:

git clone https://github.com/postgres/postgres.git
cd postgres/
scan-build -plist-html -o ./out ./configure

Output:

scan-build: Using '/Users/vsavchenko/source/llvm-project/build/Release/bin/clang-11' for static analysis
checking build system type... x86_64-apple-darwin19.4.0
checking host system type... x86_64-apple-darwin19.4.0
checking which template to use... darwin
checking whether NLS is wanted... no
checking for default port number... 5432
checking for block size... 8kB
checking for segment size... 1GB
checking for WAL block size... 8kB
checking whether the C compiler works... no
configure: error: in `/Users/vsavchenko/source/postgres':
configure: error: C compiler cannot create executables
See `config.log' for more details
scan-build: Analysis run complete.
scan-build: Analysis results (plist files) deposited in '/Users/vsavchenko/source/postgres/out/2020-05-22-194935-27687-1'
scan-build: Removing directory '/Users/vsavchenko/source/postgres/out/2020-05-22-194935-27687-1' because it contains no reports.
scan-build: No bugs found.

config.log will contain something similar to this:

configure:3987: checking whether the C compiler works
configure:4009: /Users/vsavchenko/source/llvm-project/build/Release/bin/../libexec/ccc-analyzer    conftest.c  >&5
error: analyzer output type 'html' requires an output directory to be specified with -o </path/to/output_directory>
1 error generated.
Use of uninitialized value $HtmlDir in concatenation (.) or string at /Users/vsavchenko/source/llvm-project/build/Release/bin/../libexec/ccc-analyzer line 149, <$fh> line 2.
mkdir /failures: Read-only file system at /Users/vsavchenko/source/llvm-project/build/Release/bin/../libexec/ccc-analyzer line 150.

This isn't so bad as far as news go, considering you saved a lot of work for me in terms of reproducing this! I'll look into it, thanks! I admit to not use scan-build much. :^)

Szelethus reopened this revision.Fri, May 22, 11:19 AM

Reverted in rG429f03089951d62fb370026905c87f1f25cf220f because its late and this is a breaking change to scan-build.

This revision is now accepted and ready to land.Fri, May 22, 11:19 AM