Add an option -analyzer-config stable-report-filename=true to produce more stable file names of scan-build reports.

[sylvestre.ledru]

Summary

With the option '-analyzer-config stable-report-filename=true', instead of report-XXXXXX.html, generate report-<filename>-<function, method name>-<function position>-<id>.html. (id = i++ for several issues found in the same function/method)

[Jean-Daniel]

Couldn't the "AnalyzerOptions" member be declared const ?

[sylvestre.ledru]

Add const to the AnalyzerOptions declaration

[sylvestre.ledru]

Fix a mistake with arc.

[jordan_rose]

I'm not sure this is that much better—do we guarantee that diagnostics are always reported in the same order between two runs? I'm not sure we do. And there's also a race here if two analyzer processes are analyzing two files with the same name, or if they end up reporting issues in a common header.

I'd be okay with putting the file and declaration name in before the random string, though.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

79

Might as well be const from here all the way down.

[sylvestre.ledru]

I agree, this patch is not perfect. I do not guarantee that the file name will remain the same....
However, the generated file is "source name + function/method name", in most of the cases, we won't have too many issues in the same function.
Maybe I could add the type of error in the filename. This will decrease the risk of collision (but not fix it completely)

I'd be okay with putting the file and declaration name in before the random string, though.

That would not fix my problem. I am running daily scan-build on LLVM/Clang (http://buildd-clang.debian.net/scan-build/) + Firefox but I cannot share the URL with others because they are not stable enough (because of the random aspect)...

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

79

Right, I will fix that.

[sylvestre.ledru]

If you want, I can keep the current behavior and instead add a option --no-random-URL or -no-random-filename-report. That won't impact scan-build and you will make me (and my users) happy :p

[jordan_rose]

I think my point is that your URLs still won't be stable unless your build is non-parallel and the analyzer is deterministic, but that is a good use case.

If the analyzer is non-deterministic in the output order of bugs from a single TU, I think we want to fix that, but if the source is changing, you could introduce bugs in between other bugs and shift the numbers down. Maybe you could put the line number in as well?

I'm not quite sure how to stop multiple TUs from reporting the same bugs. For that you'd need to use some kind of issue hash, like CmpRuns.py does.

[sylvestre.ledru]

OK. I was expecting clang analyzer to process them in the same order of reading... Sorry! :)

I will add the line number to the file name.

[sylvestre.ledru]

With the line number + the const propagated

[jordan_rose]

It's still possible to have problems with this (if there are multiple issues occurring on the same line), but mostly I'd think you wouldn't want to use the line number as a stable URL. What if someone inserts a comment at the top of the file?

(This is why the hash in the plist output is based on "lines from the start of the function body", when possible. Definitely not perfect either, but slightly more resilient.)

I guess a lot of this depends on what you are using this for. Do you want report URLs to stay stable even when the source code changes? Do you want to try to deduplicate when different main files produce the same diagnostic? Do you want to be able to handle multiple reports on the same line?

(One way to handle the last is to include the checker name in the report somehow, possibly via hash. It's less likely that the same line will contain two issues reported by the same checker, though not impossible.)

Also, if you are not already, please check that scan-view continues to work as well as scan-build. I don't foresee any problems since you're keeping the "report-" prefix, but just in case.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

281

declName may contain non-valid URL characters. The name of an overloaded operator function is the operator. Objective-C methods probably don't include the parent class in the name, but they could: -[FooBarClass fooBar].

[sylvestre.ledru]

I'd think you wouldn't want to use the line number as a stable URL. What if someone inserts a comment at the top of the file?

That is why I didn't add the line at the first try (I added it because you asked :) or maybe you were talking about "lines from the start of the function body"

Do you want report URLs to stay stable even when the source code changes?

Yes, at least for a few weeks until a bug is fixed and the report disappears.

Do you want to try to deduplicate when different main files produce the same diagnostic? Do you want to be able to handle multiple reports on the same line?

It is not my current goals but maybe during the GSoC.

Also, if you are not already, please check that scan-view continues to work as well as scan-build. I don't foresee any problems since you're keeping the "report-" prefix, but just in case.

Yep, I did. It works fine!

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

281

OK. I am not familiar with Objective-C. I will have a look on how improve the filename.

[jordan_rose]

I'd think you wouldn't want to use the line number as a stable URL. What if someone inserts a comment at the top of the file?

That is why I didn't add the line at the first try (I added it because you asked :) or maybe you were talking about "lines from the start of the function body"

Yeah, I'm not sure what to say about that. Sequentially numbering bugs is a problem anyway if someone else introduces a new bug. I guess you'll have to pick your poison.

I'd like to keep this new behavior off by default for now until either Ted or Anna decides that it's enough superior to the current behavior that it's worth changing, but I think it's good enough to take under a flag now, once you've decided which problem you're going to live with.

[sylvestre.ledru]

OK. I will implement these changes!

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

281

Do you know if we have a method for this or should I implement it?

[jordan_rose]

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

281

To do what, mangle an arbitrary string into an identifier, or URL-safe string? Sorry, I can't remember seeing one, though there's a lot in llvm/Support and llvm/ADT that I eventually discover. You could also just leave the special characters in and make sure they're escaped when scan-build references them by URL.

[sylvestre.ledru]

Can I commit that ? :)

[sylvestre.ledru]

Jordan, is that OK with you? Thanks

[jordan_rose]

Sorry, rather than adding a new option to Clang, please use the existing -analyzer-option API.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

73

Nitpick: The trailing _ isn't very LLVMish. Better to just shorten the input parameter to something like "Opts".

133

Why not SmallString here?

[sylvestre.ledru]

existing -analyzer-option API. I cannot find a reference to this.
Do you mean using the same mechanism as -analyzer-eagerly-assume (for example) or using -analyzer-config ?

If you are talking about the first, where is it defined? Thanks

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

73

_ removed. I will keep AnalyzerOpts if you don't mind, as consistency with PlistDiagnostics.cpp

133

Same here. As consistency with PlistDiagnostics in which a std::string is used. I don't mind updating the two files if you prefer.

[sylvestre.ledru]

ping?

[sylvestre.ledru]

ping ?

[jordan_rose]

Yes, sorry, -analyzer-config, not -analyzer-option.

(There's been a lot going on these past two weeks.)

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

133

Yes, please. No need to incur extra random heap allocations, even if it doesn't make such a difference in this case.

[sylvestre.ledru]

Uses -analyzer-config (way better indeed)
Uses SmallString

[jordan_rose]

This is looking good! Thanks for going through these revisions. One more major change, to avoid stomping on the same file.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

365–369

Please note which is the default here. Also, you're missing a period on the first line.

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

132–133

Funny indentation here.

289–301

This is race-y: what if you have two analyzer instances running at once? You should use the F_Excl flag for llvm::sys::fs::openFileForWrite.

Also, please put spaces around =.

[sylvestre.ledru]

This is race-y: what if you have two analyzer instances running at once? You should use the F_Excl flag for llvm::sys::fs::openFileForWrite.

Are you sure this can happen ? I had the feeling that the analyzer is run a on a file as a whole and since I am writing a file with a file in the name, I cannot have collision.

If you confirm, I will fix that.

[jordan_rose]

If the issue is in a header file, multiple implementation files might include that header file. Better safe than sorry, anyway.

[sylvestre.ledru]

Oh, headers, right! Thanks for confirmation.

[sylvestre.ledru]

Same player try again: With llvm::sys::fs::F_Excl ;)

[jordan_rose]

No, just put the open() in the loop, and skip the exists() check. That's faster anyway—less disk access.

[sylvestre.ledru]

Like that?

[jordan_rose]

Uh, you should probably check that the error code is actually "the file already exists" rather than something completely unrelated, like "you don't have permission to write to this directory".

[sylvestre.ledru]

Sorry. Long week. I am doing dumb things :)

I updated the code to this:

EC = llvm::sys::fs::openFileForWrite(Model.str(), FD, llvm::sys::fs::OpenFlags::F_RW | llvm::sys::fs::F_Excl);
if (EC && EC != std::errc::file_exists) {
    llvm::errs() << "warning: could not create file '" << Model.str()
                 << "': " << EC.message() << '\n';
    return;
}

Work for me

[jordan_rose]

Okay, this one looks good. Last few comments, but no need for another round of review. Thanks, Sylvestre!

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

294

Inconsistent here about whether or not you want to say "OpenFlags". Also, remember the 80-column limit in LLVM.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

418

Ah, that's why. In this case the std::string is better because it avoids a copy.

In the long run we should be eliminating getAsString in favor of streaming to raw_ostreams, but that shouldn't block your patch.

[sylvestre.ledru]

Commited as revision 210970

[sylvestre.ledru]

I implemented the 80 column change + backout of the smallstring change + remove of openflags.
Thanks again!

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

   unsigned AnalyzeAll : 1;
   unsigned AnalyzerDisplayProgress : 1;
   unsigned AnalyzeNestedBlocks : 1;
-  
   /// \brief The flag regulates if we should eagerly assume evaluations of
   /// conditionals, thus, bifurcating the path.
   ///

   /// \sa reportIssuesInMainSourceFile
   Optional<bool> ReportIssuesInMainSourceFile;
 
+  /// \sa StableReportFilename
+  Optional<bool> StableReportFilename;
+
   /// \sa getGraphTrimInterval
   Optional<unsigned> GraphTrimInterval;
 

   /// which accepts the values "true" and "false".
   bool shouldReportIssuesInMainSourceFile();
 
+  /// Returns whether or not the report filename should be random or not.
+  ///
+  /// This is controlled by the 'stable-report-filename' config option,
+  /// which accepts the values "true" and "false". Default = false
+  bool shouldWriteStableReportFilename();
+
   /// Returns whether irrelevant parts of a bug report path should be pruned
   /// out of the final output.
   ///

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

                           /* Default = */ false);
 }
 
+
+bool AnalyzerOptions::shouldWriteStableReportFilename() {
+  return getBooleanOption(StableReportFilename,
+                          "stable-report-filename",
+                          /* Default = */ false);
+}
+
 int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) {
   SmallString<10> StrBuf;
   llvm::raw_svector_ostream OS(StrBuf);

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Core/HTMLRewrite.h"
 #include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"

   std::string Directory;
   bool createdDir, noDir;
   const Preprocessor &PP;
+  AnalyzerOptions &AnalyzerOpts;
 public:
-  HTMLDiagnostics(const std::string& prefix, const Preprocessor &pp);
+  HTMLDiagnostics(AnalyzerOptions &AnalyzerOpts, const std::string& prefix, const Preprocessor &pp);
 
   virtual ~HTMLDiagnostics() { FlushDiagnostics(nullptr); }
 

 
 } // end anonymous namespace
 
-HTMLDiagnostics::HTMLDiagnostics(const std::string& prefix,
+HTMLDiagnostics::HTMLDiagnostics(AnalyzerOptions &AnalyzerOpts,
+                                 const std::string& prefix,
                                  const Preprocessor &pp)
-  : Directory(prefix), createdDir(false), noDir(false), PP(pp) {
+    : Directory(prefix), createdDir(false), noDir(false), PP(pp), AnalyzerOpts(AnalyzerOpts) {
 }
 
 void ento::createHTMLDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
                                         PathDiagnosticConsumers &C,
                                         const std::string& prefix,
                                         const Preprocessor &PP) {
-  C.push_back(new HTMLDiagnostics(prefix, PP));
+  C.push_back(new HTMLDiagnostics(AnalyzerOpts, prefix, PP));
 }
 
 //===----------------------------------------------------------------------===//

 
 void HTMLDiagnostics::ReportDiag(const PathDiagnostic& D,
                                  FilesMade *filesMade) {
-    
   // Create the HTML directory if it is missing.
   if (!createdDir) {
     createdDir = true;

   // Create a new rewriter to generate HTML.
   Rewriter R(const_cast<SourceManager&>(SMgr), PP.getLangOpts());
 
+  // Get the function/method name
+  SmallString<128> declName("unknown");
+  int offsetDecl = 0;
+  if (const Decl *DeclWithIssue = D.getDeclWithIssue()) {
+      if (const NamedDecl *ND = dyn_cast<NamedDecl>(DeclWithIssue)) {
+          declName = ND->getDeclName().getAsString();
+      }
+
+      if (const Stmt *Body = DeclWithIssue->getBody()) {
+          // Retrieve the relative position of the declaration which will be used
+          // for the file name
+          FullSourceLoc L(
+              SMgr.getExpansionLoc((*path.rbegin())->getLocation().asLocation()),
+              SMgr);
+          FullSourceLoc FunL(SMgr.getExpansionLoc(Body->getLocStart()), SMgr);
+          offsetDecl = L.getExpansionLineNumber() - FunL.getExpansionLineNumber();
+      }
+  }
+
   // Process the path.
   unsigned n = path.size();
   unsigned max = n;
 
-  for (PathPieces::const_reverse_iterator I = path.rbegin(), 
+  for (PathPieces::const_reverse_iterator I = path.rbegin(),
        E = path.rend();
         I != E; ++I, --n)
     HandlePiece(R, FID, **I, n, max);

     DirName += '/';
   }
 
+  int LineNumber = (*path.rbegin())->getLocation().asLocation().getExpansionLineNumber();
+  int ColumnNumber = (*path.rbegin())->getLocation().asLocation().getExpansionColumnNumber();
+
   // Add the name of the file as an <h1> tag.
 
   {

       << html::EscapeText(Entry->getName())
       << "</td></tr>\n<tr><td class=\"rowname\">Location:</td><td>"
          "<a href=\"#EndPath\">line "
-      << (*path.rbegin())->getLocation().asLocation().getExpansionLineNumber()
+      << LineNumber
       << ", column "
-      << (*path.rbegin())->getLocation().asLocation().getExpansionColumnNumber()
+      << ColumnNumber
       << "</a></td></tr>\n"
          "<tr><td class=\"rowname\">Description:</td><td>"
       << D.getVerboseDescription() << "</td></tr>\n";

     os << "\n<!-- BUGFILE " << DirName << Entry->getName() << " -->\n";
 
     os << "\n<!-- BUGLINE "
-       << path.back()->getLocation().asLocation().getExpansionLineNumber()
+       << LineNumber
        << " -->\n";
 
     os << "\n<!-- BUGCOLUMN "
-      << path.back()->getLocation().asLocation().getExpansionColumnNumber()
+      << ColumnNumber
       << " -->\n";
 
     os << "\n<!-- BUGPATHLENGTH " << path.size() << " -->\n";

   // Create a path for the target HTML file.
   int FD;
   SmallString<128> Model, ResultPath;
-  llvm::sys::path::append(Model, Directory, "report-%%%%%%.html");
 
-  if (std::error_code EC =
+  if (!AnalyzerOpts.shouldWriteStableReportFilename()) {
+      llvm::sys::path::append(Model, Directory, "report-%%%%%%.html");
+
+      if (std::error_code EC =
           llvm::sys::fs::createUniqueFile(Model.str(), FD, ResultPath)) {
           llvm::errs() << "warning: could not create file in '" << Directory
                        << "': " << EC.message() << '\n';
           return;
+      }
+
+  } else {
+
+      int i = 1;
+      std::error_code EC;
+      do {
+          // Find a filename which is not already used
+          Model = "";
+          llvm::sys::path::append(Model, Directory, "report-" + llvm::sys::path::filename(Entry->getName()) + "-" + declName.c_str() + "-" + std::to_string(offsetDecl) + "-" + std::to_string(i) + ".html") ;
+          EC = llvm::sys::fs::openFileForWrite(Model.str(), FD, llvm::sys::fs::OpenFlags::F_RW | llvm::sys::fs::F_Excl);
+          if (EC && EC != std::errc::file_exists) {
+              llvm::errs() << "warning: could not create file '" << Model.str()
+                           << "': " << EC.message() << '\n';
+              return;
+          }
+          i++;
+      } while (EC);
   }
 
   llvm::raw_fd_ostream os(FD, true);

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

             break;
         }
         if (!declKind.empty()) {
-          const std::string &declName = ND->getDeclName().getAsString();
+          SmallString<128> declName(ND->getDeclName().getAsString());
           o << "  <key>issue_context_kind</key>";
           EmitString(o, declKind) << '\n';
           o << "  <key>issue_context</key>";

tools/scan-build/scan-build

 sub FileWanted {
     my $baseDirRegEx = quotemeta $baseDir;
     my $file = $File::Find::name;
+
+    # The name of the file is generated by clang binary (HTMLDiagnostics.cpp)
     if ($file =~ /report-.*\.html$/) {
        my $relative_file = $file;
        $relative_file =~ s/$baseDirRegEx//g;

  -analyzer-config <options>
 
    Provide options to pass through to the analyzer's -analyzer-config flag.
+   Several options are separated with comma: 'key1=val1,key2=val2'
+
+   Available options:
+     * stable-report-filename=true or false (default)
+       Switch the page naming to:
+       report-<filename>-<function/method name>-<id>.html
+       instead of report-XXXXXX.html
 
 CONTROLLING CHECKERS: