[cfe-dev] Ownership attribute for malloc etc. checking

[Andrew McGregor]

Ok, here's an updated patch that should cover all your suggestions.

Tests are included, they pass.

It also adds a check for functions returning memory that has been
freed/taken (but not held, because while this is somewhat dangerous, it is
in fact what malloc itself might do).

Andrew

On Thu, Jun 24, 2010 at 4:11 PM, Jordy Rose <jediknil at belkadan.com> wrote:

>
> I do think this is pretty cool stuff -- like Ted said, being able to
> generalize the leak-checking code would be nice (perhaps even unifying
> malloc/free and Cocoa retain counting someday).
>
> I have a number of minor suggestions, such as asking the attribute which
> arguments matter instead of iterating over them all and seeing if they're
> important, and following the LLVM comment style (complete, capitalized
> sentences ending in [.?!] whenever possible). Also using "!" instead of
> "not". *grin*
>
> I think you have a logic error dealing with held memory. You're allowing a
> double-free if the current free is a hold, but I think what you want is to
> allow /uses/ if the /previous/ free was a hold. Perhaps the RefState kind
> needs a new enum value, Relinquished or something.
>
> It'd be nice to write a couple of tests for this new attribute. It's
> probably sufficiently new to warrant its own test file in the test/Analysis
> directory.
>
> Secretly I would love for the module name /not/ to be checked at all, only
> associated with a pointer when ownership_returns is encountered. It would
> then be expected that any non-escaping marked pointer is cleaned up by the
> end of the function body. This would let people define their own modules
> without any extra configuration at all.
>
> const char * __attribute__((ownership_returns(my_app_intern)))
> intern(const char *);
> void __attribute__((ownership_takes(my_app_intern))) release(const char
> *);
>
> void useInput (const char *input) {
>  const char *str = intern(input);
>  // do complicated stuff with str
>  release(str);
> }
>
> The downside, of course, is that /any/ use of the pointer as a function
> argument would count as escaping, making it not very useful. Perhaps there
> could be heuristics for this sort of thing -- a non-builtin module name
> /must/ be balanced or explicitly escape, or else be passed to a free-like
> function. For things like "malloc" and "fopen", we probably have to be more
> lenient.
>
> If this is extended to work with multiple owners, you'll also run into the
> problem of two kinds of "hold" -- in both cases, there's a new owner, but
> the current owner doesn't necessarily give up ownership. But that's a
> problem for another day.
>
> Last, as a small note, I don't think ownership_returns should require a
> size argument at all -- it falls down for something as simple as this:
>
> struct Node *allocNode() {
>  return (struct Node *)malloc(sizeof(struct Node));
> }
>
> I see you're using MallocMemAux(); if you use the second form, which takes
> a size SVal instead of an Expr*, then you can handle cases where the size
> is unknown.
>
> Overall, I like it! Perhaps clean it up a bit, but having a general
> "ownership checker" seems like a great idea.
>
> Jordy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100625/0cb782a3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-ownership-withtests.patch
Type: text/x-patch
Size: 30684 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100625/0cb782a3/attachment.bin>