[PATCH] Check missing or extra releases of ivars in dealloc

Hi,

The attached patch adds additional checks to -warn-objc-missing-dealloc. It checks that all ivars which are used in implementation of synthesized properties are either
a) released in dealloc if the property has retain" or "copy" attribute OR
b) not released in dealloc if the property has "assign" attribute

If it looks useful to you, please review & include it into clang.

clang.patch (3.04 KB)

ivars-in-dealloc.m (862 Bytes)

Hi Nikita,

Overall this patch looks really great. At some point it would be good for it to utilize the logic from the retain/release checker to see if an ivar has been released instead of walking the AST to see if an ivar has been passed a 'release' message. Not only will this allow us to handle the many edge cases with syntax (e.g., casts, parentheses, etc.), it will also allow us to determine if a -dealloc implementation correctly releases an ivar along every path, etc.

If you don't mind waiting just a bit, I'm going to test drive these checks over some large Objective-C projects to see what kind of results it gets!

Ted

Actually, I found one deficiency in my patch but I'm not sure how to fix it. Since I'm only interested in checking 'release' messages sent to ivars of 'readwrite' properties, I check if PD->isReadOnly() returns true. However, if a property is declared as 'readonly' in @interface and then redeclared later as 'readwrite' (in a class extension or in a protocol), PD->isReadOnly() still returns true and some potentially missing releases are not catched by my check. Is there some reusable code in clang which would take property attribute overriding into account and give the the "final" set of attributes, or should I write class extension / protocol property lookup myself?

Hi Nikita,

Other than adding a "bug category" for these warnings, I basically applied the patch as is:

   http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20081027/008677.html

We will continue to evolve the -dealloc checks over time, and this is a great addition!

Ted

I'm don't know if we have such a property attribute lookup mechanism in place; but having an API that returns the "canonical" attributes for a class or protocol would be really useful. This is something fairly fundamental that could be put in the AST library. Essentially it would take as input the ObjCInterfaceDecl and an ASTContext and return the canonical attributes.

Steve: You're the expert on the state of properties in the AST. I haven't looked all that much in what we do in Sema with properties (where some of these issues might pop up). Do we have such a thing already in place?

Also classes with only 'assigned' properties should not be flagged as missing a dealloc method.
I filled a bug report about that a few days ago (2956).

Regards,
Thomas

The attached patch fixes false positives reported in http://llvm.org/bugs/show_bug.cgi?id=2978

clang-dealloc-check.patch (3.76 KB)