[PATCH] D52804: [analyzer] NFC: RetainCountChecker: Avoid dumping symbols during normal operation.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, george.karpenkov.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

The checker was trying to produce a separate `ProgramPointTag` for every dead symbol it cleans up, so that to potentially be able to produce a separate `ExplodedNode` for every symbol, and for that purpose it was dumping the symbol into a string stream.

We don't need a separate node for every symbol, because whenever the first symbol leaks, a bug is emitted, the analysis is sinked, and the checker callback immediately returns due to `State` variable turning into null, so we never get to see the second leaking symbol.

Then, even if we needed a separate node for every symbol, it'd be much better to print the raw pointer than pretty-print the symbol, because pointers are more unique. Additionally, we are no longer able to break normal analysis while experimenting with debug dumps, which is exactly how this behavior got noticed - we noticed a crash similar to https://reviews.llvm.org/D52756 on a buildbot that definitely doesn't dump exploded graphs.

Because https://reviews.llvm.org/D52756 has already landed, this patch doesn't change the actual behavior of the Analyzer, just prevents future bugs.


Repository:
  rC Clang

https://reviews.llvm.org/D52804

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp


Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -1318,19 +1318,6 @@
   processLeaks(state, Leaked, Ctx, Pred);
 }
 
-const ProgramPointTag *
-RetainCountChecker::getDeadSymbolTag(SymbolRef sym) const {
-  const CheckerProgramPointTag *&tag = DeadSymbolTags[sym];
-  if (!tag) {
-    SmallString<64> buf;
-    llvm::raw_svector_ostream out(buf);
-    out << "Dead Symbol : ";
-    sym->dumpToStream(out);
-    tag = new CheckerProgramPointTag(this, out.str());
-  }
-  return tag;
-}
-
 void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                           CheckerContext &C) const {
   ExplodedNode *Pred = C.getPredecessor();
@@ -1346,8 +1333,8 @@
     if (const RefVal *T = B.lookup(Sym)){
       // Use the symbol as the tag.
       // FIXME: This might not be as unique as we would like.
-      const ProgramPointTag *Tag = getDeadSymbolTag(Sym);
-      state = handleAutoreleaseCounts(state, Pred, Tag, C, Sym, *T);
+      static CheckerProgramPointTag Tag(this, "DeadSymbolAutorelease");
+      state = handleAutoreleaseCounts(state, Pred, &Tag, C, Sym, *T);
       if (!state)
         return;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D52804.168034.patch
Type: text/x-patch
Size: 1391 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181002/11c6b4c0/attachment.bin>