[Analyzer] Report every bug if only uniqueing location differs.

[balazske]

Summary

Two CSA bug reports where only the uniqueing location is different
should be treated as different problems. The role of uniqueing location
is to differentiate bug reports.

[baloghadamsoftware]

clang/lib/Analysis/PathDiagnostic.cpp

333

Why this asymmetry?

389

Asymmetry again. What if YUL is valid?

[balazske]

clang/lib/Analysis/PathDiagnostic.cpp

333

The function returns something like "XL is less than YL". The invalid source locations should come always as first (or last?) in the ordering.

389

Here we have already a precondition that XUL == YUL (from line 358).

[Szelethus]

clang/lib/Analysis/PathDiagnostic.cpp

1136–1137

This looks a bit odd -- why do we need both of these?

Also, didn't we use uniqueing location in the BugReportEquivClass or whatever its called? Why do we need to add this here as well? I would like some technical explanation.

clang/test/Analysis/malloc.c

793

On an unrelated note, shouldn't one of the notes be here? @NoQ, is this the same issue as the one you raised with zombie symbols? http://lists.llvm.org/pipermail/cfe-dev/2016-March/047922.html

clang/test/Analysis/pr22954.c

346–357

What a god awful test case.

[NoQ]

clang/lib/Analysis/PathDiagnostic.cpp

1136–1137

As of now it probably does nothing. But technically we allow our users to set a completely arbitrary uniqueing decl that doesn't necessarily correspond to the uniqueing location, so we'll probably need to take it into account.

Generally, uniqueing decls are for uniqueing and otherwise identifying bugs regardless of slight changes in the source code. Currently this means issue hashes.

clang/test/Analysis/malloc.c

793

The warning about the first malloc indeed probably required the zombie symbol bug to be fixed. That said, the warning isn't really guaranteed to be on that line, because there's no guarantee that dead symbol collection runs *immediately* after the overwrite (which causes the symbol to become dead), and the overwrite is in fact the last thing that happens in this function.

[NoQ]

Whoops, i meant to post the comment in https://reviews.llvm.org/rG22a084cfa337d5e5ea90eba5261f7937e28d250b#937772 here. Anyway, i found a crash caused by this patch.

[balazske]

Crash is fixed here: D84843

clang/lib/Analysis/PathDiagnostic.cpp

for ( ; X_I != X_end && Y_I != Y_end; ++X_I, ++Y_I) {
     if (b.hasValue())
       return b.getValue();
   }
 
   return None;
 }
 
 static bool compareCrossTUSourceLocs(FullSourceLoc XL, FullSourceLoc YL) {
+  if (XL.isInvalid() && YL.isValid())
+    return true;
+  if (XL.isValid() && YL.isInvalid())
+    return false;
   std::pair<FileID, unsigned> XOffs = XL.getDecomposedLoc();
   std::pair<FileID, unsigned> YOffs = YL.getDecomposedLoc();
   const SourceManager &SM = XL.getManager();
   std::pair<bool, bool> InSameTU = SM.isInTheSameTranslationUnit(XOffs, YOffs);
   if (InSameTU.first)
     return XL.isBeforeInTranslationUnitThan(YL);
   const FileEntry *XFE = SM.getFileEntryForID(XL.getSpellingLoc().getFileID());
   const FileEntry *YFE = SM.getFileEntryForID(YL.getSpellingLoc().getFileID());
   if (!XFE || !YFE)
     return XFE && !YFE;
   int NameCmp = XFE->getName().compare(YFE->getName());
   if (NameCmp != 0)
     return NameCmp == -1;
   // Last resort: Compare raw file IDs that are possibly expansions.
   return XL.getFileID() < YL.getFileID();
 }
 
 static bool compare(const PathDiagnostic &X, const PathDiagnostic &Y) {
   FullSourceLoc XL = X.getLocation().asLocation();
   FullSourceLoc YL = Y.getLocation().asLocation();
   if (XL != YL)
     return compareCrossTUSourceLocs(XL, YL);
+  FullSourceLoc XUL = X.getUniqueingLoc().asLocation();
+  FullSourceLoc YUL = Y.getUniqueingLoc().asLocation();
+  if (XUL != YUL)
+    return compareCrossTUSourceLocs(XUL, YUL);
   if (X.getBugType() != Y.getBugType())
     return X.getBugType() < Y.getBugType();
   if (X.getCategory() != Y.getCategory())
     return X.getCategory() < Y.getCategory();
   if (X.getVerboseDescription() != Y.getVerboseDescription())
     return X.getVerboseDescription() < Y.getVerboseDescription();
   if (X.getShortDescription() != Y.getShortDescription())
     return X.getShortDescription() < Y.getShortDescription();
-  if (X.getDeclWithIssue() != Y.getDeclWithIssue()) {
-    const Decl *XD = X.getDeclWithIssue();
-    if (!XD)
+  auto CompareDecls = [&XL](const Decl *D1, const Decl *D2) -> Optional<bool> {
+    if (D1 == D2)
+      return None;
+    if (!D1)
       return true;
-    const Decl *YD = Y.getDeclWithIssue();
-    if (!YD)
+    if (!D2)
       return false;
-    SourceLocation XDL = XD->getLocation();
-    SourceLocation YDL = YD->getLocation();
-    if (XDL != YDL) {
+    SourceLocation D1L = D1->getLocation();
+    SourceLocation D2L = D2->getLocation();
+    if (D1L != D2L) {
       const SourceManager &SM = XL.getManager();
-      return compareCrossTUSourceLocs(FullSourceLoc(XDL, SM),
-                                      FullSourceLoc(YDL, SM));
+      return compareCrossTUSourceLocs(FullSourceLoc(D1L, SM),
+                                      FullSourceLoc(D2L, SM));
     }
+    return None;
+  };
+  if (auto Result = CompareDecls(X.getDeclWithIssue(), Y.getDeclWithIssue()))
+    return *Result;
+  if (XUL.isValid()) {
+    if (auto Result = CompareDecls(X.getUniqueingDecl(), Y.getUniqueingDecl()))
+      return *Result;
   }
   PathDiagnostic::meta_iterator XI = X.meta_begin(), XE = X.meta_end();
   PathDiagnostic::meta_iterator YI = Y.meta_begin(), YE = Y.meta_end();
   if (XE - XI != YE - YI)
     return (XE - XI) < (YE - YI);
   for ( ; XI != XE ; ++XI, ++YI) {
     if (*XI != *YI)
       return (*XI) < (*YI);
   }

 }
 
 void PathDiagnosticPopUpPiece::Profile(llvm::FoldingSetNodeID &ID) const {
   PathDiagnosticSpotPiece::Profile(ID);
 }
 
 void PathDiagnostic::Profile(llvm::FoldingSetNodeID &ID) const {
   ID.Add(getLocation());
+  ID.Add(getUniqueingLoc());
+  ID.AddPointer(getUniqueingLoc().isValid() ? getUniqueingDecl() : nullptr);
   ID.AddString(BugType);
   ID.AddString(VerboseDesc);
   ID.AddString(Category);
 }
 
 void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const {
   Profile(ID);
   for (const auto &I : path)

clang/test/Analysis/malloc.c

 void mallocEscapeMalloc() {
   int *p = malloc(12);
   myfoo(p);
   p = malloc(12);
 } // expected-warning{{Potential leak of memory pointed to by}}
 
 void mallocMalloc() {
   int *p = malloc(12);
   p = malloc(12);
-} // expected-warning {{Potential leak of memory pointed to by}}
+} // expected-warning {{Potential leak of memory pointed to by}}\
+  // expected-warning {{Potential leak of memory pointed to by}}
 
 void mallocFreeMalloc() {
   int *p = malloc(12);
   free(p);
   p = malloc(12);
   free(p);
 }
 

clang/test/Analysis/pr22954.c

 };
 
 struct JJ {
   struct jj s1[3];
   char * s2;
 };
 
 int f19(int i) {
   struct JJ J0 = {{{1, 2, 0}, {3, 4, 0}, {5, 6, 0}}, 0};
   J0.s2 = strdup("hello");
   J0.s1[0].s2 = strdup("hello");
   J0.s1[1].s2 = strdup("hi");
   J0.s1[2].s2 = strdup("world");
   char input[2] = {'a', 'b'};
   memcpy(J0.s1[i].s1, input, 2);
   clang_analyzer_eval(J0.s1[0].s1[0] == 1); // expected-warning{{UNKNOWN}}\
   expected-warning{{Potential leak of memory pointed to by field 's2'}}\
+  expected-warning{{Potential leak of memory pointed to by field 's2'}}\
+  expected-warning{{Potential leak of memory pointed to by field 's2'}}\
   expected-warning{{Potential leak of memory pointed to by 'J0.s2'}}
   clang_analyzer_eval(J0.s1[0].s1[1] == 2); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[1].s1[0] == 3); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[1].s1[1] == 4); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[2].s1[0] == 5); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[2].s1[1] == 6); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[i].s1[0] == 5); // expected-warning{{UNKNOWN}}
   clang_analyzer_eval(J0.s1[i].s1[1] == 6); // expected-warning{{UNKNOWN}}
   // FIXME: memory leak warning for J0.s2 should be emitted here instead of after memcpy call.