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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Analysis/PathDiagnostic.cpp:1136-1137
   ID.Add(getLocation());
+  ID.Add(getUniqueingLoc());
+  ID.AddPointer(getUniqueingLoc().isValid() ? getUniqueingDecl() : nullptr);
   ID.AddString(BugType);
----------------
Szelethus wrote:
> 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.
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.


================
Comment at: clang/test/Analysis/malloc.c:793
   int *p = malloc(12);
   p = malloc(12);
+} // expected-warning {{Potential leak of memory pointed to by}}\
----------------
Szelethus wrote:
> 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83115/new/

https://reviews.llvm.org/D83115