[cfe-commits] [analyzer] Patch to stop tracking symbols based on a retain count summary of inlined function

[Jordan Rose]

On Aug 21, 2012, at 12:11 , Anna Zaks <ganna at apple.com> wrote:

> 
> On Aug 21, 2012, at 9:04 AM, Jordan Rose wrote:
> 
>> 
>> On Aug 20, 2012, at 18:47 , Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> Yeah, I don't like this approach as much as I thought I would. My original reason for making a new summary out of an old one was to re-use the existing summary evaluation code. This eliminates that, and processSummaryOfInlined is crippled since it only handles StopTrackingHard and NoRetHard. And there's a lot of duplication elsewhere.
>>> 
>>> I'd rather see this:
>>> 
>>> RetainSummaryManager &Summaries = getSummaryManager(C);
>>> const RetainSummary *Summ = Summaries.getSummary(Call, C.getState());
>>> 
>>> if (C.wasInlined && !Summ->appliesToInlined())
>>>   return;
>>> 
>>> checkSummary(*Summ, Call, C);
>> 
>> Thinking about this again, this is about whether we're saying "this summary should be applied even when inlining", or "these /effects/ should be applied even when inlining". Even in the latter case I'd consider adding flags to the ArgEffects rather than adding new ArgEffect kinds, but that's an interesting distinction/decision. Obviously the latter is more flexible, but which do you think we /should/ be doing?
> 
> 
> There are several reasons why I am not reusing the checkSummary code when processing summaries for inlined function.
>  - As you mentioned above, parts/individual effects in a summary are marked as "applied when inlining" not the whole summary. It makes sense to keep it this way. We make decisions on what effect each argument/return value should have separately.
>  - I do not perform checking for errors in processSummaryOfInlined, which is a big part of checkSummary. Even in DecRefAndStopTracking, I think it's OK to not check for use-after-free since we did inline the function and know better if it used the object or not. We should have generated the warning while analyzing the function in the former case.
>  - Passing an extra flag to checkSummary to act differently in the inlined case is much more messy - the only thing the functions share is the argument visitation logic. I'd rather have the argument iterator and get receiver code duplicated rather than one huge function with a lot of conditions. If you are very concerned about it, we could reduce parameter visitation to a single loop by adding another iteration APIs to EvalCall and RetainSummary, which processes both the arguments and receiver. That would make both functions (checkSummary and processSummaryOfInlined) much smaller. (Not sure that it's worth it though.)

Okay, you've convinced me. I wish we didn't have to distinguish "StopTracking" and "StopTrackingHard", but unfortunately a few uses of getPersistentStopSummary() only really apply when the function is not inlined. (In particular, getSummary's switch statement for "unsupported calls". We probably can't assume anything about how most of those calls affect retain counts, except possibly blocks, unless we are able to inline.)

Spot comments on the original patch:

+                 // In some cases, we obtain a better summary for this checker
+                 // by looking at the call site than by inlining the function.
+                 // Signifies that we should stop tracking the symbol even if
+                 // the perfect summary is available.
+                 StopTrackingHard,

I don't like this phrasing; the summary is not "better" at the call site, nor is inlining a "perfect summary" with respect to retain count semantics. In particular, "StopTracking" is always a heuristic; we are voluntarily sacrificing coverage of this symbol because we suspect our model doesn't cover this particular usage. (Remember, most uses of delegates, probably the vast majority, would still benefit from leak warnings…but we err on the side of false negatives.)

I don't have a good alternate phrasing in mind right now.


+                 // The function decrements the reference count and we should 
+                 // stop tracking the argument.
+                 DecRefAndStopTrackingHard, DecRefMsgAndStopTrackingHard

This comment doesn't explain anything, IMHO; if anything it is less explicit than the name since there are now two active agents, "the function" and "we". (If you want me to add comments to all the current ArgEffects and RetEffects I can.)


+              // Treat this function as returning a non-tracked symbol even if
+              // the function has been inlined. This is used where the call
+              // site summary is more presise than the inlining produced
+              // summary.
+              NoRetHard

Again, the call summary is not more precise (sp). It augments the effects of inlining, and in this case augments the effects of inlining to be more conservative.


+  // Evaluate the effect of the arguments.
+  for (unsigned idx = 0, e = CallOrMsg.getNumArgs(); idx != e; ++idx) {
+    SVal V = CallOrMsg.getArgSVal(idx);
+    if (SymbolRef Sym = V.getAsLocSymbol()) {
+      if (Summ.getArg(idx) == StopTrackingHard) {
+        state = removeRefBinding(state, Sym);
+        C.addTransition(state);
+      }
+    }
+  }

In this version of the loop, it may be cheaper to test the summary's effect before seeing if the argument is a symbol.


In the test case:
++ (void) test: (Delegate *)d {
+  Cell *obj1 = [[Cell alloc] initWithDelegate: d]; // no-warning
+  Cell *obj2 = [[Cell alloc] init]; // no-warning
+  updateObject(obj2, releaseObj);
+  [Cell updateObject: obj2
+        WithCallback: releaseObj];
+
+}

The last test case should probably be a separate object, since once you StopTracking[Hard] an object it doesn't matter what else you do to it. Also, there are two paths I can think of that should be added here: calling a non-init method that contains the word "delegate" (-setDelegate: would work well), and making sure the /receiver/ is invalidated when you use a callback.

Not so happy about the additional complexity, but I guess this is necessary for properly handling inlined calls. Thanks for bearing with me.
Jordan


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120822/a0c2a2d4/attachment.html>