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

[Anna Zaks]

On Aug 20, 2012, at 6:47 PM, Jordan Rose 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);
> 
> The only difference between the "hard" and "soft" versions is that NoRetHard means /remove/ RefBindings, while NoRet means don't touch them. This could be renamed to "Untracked" or "UntrackedSymbol".
> 
> 
> -  void updateSummaryForCall(const RetainSummary *&Summ,
> -                            const CallEvent &Call);
> +  void updateForCallbackArgument(const RetainSummary *&Summ,
> +                                 const CallEvent &Call);
> 
> This is not a core point about the functionality of this patch, but I think this is an inferior interface. The idea behind 'updateSummaryForCall' is that it takes a context-less summary derived from an ObjCMethodDecl, or even just a selector, and adds the context of the arguments and possibly additional information about the receiver besides its type. The summary generation before this point can be cached and re-used if we call the same method later, but updateSummaryForCall is the time to make changes specific to this CallEvent.
> 
> Currently, the only such change is the presence or absence of a callback parameter, but there's no reason that can't change in the future. Keeping this as 'updateSummaryForCall' gives a proper opaque effect: given a summary for a given decl, update it for this particular call. getSummary shouldn't be thinking about the various ways in which a CallEvent could affect a base summary.
> 

OK. Thanks for the background. I'll change the name back to what it was. (The name was so generic and what the function is doing is very specific, but I see your argument.) 

> I have some spot comments, but I'll save those for the next iteration of the patch, or perhaps tomorrow morning.
> 
> Jordan
> 
> 
> On Aug 20, 2012, at 16:59 , Anna Zaks <ganna at apple.com> wrote:
> 
>> This resolves retain count checker false positives that are caused by inlining ObjC and other methods. Essentially, if we are passing an object to a method with "delegate" in the selector or a function pointer as another argument, we should stop tracking the other parameters/return value as far as the retain count checker is concerned.
>> 
>> Submitting as a patch per Jordan's request.
>> 
>> Anna.
>> <Retain_Count_Stop_Tracking_Even_If_Inlined.diff>
>