[cfe-dev] CFRefCount Problem #2: Region Invalidation

[Jordy Rose]

On Aug 27, 2011, at 14:08, Ted Kremenek wrote:

> This looks great.  I understand the need to handle CXXConstructExprs in checkRegionChanges(), but why do we need checkPostStmt() for CXXConstructExprs?  We'll never have a summary to evaluate for those.


It's to handle the "stop tracking" aspect of C++ constructors. checkRegionChanges() actually /doesn't/ invalidate the arguments to C++ constructors, because it treats them as top-level arguments, which maintain their retain counts. It's the postStmt check that currently stops tracking the arguments to C++ methods.

I think it would actually be /great/ to allow annotations on C++ constructors, so that we could do retain-count checks for something like:

{
  LocalObject x([[Foo alloc] init]);
  [*x doSomething];
} // *x is released by LocalObject's destructor
{
  LocalObject y([[[Foo alloc] init] autorelease]);
} // expected-warning{{over-release of object}}

This is certainly a ways off (at the very least we need better destructor support, cf. PR3888), but including this post-statement check means that constructors are treated like any other C++ method calls -- they get a stop-tracking summary from the summary manager, and that's that. If we add support for method calls, it'll apply to constructors for free.

If that's not what we want, we /still/ need to have a post-statement check for CXXConstructExprs to stop tracking all the arguments. Or we need to add the current Expr (or ProgramPoint, as you mentioned) to checkRegionChanges. Which do you think is better here?

Jordy

P.S. I just realized that NewExprs are not implemented in terms of ConstructExprs. If they were, it would make things easier. I don't really want to add NewExprs to CallOrObjCMessage too! Guess that's a new patch to work on, in a completely different part of the system.

P.P.S. I am also totally ignoring placement-args for 'new', which also won't have their reference counts invalidated, on the grounds that I can't think of a good design where using an object as a placement-argument for 'new' should change the local view of its reference count.


> On Aug 25, 2011, at 4:01 PM, Jordy Rose wrote:
> 
>> Okay, here's what I've come up with:
>> 
>> The checkRegionChanges callback now looks like this:
>> 
>> const ProgramState *
>> checkRegionChanges(const ProgramState *state,
>>                  const StoreManager::InvalidatedSymbols *invalidated,
>>                  ArrayRef<const MemRegion *> ExplicitRegions,
>>                  ArrayRef<const MemRegion *> Regions) const;
>> 
>> ...where ExplicitRegions contains the regions specifically requested for invalidation. (An ArrayRef also seems better than the begin/end pair we currently use.)
>> 
>> This is enough to manually recreate the whitelist in /almost/ the same way as before. What's missing is the invalidation of arguments to C++ constructors (and C++ new-expressions), which now show up as "top-level arguments". To fix this, I added CXXConstructExpr to CallOrObjCMessage and added another post-statement check to RetainReleaseChecker. (Currently we just stop tracking ObjC objects that get passed into C++-land, but that could change in the future.)
>> 
>> Amusingly, this passed the limit on the number of checks allowed per Checker, so I increased that as well.
>> 
>> Consequently, this is a rather ugly patch. I can try to separate it into smaller pieces if you want.
>> 
>> Jordy
>> 
>> <CFRefCount-NoWhitelist.patch>
>