[PATCH] [analyzer] Detect duplicate [super dealloc] calls

[Anna Zaks]

David, 

I've listed some small concerns throughout and have not looked at the tests in a lot of detail yet.

However, the main problem is that you store too much state in SuperDeallocState. (We try to keep the states as lean as possible since they are one of the major bottlenecks and have considerable impact on performance.) Most of what you store is only consumed by BugReporterVisitor. You might be able to work around storing this info by lazily reconstructing it at bug reporting time.

When bug is reported, the path is visited bottom up; this is when your visitor will get called. You can teach the visitor to detect the first node in which the SuperDeallocState for the given SymRef has been added (as the path is visited bottom up). At that point, you look up the statement associated with the ProgramPoint and if it's a message call, you found the node to which the diagnostic should be attached. (Take a look at other checkers such as MallocChecker; they are all based on this lazy approach.)

You might also want to play with Stack Hints in error reporting since your reports are likely to span multiple functions. These are the notes that get attached to the call site of the functions that contain the diagnostic. You should use -analyzer-output=text (and -analyzer-output=plist) when testing the full path experience. (Ex: See ./test/Analysis/keychainAPI-diagnostic-visitor.m)

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:156
@@ +155,3 @@
+
+  // FIXME: A callback should disable checkers at the start of functions.
+  if (!shouldRunOnFunctionOrMethod(
----------------
Is this done for performance purposes? Can [super dealloc] be called from functions? isSuperDeallocMessage check seems faster.

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:161
@@ +160,3 @@
+
+  // Check for [super dealloc] method call.
+  if (isSuperDeallocMessage(M)) {
----------------
It would be better to do an early return here as well, unless you plan to expand this for handling other calls.
if (!isSuperDeallocMessage(M))
  return;

================
Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:182
@@ +181,3 @@
+  SymbolRef SelfSymbol = M.getSelfSVal().getAsSymbol();
+  const SuperDeallocState *SDState = State->get<CalledSuperDealloc>(SelfSymbol);
+  if (!SDState)
----------------
These should be guarded on isSuperDeallocMessage. Otherwise, we would do a lookup every time we see a method call.

http://reviews.llvm.org/D5238