Fix IssueHash generation

[o.gyorgy]

Summary

Fixing IssueHash generation.
There were some problems with the current version.

If the Decl *D pointer is nullptr the NormalizeLine would crash while getting the LangOptions. The required LangOptions can be provided by the HTML or Plist Diagnostics.

It is possible that the column number provided by the find_first_not_of is 0 in that case the Lexer reads up a wrong source line (translateLineCol requires that column numbering should start at 1).

I think there should be an assert checking the line and col values in the sourcemanagers translateLineCol. The same way as in translateFileLineCol which does this check.

[o.gyorgy]

Regenerate patch with context and clang format HTMLDiagnostics.cpp.

[xazax.hun]

include/clang/StaticAnalyzer/Core/IssueHash.h

42

Please put & next to he variable name, here and some other places.

[o.gyorgy]

Some small format changes. Based on the review.

[seaneveson]

If the Decl *D pointer is nullptr the NormalizeLine would crash while getting the LangOptions.

Do you have a reproducible test case for this?

[o.gyorgy]

I did not create a test checker for the NormalizeLine error in the patch.
Should I add a test checker for this? Do you have any suggestions
where to put it if required?

GetEnclosingDeclContextSignature already checks for nullptr, this
should have been done in the NormalizeLine function also, but in that
case it is not enough because the Lexer needs the LangOpts.

[seaneveson]

In D14919#296597, @o.gyorgy wrote:

I did not create a test checker for the NormalizeLine error in the patch.
Should I add a test checker for this?

I was wondering what sort of source code causes the crash, but I do think it would be good to have a regression test.

Do you have any suggestions where to put it if required?

Maybe in bug_hash_test.cpp, although I'm not sure its necessary to check the plist output when testing for a crash.

[xazax.hun]

In D14919#297168, @seaneveson wrote:

I was wondering what sort of source code causes the crash, but I do think it would be good to have a regression test.

It is possible to generate bug reports without associated declaration, altough it is not advies. I think the reason why it is possible, to give flexibility. E.g.: one could report issues regarding documentation.

[zaks.anna]

LGTM.

[xazax.hun]

Committed in r254394.

include/clang/StaticAnalyzer/Core/IssueHash.h

 #define LLVM_CLANG_STATICANALYZER_CORE_ISSUE_HASH_H
 
 #include "llvm/ADT/SmallString.h"
 
 namespace clang {
 class Decl;
 class SourceManager;
 class FullSourceLoc;
+class LangOptions;
 
 /// \brief Get an MD5 hash to help identify bugs.
 ///
 /// This function returns a hash that helps identify bugs within a source file.
 /// This identification can be utilized to diff diagnostic results on different
 /// snapshots of a projects, or maintain a database of suppressed diagnotics.
 ///
 /// The hash contains the normalized text of the location associated with the
 /// diagnostic. Normalization means removing the whitespaces. The associated
 /// location is the either the last location of a diagnostic path or a uniqueing
 /// location. The bugtype and the name of the checker is also part of the hash.
 /// The last component is the string representation of the enclosing declaration
 /// of the associated location.
 /// 
 /// In case a new hash is introduced, the old one should still be maintained for
 /// a while. One should not introduce a new hash for every change, it is
 /// possible to introduce experimental hashes that may change in the future.
 /// Such hashes should be marked as experimental using a comment in the plist
 /// files.
 llvm::SmallString<32> GetIssueHash(const SourceManager &SM,
                                    FullSourceLoc &IssueLoc,
                                    llvm::StringRef CheckerName,
-                                   llvm::StringRef BugType, const Decl *D);
+                                   llvm::StringRef BugType, const Decl *D,
+                                   const LangOptions& LangOpts);
 
 /// \brief Get the string representation of issue hash. See GetIssueHash() for
 /// more information.
 std::string GetIssueString(const SourceManager &SM, FullSourceLoc &IssueLoc,
                            llvm::StringRef CheckerName, llvm::StringRef BugType,
-                           const Decl *D);
+                           const Decl *D, const LangOptions& LangOpts);
 } // namespace clang
 
 #endif

lib/Basic/SourceManager.cpp

 
 /// \brief Get the source location in \arg FID for the given line:col.
 /// Returns null location if \arg FID is not a file SLocEntry.
 SourceLocation SourceManager::translateLineCol(FileID FID,
                                                unsigned Line,
                                                unsigned Col) const {
   // Lines are used as a one-based index into a zero-based array. This assert
   // checks for possible buffer underruns.
-  assert(Line != 0 && "Passed a zero-based line");
+  assert(Line && Col && "Line and column should start from 1!");
 
   if (FID.isInvalid())
     return SourceLocation();
 
   bool Invalid = false;
   const SLocEntry &Entry = getSLocEntry(FID, &Invalid);
   if (Invalid)
     return SourceLocation();

lib/StaticAnalyzer/Checkers/DebugCheckers.cpp

public:
   void checkPostStmt(const Stmt *S, CheckerContext &C) const {
     if (!BT)
       BT.reset(new BugType(this, "Dump hash components", "debug"));
 
     ExplodedNode *N = C.generateNonFatalErrorNode();
     if (!N)
       return;
 
+    const LangOptions &Opts = C.getLangOpts();
     const SourceManager &SM = C.getSourceManager();
     FullSourceLoc FL(S->getLocStart(), SM);
     std::string HashContent =
         GetIssueString(SM, FL, getCheckName().getName(), BT->getCategory(),
-                       C.getLocationContext()->getDecl());
+                       C.getLocationContext()->getDecl(), Opts);
 
     C.emitReport(llvm::make_unique<BugReport>(*BT, HashContent, N));
   }
 };
 }
 
 void ento::registerBugHashDumper(CheckerManager &mgr) {
   mgr.registerChecker<BugHashDumper>();
 }

lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

// Embed meta-data tags.
 
     os << "\n<!-- BUGFILE " << DirName << Entry->getName() << " -->\n";
 
     os << "\n<!-- FILENAME " << llvm::sys::path::filename(Entry->getName()) << " -->\n";
 
     os  << "\n<!-- FUNCTIONNAME " <<  declName << " -->\n";
 
     os << "\n<!-- ISSUEHASHCONTENTOFLINEINCONTEXT "
-       << GetIssueHash(SMgr, L, D.getCheckName(), D.getBugType(), DeclWithIssue)
-       << " -->\n";
+       << GetIssueHash(SMgr, L, D.getCheckName(), D.getBugType(), DeclWithIssue,
+                       PP.getLangOpts()) << " -->\n";
 
     os << "\n<!-- BUGLINE "
        << LineNumber
        << " -->\n";
 
     os << "\n<!-- BUGCOLUMN "
       << ColumnNumber
       << " -->\n";

lib/StaticAnalyzer/Core/IssueHash.cpp

static StringRef GetNthLineOfFile(llvm::MemoryBuffer *Buffer, int Line) {
   llvm::line_iterator LI(*Buffer, false);
   for (; !LI.is_at_eof() && LI.line_number() != Line; ++LI)
     ;
 
   return *LI;
 }
 
 static std::string NormalizeLine(const SourceManager &SM, FullSourceLoc &L,
-                                 const Decl *D) {
+                                 const LangOptions &LangOpts) {
   static StringRef Whitespaces = " \t\n";
 
-  const LangOptions &Opts = D->getASTContext().getLangOpts();
   StringRef Str = GetNthLineOfFile(SM.getBuffer(L.getFileID(), L),
                                    L.getExpansionLineNumber());
   unsigned col = Str.find_first_not_of(Whitespaces);
-
+  col++;
   SourceLocation StartOfLine =
       SM.translateLineCol(SM.getFileID(L), L.getExpansionLineNumber(), col);
   llvm::MemoryBuffer *Buffer =
       SM.getBuffer(SM.getFileID(StartOfLine), StartOfLine);
   if (!Buffer)
     return {};
 
   const char *BufferPos = SM.getCharacterData(StartOfLine);
 
   Token Token;
-  Lexer Lexer(SM.getLocForStartOfFile(SM.getFileID(StartOfLine)), Opts,
+  Lexer Lexer(SM.getLocForStartOfFile(SM.getFileID(StartOfLine)), LangOpts,
               Buffer->getBufferStart(), BufferPos, Buffer->getBufferEnd());
 
   size_t NextStart = 0;
   std::ostringstream LineBuff;
   while (!Lexer.LexFromRawLexer(Token) && NextStart < 2) {
     if (Token.isAtStartOfLine() && NextStart++ > 0)
       continue;
     LineBuff << std::string(SM.getCharacterData(Token.getLocation()),
static llvm::SmallString<32> GetHashOfContent(StringRef Content) {
   llvm::MD5::stringifyResult(MD5Res, Res);
 
   return Res;
 }
 
 std::string clang::GetIssueString(const SourceManager &SM,
                                   FullSourceLoc &IssueLoc,
                                   StringRef CheckerName, StringRef BugType,
-                                  const Decl *D) {
+                                  const Decl *D,
+                                  const LangOptions &LangOpts) {
   static StringRef Delimiter = "$";
 
   return (llvm::Twine(CheckerName) + Delimiter +
           GetEnclosingDeclContextSignature(D) + Delimiter +
           llvm::utostr(IssueLoc.getExpansionColumnNumber()) + Delimiter +
-          NormalizeLine(SM, IssueLoc, D) + Delimiter + BugType)
+          NormalizeLine(SM, IssueLoc, LangOpts) + Delimiter + BugType)
       .str();
 }
 
 SmallString<32> clang::GetIssueHash(const SourceManager &SM,
                                     FullSourceLoc &IssueLoc,
                                     StringRef CheckerName, StringRef BugType,
-                                    const Decl *D) {
+                                    const Decl *D,
+                                    const LangOptions &LangOpts) {
+
   return GetHashOfContent(
-      GetIssueString(SM, IssueLoc, CheckerName, BugType, D));
+      GetIssueString(SM, IssueLoc, CheckerName, BugType, D, LangOpts));
 }

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

for (std::vector<const PathDiagnostic*>::iterator DI=Diags.begin(),
     o << "   <key>issue_hash_content_of_line_in_context</key>";
     PathDiagnosticLocation UPDLoc = D->getUniqueingLoc();
     FullSourceLoc L(SM->getExpansionLoc(UPDLoc.isValid()
                                             ? UPDLoc.asLocation()
                                             : D->getLocation().asLocation()),
                     *SM);
     const Decl *DeclWithIssue = D->getDeclWithIssue();
     EmitString(o, GetIssueHash(*SM, L, D->getCheckName(), D->getBugType(),
-                               DeclWithIssue))
+                               DeclWithIssue, LangOpts))
         << '\n';
 
     // Output information about the semantic context where
     // the issue occurred.
     if (const Decl *DeclWithIssue = D->getDeclWithIssue()) {
       // FIXME: handle blocks, which have no name.
       if (const NamedDecl *ND = dyn_cast<NamedDecl>(DeclWithIssue)) {
         StringRef declKind;

test/Analysis/bug_hash_test.cpp

 // CHECK-NEXT:         <key>line</key><integer>14</integer>
 // CHECK-NEXT:         <key>col</key><integer>28</integer>
 // CHECK-NEXT:         <key>file</key><integer>0</integer>
 // CHECK-NEXT:        </dict>
 // CHECK-NEXT:       </array>
 // CHECK-NEXT:     </array>
 // CHECK-NEXT:     <key>depth</key><integer>0</integer>
 // CHECK-NEXT:     <key>extended_message</key>
-// CHECK-NEXT:     <string>debug.DumpBugHash$int f()$28$namespaceAA{$debug</string>
+// CHECK-NEXT:     <string>debug.DumpBugHash$int f()$28$constexprintf(){return5;}$debug</string>
 // CHECK-NEXT:     <key>message</key>
-// CHECK-NEXT:     <string>debug.DumpBugHash$int f()$28$namespaceAA{$debug</string>
+// CHECK-NEXT:     <string>debug.DumpBugHash$int f()$28$constexprintf(){return5;}$debug</string>
 // CHECK-NEXT:    </dict>
 // CHECK-NEXT:   </array>
-// CHECK-NEXT:   <key>description</key><string>debug.DumpBugHash$int f()$28$namespaceAA{$debug</string>
+// CHECK-NEXT:   <key>description</key><string>debug.DumpBugHash$int f()$28$constexprintf(){return5;}$debug</string>
 // CHECK-NEXT:   <key>category</key><string>debug</string>
 // CHECK-NEXT:   <key>type</key><string>Dump hash components</string>
 // CHECK-NEXT:   <key>check_name</key><string>debug.DumpBugHash</string>
 // CHECK-NEXT:   <!-- This hash is experimental and going to change! -->
-// CHECK-NEXT:   <key>issue_hash_content_of_line_in_context</key><string>f8ee38da3de42e209c4afa886b5531ab</string>
+// CHECK-NEXT:   <key>issue_hash_content_of_line_in_context</key><string>f5471f52854dc14167fe96db50c4ba5f</string>
 // CHECK-NEXT:  <key>issue_context_kind</key><string>function</string>
 // CHECK-NEXT:  <key>issue_context</key><string>f</string>
 // CHECK-NEXT:  <key>issue_hash_function_offset</key><string>0</string>
 // CHECK-NEXT:  <key>location</key>
 // CHECK-NEXT:  <dict>
 // CHECK-NEXT:   <key>line</key><integer>14</integer>
 // CHECK-NEXT:   <key>col</key><integer>28</integer>
 // CHECK-NEXT:   <key>file</key><integer>0</integer>

test/Analysis/diagnostics/report-issues-within-main-file.cpp

 // CHECK-NEXT:     <string>Memory allocated by &apos;new[]&apos; should be deallocated by &apos;delete[]&apos;, not &apos;delete&apos;</string>
 // CHECK-NEXT:    </dict>
 // CHECK-NEXT:   </array>
 // CHECK-NEXT:   <key>description</key><string>Memory allocated by &apos;new[]&apos; should be deallocated by &apos;delete[]&apos;, not &apos;delete&apos; (within a call to &apos;~auto_ptr&apos;)</string>
 // CHECK-NEXT:   <key>category</key><string>Memory Error</string>
 // CHECK-NEXT:   <key>type</key><string>Bad deallocator</string>
 // CHECK-NEXT:   <key>check_name</key><string>unix.MismatchedDeallocator</string>
 // CHECK-NEXT:   <!-- This hash is experimental and going to change! -->
-// CHECK-NEXT:   <key>issue_hash_content_of_line_in_context</key><string>f21ac032efaa3d1459a5ed31f0ad44f0</string>
+// CHECK-NEXT:   <key>issue_hash_content_of_line_in_context</key><string>f689fbd54138491e228f0f89bb02bfb2</string>
 // CHECK-NEXT:  <key>issue_context_kind</key><string>function</string>
 // CHECK-NEXT:  <key>issue_context</key><string>mainPlusHeader</string>
 // CHECK-NEXT:  <key>issue_hash_function_offset</key><string>2</string>
 // CHECK-NEXT:  <key>location</key>
 // CHECK-NEXT:  <dict>
 // CHECK-NEXT:   <key>line</key><integer>7</integer>
 // CHECK-NEXT:   <key>col</key><integer>1</integer>
 // CHECK-NEXT:   <key>file</key><integer>0</integer>