[analyzer] Explain why analyzer report is not generated (fix for PR12421).

[ayartsev]

Summary

Hi all,

Currently if the path diagnostic consumer (e.g HTMLDiagnostics and PlistDiagnostics) do not support cross file diagnostics then the path diagnostic report is just silently omitted in the case of cross file diagnostics. If the analyzer finds an issue the missing report looks like a Clang bug as an issue is successfully reported to stdout but no report is generated.
The patch adds a little verbosity to Clang in the case considered above.
PR12421 is devoted to the issue.
Please review!
Tried to write tests that would cover warnings inside the 'for' loop, but failed. Any ideas?

[zaks.anna]

test/Analysis/PR12421.c

11 ↗

(On Diff #64431)

We should use the name of the diagnostic consumer here - that will only be legible for the developers working on the attic analyzer core.

test/Analysis/PR12421.h

1 ↗

(On Diff #64431)

Please. do not use the PR as a file name. Use the purpose of the test instead,

[ayartsev]

test/Analysis/PR12421.c

11 ↗

(On Diff #64431)

Done. As for me the name of the diagnostic consumer also makes the warning more clear and helpful for an ordinary user. From the consumer name he can see what report format is talked about and maybe change the scan-build (which setups the '-analyzer-output' frontend option internally) options. Do you still want to remove the consumer name from the warning?

[zaks.anna]

test/Analysis/PR12421.c

11 ↗

(On Diff #64431)

"HTMLDiagnostics" is not a name a user would be familiar with. You should use only familiar terms in diagnostics.

test/Analysis/PR12421.h

1 ↗

(On Diff #64431)

this does not seem to be done.

[ayartsev]

test/Analysis/PR12421.c

11 ↗

(On Diff #64431)

Ok.

test/Analysis/PR12421.h

1 ↗

(On Diff #64431)

Did you see an updated diff? Or I'm probably missing something. Maybe you mean something like this: check-diag-cross-file-boundaries-no-report.* ?

[zaks.anna]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

Can/should we be specific about what the user-specified output format is?

[ayartsev]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

It's unable to extract information about user-specified output format from the "PathDiagnosticConsumer" interface. And this class seem to be too generic to contain "AnalyzerOptions" member or to have e.g. "pure virtual getOutputFormatName()".
So the only way I see to get info about output format is to use "PathDiagnosticConsumer::getName()".
Maybe it makes sense just to add a hint to use "--analyzer-output" driver option to change output format. However this option is not documented at all and is not displayed in clang help. What do you think?

[ayartsev]

Ping.

[ayartsev]

Gentle ping.

[zaks.anna]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

I think mentioning the option is the best option. What is that option called in scan-build?

[ayartsev]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

scan-build (both perl and python versions) has two options: "-plist" and "-plist-html" that are translated to "-analyzer-output=plist" and "-analyzer-output=plist-html" frontend options respectively.

[ayartsev]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

I suggest to document the "--analyzer-output" option and to mention this option in the warning.

[zaks.anna]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

15

That sounds good to me.

[ayartsev]

Updated the patch, added help entry for the "--analyzer-output" driver option. Please review.

[ayartsev]

Updated the patch. Important change in Options.td was missing in the last patch + indentation fixed.
Still Ok to commit?

[zaks.anna]

LGTM. Thanks.

[ayartsev]

Closed by r283499.

include/clang/Driver/Options.td

 
 def _mhwdiv_EQ : Joined<["--"], "mhwdiv=">, Alias<mhwdiv_EQ>;
 def _mhwdiv : Separate<["--"], "mhwdiv">, Alias<mhwdiv_EQ>;
 def _CLASSPATH_EQ : Joined<["--"], "CLASSPATH=">, Alias<fclasspath_EQ>;
 def _CLASSPATH : Separate<["--"], "CLASSPATH">, Alias<fclasspath_EQ>;
 def _all_warnings : Flag<["--"], "all-warnings">, Alias<Wall>;
 def _analyze_auto : Flag<["--"], "analyze-auto">, Flags<[DriverOption]>;
 def _analyzer_no_default_checks : Flag<["--"], "analyzer-no-default-checks">, Flags<[DriverOption]>;
-def _analyzer_output : JoinedOrSeparate<["--"], "analyzer-output">, Flags<[DriverOption]>;
+def _analyzer_output : JoinedOrSeparate<["--"], "analyzer-output">, Flags<[DriverOption]>,
+  HelpText<"Static analyzer report output format (html|plist|plist-multi-file|plist-html|text).">;
 def _analyze : Flag<["--"], "analyze">, Flags<[DriverOption, CoreOption]>,
   HelpText<"Run the static analyzer">;
 def _assemble : Flag<["--"], "assemble">, Alias<S>;
 def _assert_EQ : Joined<["--"], "assert=">, Alias<A>;
 def _assert : Separate<["--"], "assert">, Alias<A>;
 def _bootclasspath_EQ : Joined<["--"], "bootclasspath=">, Alias<fbootclasspath_EQ>;
 def _bootclasspath : Separate<["--"], "bootclasspath">, Alias<fbootclasspath_EQ>;
 def _classpath_EQ : Joined<["--"], "classpath=">, Alias<fclasspath_EQ>;

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

 //===--- PathDiagnostic.cpp - Path-Specific Diagnostic Handling -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the PathDiagnostic-related interfaces.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
void PathDiagnosticConsumer::HandlePathDiagnostic(
   // If the PathDiagnosticConsumer does not support diagnostics that
   // cross file boundaries, prune out such diagnostics now.
   if (!supportsCrossFileDiagnostics()) {
     // Verify that the entire path is from the same FileID.
     FileID FID;
     const SourceManager &SMgr = D->path.front()->getLocation().getManager();
     SmallVector<const PathPieces *, 5> WorkList;
     WorkList.push_back(&D->path);
+    SmallString<128> buf;
+    llvm::raw_svector_ostream warning(buf);
+    warning << "warning: Path diagnostic report is not generated. Current "
+            << "output format does not support diagnostics that cross file "
+            << "boundaries. Refer to --analyzer-output for valid output "
+            << "formats\n";
 
     while (!WorkList.empty()) {
       const PathPieces &path = *WorkList.pop_back_val();
 
       for (PathPieces::const_iterator I = path.begin(), E = path.end(); I != E;
            ++I) {
         const PathDiagnosticPiece *piece = I->get();
         FullSourceLoc L = piece->getLocation().asLocation().getExpansionLoc();
 
         if (FID.isInvalid()) {
           FID = SMgr.getFileID(L);
-        } else if (SMgr.getFileID(L) != FID)
-          return; // FIXME: Emit a warning?
+        } else if (SMgr.getFileID(L) != FID) {
+          llvm::errs() << warning.str();
+          return;
+        }
 
         // Check the source ranges.
         ArrayRef<SourceRange> Ranges = piece->getRanges();
         for (ArrayRef<SourceRange>::iterator I = Ranges.begin(),
                                              E = Ranges.end(); I != E; ++I) {
           SourceLocation L = SMgr.getExpansionLoc(I->getBegin());
-          if (!L.isFileID() || SMgr.getFileID(L) != FID)
-            return; // FIXME: Emit a warning?
+          if (!L.isFileID() || SMgr.getFileID(L) != FID) {
+            llvm::errs() << warning.str();
+            return;
+          }
           L = SMgr.getExpansionLoc(I->getEnd());
-          if (!L.isFileID() || SMgr.getFileID(L) != FID)
-            return; // FIXME: Emit a warning?
+          if (!L.isFileID() || SMgr.getFileID(L) != FID) {
+            llvm::errs() << warning.str();
+            return;
+          }
         }
 
         if (const PathDiagnosticCallPiece *call =
             dyn_cast<PathDiagnosticCallPiece>(piece)) {
           WorkList.push_back(&call->path);
         }
         else if (const PathDiagnosticMacroPiece *macro =
                  dyn_cast<PathDiagnosticMacroPiece>(piece)) {

test/Analysis/diagnostics/diag-cross-file-boundaries.h

+static void f() {
+  int *p = 0;
+  *p = 1;       // expected-warning{{Dereference of null pointer}}
+}

test/Analysis/diagnostics/diag-cross-file-boundaries.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=html -o PR12421.html %s 2>&1 | FileCheck %s
+
+// Test for PR12421
+#include "diag-cross-file-boundaries.h"
+
+int main(){
+  f();
+  return 0;
+}
+
+// CHECK: warning: Path diagnostic report is not generated.