Ok, here's an updated patch that should cover all your suggestions.<div><br></div><div>Tests are included, they pass.</div><div><br></div><div>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).</div>
<div><br></div><div>Andrew<br><br><div class="gmail_quote">On Thu, Jun 24, 2010 at 4:11 PM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com">jediknil@belkadan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
I do think this is pretty cool stuff -- like Ted said, being able to<br>
generalize the leak-checking code would be nice (perhaps even unifying<br>
malloc/free and Cocoa retain counting someday).<br>
<br>
I have a number of minor suggestions, such as asking the attribute which<br>
arguments matter instead of iterating over them all and seeing if they're<br>
important, and following the LLVM comment style (complete, capitalized<br>
sentences ending in [.?!] whenever possible). Also using "!" instead of<br>
"not". *grin*<br>
<br>
I think you have a logic error dealing with held memory. You're allowing a<br>
double-free if the current free is a hold, but I think what you want is to<br>
allow /uses/ if the /previous/ free was a hold. Perhaps the RefState kind<br>
needs a new enum value, Relinquished or something.<br>
<br>
It'd be nice to write a couple of tests for this new attribute. It's<br>
probably sufficiently new to warrant its own test file in the test/Analysis<br>
directory.<br>
<br>
Secretly I would love for the module name /not/ to be checked at all, only<br>
associated with a pointer when ownership_returns is encountered. It would<br>
then be expected that any non-escaping marked pointer is cleaned up by the<br>
end of the function body. This would let people define their own modules<br>
without any extra configuration at all.<br>
<br>
const char * __attribute__((ownership_returns(my_app_intern)))<br>
intern(const char *);<br>
void __attribute__((ownership_takes(my_app_intern))) release(const char<br>
*);<br>
<br>
void useInput (const char *input) {<br>
  const char *str = intern(input);<br>
  // do complicated stuff with str<br>
  release(str);<br>
}<br>
<br>
The downside, of course, is that /any/ use of the pointer as a function<br>
argument would count as escaping, making it not very useful. Perhaps there<br>
could be heuristics for this sort of thing -- a non-builtin module name<br>
/must/ be balanced or explicitly escape, or else be passed to a free-like<br>
function. For things like "malloc" and "fopen", we probably have to be more<br>
lenient.<br>
<br>
If this is extended to work with multiple owners, you'll also run into the<br>
problem of two kinds of "hold" -- in both cases, there's a new owner, but<br>
the current owner doesn't necessarily give up ownership. But that's a<br>
problem for another day.<br>
<br>
Last, as a small note, I don't think ownership_returns should require a<br>
size argument at all -- it falls down for something as simple as this:<br>
<br>
struct Node *allocNode() {<br>
  return (struct Node *)malloc(sizeof(struct Node));<br>
}<br>
<br>
I see you're using MallocMemAux(); if you use the second form, which takes<br>
a size SVal instead of an Expr*, then you can handle cases where the size<br>
is unknown.<br>
<br>
Overall, I like it! Perhaps clean it up a bit, but having a general<br>
"ownership checker" seems like a great idea.<br>
<font color="#888888"><br>
Jordy<br>
</font></blockquote></div><br></div>