[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

[Szelethus]

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.

[baloghadamsoftware]

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

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

26

Why?

[Szelethus]

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]

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]

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.

[Szelethus]

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

[vsavchenko]

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.

[Szelethus]

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]

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

clang/include/clang/Basic/DiagnosticDriverKinds.td

def err_analyzer_config_invalid_input : Error<
   "invalid input for analyzer-config option '%0', that expects %1 value">;
 def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">;
 def err_analyzer_checker_option_unknown : Error<
   "checker '%0' has no option called '%1'">;
 def err_analyzer_checker_option_invalid_input : Error<
   "invalid input for checker option '%0', that expects %1">;
 def err_analyzer_checker_incompatible_analyzer_option : Error<
   "checker cannot be enabled with analyzer option '%0' == %1">;
+def err_analyzer_missing_output_loc : Error<
+  "analyzer output type '%0' requires an output %1 to be specified with "
+  "-o </path/to/output_%1>">;
 
 def err_drv_invalid_hvx_length : Error<
   "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
 def warn_drv_vectorize_needs_hvx : Warning<
   "auto-vectorization requires HVX, use -mhvx to enable it">,
   InGroup<OptionIgnored>;
 
 def err_drv_module_header_wrong_kind : Error<

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

public:
   /// Vector of checker/package names which will not emit warnings.
   std::vector<std::string> SilencedCheckersAndPackages;
 
   /// A key-value table of use-specified configuration values.
   // TODO: This shouldn't be public.
   ConfigTable Config;
   AnalysisStores AnalysisStoreOpt = RegionStoreModel;
   AnalysisConstraints AnalysisConstraintsOpt = RangeConstraintsModel;
-  AnalysisDiagClients AnalysisDiagOpt = PD_HTML;
+  AnalysisDiagClients AnalysisDiagOpt = PD_TEXT_MINIMAL;
   AnalysisPurgeMode AnalysisPurgeOpt = PurgeStmt;
 
   std::string AnalyzeSpecificFunction;
 
   /// File path to which the exploded graph should be dumped.
   std::string DumpExplodedGraphTo;
 
   /// Store full compiler invocation for reproducible instructions in the

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

 #include "clang/AST/Stmt.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/Token.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Rewrite/Core/HTMLRewrite.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"

 
 } // namespace
 
 void ento::createHTMLDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &OutputDir, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
 
-  // FIXME: HTML is currently our default output type, but if the output
-  // directory isn't specified, it acts like if it was in the minimal text
-  // output mode. This doesn't make much sense, we should have the minimal text
-  // as our default. In the case of backward compatibility concerns, this could
-  // be preserved with -analyzer-config-compatibility-mode=true.
-  createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
-
-  // TODO: Emit an error here.
-  if (OutputDir.empty())
+  if (OutputDir.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "html" << "directory";
     return;
+  }
 
   C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, true));
+  createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
 }
 
 void ento::createHTMLSingleFileDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &OutputDir, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
 
-  // TODO: Emit an error here.
-  if (OutputDir.empty())
+  if (OutputDir.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "html-single-file" << "directory";
     return;
+  }
 
   C.push_back(new HTMLDiagnostics(AnalyzerOpts, OutputDir, PP, false));
   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputDir, PP, CTU);
 }
 
 void ento::createPlistHTMLDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &prefix, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
+
+  if (prefix.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "plist-html" << "directory";
+    return;
+  }
+
   createHTMLDiagnosticConsumer(
       AnalyzerOpts, C, std::string(llvm::sys::path::parent_path(prefix)), PP,
       CTU);
   createPlistMultiFileDiagnosticConsumer(AnalyzerOpts, C, prefix, PP, CTU);
   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, prefix, PP, CTU);
 }
 
 //===----------------------------------------------------------------------===//

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/PlistSupport.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/TokenConcatenation.h"
 #include "clang/Rewrite/Core/HTMLRewrite.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/SmallPtrSet.h"
static void printBugPath(llvm::raw_ostream &o, const FIDMap& FM,
   o << "   </array>\n";
 }
 
 //===----------------------------------------------------------------------===//
 // Methods of PlistDiagnostics.
 //===----------------------------------------------------------------------===//
 
 PlistDiagnostics::PlistDiagnostics(
-    AnalyzerOptions &AnalyzerOpts, const std::string &output,
+    AnalyzerOptions &AnalyzerOpts, const std::string &OutputFile,
     const Preprocessor &PP, const cross_tu::CrossTranslationUnitContext &CTU,
     bool supportsMultipleFiles)
-    : OutputFile(output), PP(PP), CTU(CTU), AnOpts(AnalyzerOpts),
+    : OutputFile(OutputFile), PP(PP), CTU(CTU), AnOpts(AnalyzerOpts),
       SupportsCrossFileDiagnostics(supportsMultipleFiles) {
   // FIXME: Will be used by a later planned change.
   (void)this->CTU;
 }
 
 void ento::createPlistDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &OutputFile, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
 
-  // TODO: Emit an error here.
-  if (OutputFile.empty())
+  if (OutputFile.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "plist" << "file";
     return;
+  }
 
   C.push_back(new PlistDiagnostics(AnalyzerOpts, OutputFile, PP, CTU,
                                    /*supportsMultipleFiles*/ false));
   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputFile, PP, CTU);
 }
 
 void ento::createPlistMultiFileDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &OutputFile, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
 
-  // TODO: Emit an error here.
-  if (OutputFile.empty())
+  if (OutputFile.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "plist-multi-file" << "file";
     return;
+  }
 
   C.push_back(new PlistDiagnostics(AnalyzerOpts, OutputFile, PP, CTU,
                                    /*supportsMultipleFiles*/ true));
   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, OutputFile, PP, CTU);
 }
 
 void PlistDiagnostics::FlushDiagnosticsImpl(
                                     std::vector<const PathDiagnostic *> &Diags,

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 
 using namespace llvm;

 };
 } // end anonymous namespace
 
 void ento::createSarifDiagnosticConsumer(
     AnalyzerOptions &AnalyzerOpts, PathDiagnosticConsumers &C,
     const std::string &Output, const Preprocessor &PP,
     const cross_tu::CrossTranslationUnitContext &CTU) {
 
-  // TODO: Emit an error here.
-  if (Output.empty())
+  if (Output.empty()) {
+    PP.getDiagnostics().Report(diag::err_analyzer_missing_output_loc)
+      << "sarif" << "file";
     return;
+  }
 
   C.push_back(new SarifDiagnostics(AnalyzerOpts, Output, PP.getLangOpts()));
   createTextMinimalPathDiagnosticConsumer(AnalyzerOpts, C, Output, PP, CTU);
 }
 
 static StringRef getFileName(const FileEntry &FE) {
   StringRef Filename = FE.tryGetRealPathName();
   if (Filename.empty())

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CallGraph.h"
 #include "clang/Analysis/CodeInjector.h"
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/StaticAnalyzer/Checkers/LocalCheckers.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"

clang/test/Analysis/output_types.cpp

+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=plist \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PLIST
+
+// CHECK-PLIST: error: analyzer output type 'plist' requires an output file to
+// CHECK-PLIST-SAME: be specified with -o </path/to/output_file>
+
+
+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=plist-multi-file \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PLIST-MULTI
+
+// CHECK-PLIST-MULTI: error: analyzer output type 'plist-multi-file' requires
+// CHECK-PLIST-MULTI-SAME: an output file to be specified with
+// CHECK-PLIST-MULTI-SAME: -o </path/to/output_file>
+
+
+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=plist-html \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PLIST-HTML
+
+// CHECK-PLIST-HTML: error: analyzer output type 'plist-html' requires an output
+// CHECK-PLIST-HTML-SAME: directory to be specified with
+// CHECK-PLIST-HTML-SAME: -o </path/to/output_directory>
+
+
+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=sarif \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-SARIF
+
+// CHECK-SARIF: error: analyzer output type 'sarif' requires an output file to
+// CHECK-SARIF-SAME: be specified with -o </path/to/output_file>
+
+
+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=html \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-HTML
+
+// CHECK-HTML: error: analyzer output type 'html' requires an output directory
+// CHECK-HTML-SAME: to be specified with -o </path/to/output_directory>
+
+
+// RUN: not %clang_analyze_cc1 %s \
+// RUN:   -analyzer-output=html-single-file \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-HTML-SINGLE
+
+// CHECK-HTML-SINGLE: error: analyzer output type 'html-single-file' requires
+// CHECK-HTML-SINGLE-SAME: an output directory to be specified with
+// CHECK-HTML-SINGLE-SAME: -o </path/to/output_directory>