<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Looking good. Here are a few comments related to your recent changes:<div><br></div><div><div>+ // If ptr is NULL, no operation is performed.</div><div>+ if (nullState) {</div><div>+ if (!notNullState) {</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span> return;</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div>+ }</div><div><br></div><div>There are some tabs in here. Also, this can be written more succinctly:</div><div><br></div><div>if (nullState && !notNullState)</div><div> return;</div><div><br></div><div>+</div><div>+ SymbolRef Sym = val.getLocSymbolInBase();</div><div>+ if (Sym) {</div><div>+ const RefState *RS = state->get<RegionState>(Sym);</div><div><br></div><div>I think you want to use 'notNullState' here, since your assumption is that this transition occurs only when the value is non-null. When 'nullState' is not null, you'll also probably want a transition so that path has consistent checking of the nullity of the symbol.</div><div><br></div><div>Minor style nit:</div><div><br></div><div> state->set<RegionState> (S...)</div><div> isa<HeapSpaceRegion> (MS)</div><div><br></div><div>Please don't write your function calls that way (with the extra space). It's inconsistent with the rest of the codebase, and it's not even consistent within the file.</div><div><br><div><div>On Jul 27, 2010, at 8:23 AM, Andrew McGregor wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">So, here it is again. This version addresses all your comments, I think. I went a bit further in deleting unnecessary methods etc.<div><br></div><div>Passes all its tests on OS X (previous patch was tested on Linux).</div>
<div><br></div><div>Andrew</div><div><br><div class="gmail_quote">On Fri, Jul 23, 2010 at 8:25 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br>
On Jul 20, 2010, at 8:55 PM, Andrew McGregor wrote:<br>
<br>
> Regenerated patch against an SVN pull today.<br>
><br>
> This seems complete.<br>
><br>
> No new tests fail with this patch, and it passes all its own tests.<br>
><br>
> I have compiled large portions of an embedded Linux distribution with checking, and given annotations on commonly used container functions, this reduces the false positive rate for malloc and null pointer dereference checks enormously. Without ownership attributes, approx 10k warnings, with attributes in only a few headers approx 2k warnings (mostly glib headers plus a few in the proprietary parts of the code), and a sampling of those shows that they are mostly real problems (if not likely to occur in actual execution, or of much significance). We have a checked build in our nightly runs, and it has already found the root cause of a number of real issues.<br>
><br>
> The patch itself is a git diff since I am working with git-svn, I have options for regenerating it if that's not suitable.<br>
><br>
> Please review.<br>
><br>
> Thanks,<br>
><br>
> Andrew McGregor<br>
> Allied Telesis Labs<br>
</div></div>> <clang-ownership-withtests-r3.patch><br>
<br>
Hi Andrew,<br>
<br>
This is looking good, but there are a few things that are worth addressing. A few specific comments:<br>
<br>
+void OwnershipAttr::Destroy(ASTContext &C) {<br>
+ if (ArgNums)<br>
+ C.Deallocate(ArgNums);<br>
+ AttrWithString::Destroy(C);<br>
+}<br>
+<br>
+void OwnershipTakesAttr::Destroy(ASTContext &C) {<br>
+ OwnershipAttr::Destroy(C);<br>
+}<br>
+<br>
+void OwnershipHoldsAttr::Destroy(ASTContext &C) {<br>
+ OwnershipAttr::Destroy(C);<br>
+}<br>
+<br>
+<br>
+void OwnershipReturnsAttr::Destroy(ASTContext &C) {<br>
+ OwnershipAttr::Destroy(C);<br>
+}<br>
+<br>
<br>
It looks like all the Destroy methods just call OwnershipAttr::Destroy. Why implement them?<br>
<br>
Concerning the constructors:<br>
<br>
+<br>
+OwnershipTakesAttr::OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums,<br>
+ unsigned size, llvm::StringRef module) :<br>
+ OwnershipAttr(attr::OwnershipTakes, C, arg_nums, size, module, Takes) {<br>
+ setKind( Takes);<br>
+}<br>
+<br>
<br>
Why call setKind() at all? 'Takes' is already passed up to OwnershipAttr. Actually, I think setKind() isn't needed at all; the kind should be immutable for the lifetime of the attribute object.<br>
<br>
Next, for the definition of OwnershipAttr:<br>
<br>
+class OwnershipAttr: public AttrWithString {<br>
+ protected:<br>
+ unsigned* ArgNums;<br>
+ unsigned Size;<br>
+public:<br>
+ enum OwnershipKind { Holds, Takes, Returns, None };<br>
+ OwnershipKind OKind;<br>
+ attr::Kind AKind;<br>
+public:<br>
+ OwnershipAttr(attr::Kind AK, ASTContext &C, unsigned* arg_nums, unsigned size,<br>
+ llvm::StringRef module, OwnershipKind kind);<br>
+<br>
+<br>
+ virtual void Destroy(ASTContext &C);<br>
+<br>
+ OwnershipKind getKind() const {<br>
+ return OKind;<br>
+ }<br>
+ void setKind(const OwnershipKind k) {<br>
+ OKind = k;<br>
+ }<br>
+ bool isKind(const OwnershipKind k) const {<br>
+ return OKind == k;<br>
+ }<br>
+<br>
+ llvm::StringRef getModule() const {<br>
+ return getString();<br>
+ }<br>
+ void setModule(ASTContext &C, llvm::StringRef module) {<br>
+ ReplaceString(C, module);<br>
+ }<br>
+ bool isModule(const char *m) const {<br>
+ return getModule().equals(m);<br>
+ }<br>
<br>
<br>
Please add a doxygen comment above OwnershipAttr that describes its purpose. Also, the 'getModule' and 'isAttrArg' aren't obvious in their purpose just from inspecting this class declaration. Please add doxygen comments for them as well.<br>
<br>
Now on to DiagnosticSemaKinds.td:<br>
<br>
--- a/include/clang/Basic/DiagnosticSemaKinds.td<br>
+++ b/include/clang/Basic/DiagnosticSemaKinds.td<br>
@@ -836,6 +836,12 @@ def err_attribute_requires_objc_interface : Error<<br>
"attribute may only be applied to an Objective-C interface">;<br>
def err_nonnull_pointers_only : Error<<br>
"nonnull attribute only applies to pointer arguments">;<br>
+def err_ownership_returns_integers_only : Error<<br>
+ "ownership_returns attribute only applies to integer arguments">;<br>
+def err_ownership_takes_pointers_only : Error<<br>
+ "ownership_takes attribute only applies to pointer arguments">;<br>
+def err_ownership_holds_pointers_only : Error<<br>
+ "ownership_holds attribute only applies to pointer arguments">;<br>
<br>
Instead of having 3 diagnostics, you only need one. You can pass the name of the attribute when issuing the diagnostic, and use a %0 to represent a string substitution. It makes it more general and reduces clutter.<br>
<br>
On to the checker itself....<br>
<br>
+void MallocChecker::PreVisitBind(CheckerContext &C,<br>
+ const Stmt *AssignE,<br>
+ const Stmt *StoreE,<br>
+ SVal location,<br>
+ SVal val) {<br>
<br>
Please add a comment above/within this method that says what it is doing. In a previous email, you said:<br>
<br>
"The PreVisitBind implements the same algorithm as already used by the Objective C ownership checker: if the pointer escaped from this scope by assignment, let it go. However, assigning to fields of a stack-storage structure does not transfer ownership."<br>
<br>
Please include this in a comment. I have found such documentation to be invaluable, even for code that I wrote myself.<br>
<br>
+ // If ptr is NULL, no operation is preformed.<br>
<br>
Spelling: performed<br>
<br>
+ if (!val.isZeroConstant()) {<br>
+<br>
<br>
This isn't a generic way to detect of 'val' is null. Val may be a symbol whose value is constrained to be null along a given path. Take a look at the DereferenceChecker on how we detect if a value is null by using 'Assume' on the GRState. That way is completely generic, irrespective of how the argument is written.<br>
<br>
Now on to the test case:<br>
<br>
+++ b/test/Analysis/malloc.c<br>
@@ -4,22 +4,134 @@ void *malloc(size_t);<br>
void free(void *);<br>
void *realloc(void *ptr, size_t size);<br>
void *calloc(size_t nmemb, size_t size);<br>
+void __attribute((ownership_returns(malloc))) *my_malloc(size_t);<br>
+void __attribute((ownership_takes(malloc, 1))) my_free(void *);<br>
+void __attribute((ownership_returns(malloc, 1))) *my_malloc2(size_t);<br>
+void __attribute((ownership_holds(malloc, 1))) my_hold(void *);<br>
+// Silly, but not an error. Duplicate has no extra effect.<br>
+// If two are of different kinds, that is an error, but the test case doesn't work.<br>
<br>
This comment doesn't mean anything to me. It feels like a mental dump of something you had in your head, but doesn't really explain anything to someone else. I'm not certain what you are actually referring to when you say "Silly, but not an error." Please clarify this comment.<br>
<br>
This is looking really good!<br>
<br>
<br>
<br>
</blockquote></div><br></div>
<span><clang-ownership-withtests-r4.patch></span></blockquote></div><br></div></div></body></html>