[cfe-dev] Ownership attribute for malloc checking, revised patch

Ted Kremenek kremenek at apple.com
Fri Jul 23 13:25:13 PDT 2010

On Jul 20, 2010, at 8:55 PM, Andrew McGregor wrote:

> Regenerated patch against an SVN pull today.
> This seems complete.
> No new tests fail with this patch, and it passes all its own tests.
> 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.
> 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.
> Please review.
> Thanks,
> Andrew McGregor
> Allied Telesis Labs
> <clang-ownership-withtests-r3.patch>

Hi Andrew,

This is looking good, but there are a few things that are worth addressing.  A few specific comments:

+void OwnershipAttr::Destroy(ASTContext &C) {
+  if (ArgNums)
+    C.Deallocate(ArgNums);
+  AttrWithString::Destroy(C);
+void OwnershipTakesAttr::Destroy(ASTContext &C) {
+  OwnershipAttr::Destroy(C);
+void OwnershipHoldsAttr::Destroy(ASTContext &C) {
+  OwnershipAttr::Destroy(C);
+void OwnershipReturnsAttr::Destroy(ASTContext &C) {
+  OwnershipAttr::Destroy(C);

It looks like all the Destroy methods just call OwnershipAttr::Destroy.  Why implement them?

Concerning the constructors:

+OwnershipTakesAttr::OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums,
+                                       unsigned size, llvm::StringRef module) :
+  OwnershipAttr(attr::OwnershipTakes, C, arg_nums, size, module, Takes) {
+  setKind( Takes);

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.

Next, for the definition of OwnershipAttr:

+class OwnershipAttr: public AttrWithString {
+ protected:
+  unsigned* ArgNums;
+  unsigned Size;
+  enum OwnershipKind { Holds, Takes, Returns, None };
+  OwnershipKind OKind;
+  attr::Kind AKind;
+  OwnershipAttr(attr::Kind AK, ASTContext &C, unsigned* arg_nums, unsigned size,
+                       llvm::StringRef module, OwnershipKind kind);
+  virtual void Destroy(ASTContext &C);
+  OwnershipKind getKind() const {
+    return OKind;
+  }
+  void setKind(const OwnershipKind k) {
+    OKind = k;
+  }
+  bool isKind(const OwnershipKind k) const {
+    return OKind == k;
+  }
+  llvm::StringRef getModule() const {
+    return getString();
+  }
+  void setModule(ASTContext &C, llvm::StringRef module) {
+    ReplaceString(C, module);
+  }
+  bool isModule(const char *m) const {
+    return getModule().equals(m);
+  }

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.

Now on to DiagnosticSemaKinds.td:

--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -836,6 +836,12 @@ def err_attribute_requires_objc_interface : Error<
   "attribute may only be applied to an Objective-C interface">;
 def err_nonnull_pointers_only : Error<
   "nonnull attribute only applies to pointer arguments">;
+def err_ownership_returns_integers_only : Error<
+  "ownership_returns attribute only applies to integer arguments">;
+def err_ownership_takes_pointers_only : Error<
+  "ownership_takes attribute only applies to pointer arguments">;
+def err_ownership_holds_pointers_only : Error<
+  "ownership_holds attribute only applies to pointer arguments">;

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.

On to the checker itself....

+void MallocChecker::PreVisitBind(CheckerContext &C,
+                                 const Stmt *AssignE,
+                                 const Stmt *StoreE,
+                                 SVal location,
+                                 SVal val) {

Please add a comment above/within this method that says what it is doing.  In a previous email, you said:

"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."

Please include this in a comment.  I have found such documentation to be invaluable, even for code that I wrote myself.

+  // If ptr is NULL, no operation is preformed.

Spelling: performed

+  if (!val.isZeroConstant()) {

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.

Now on to the test case:

+++ b/test/Analysis/malloc.c
@@ -4,22 +4,134 @@ void *malloc(size_t);
 void free(void *);
 void *realloc(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size);
+void __attribute((ownership_returns(malloc))) *my_malloc(size_t);
+void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+void __attribute((ownership_returns(malloc, 1))) *my_malloc2(size_t);
+void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+// Silly, but not an error.  Duplicate has no extra effect.
+// If two are of different kinds, that is an error, but the test case doesn't work.

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.

This is looking really good!

More information about the cfe-dev mailing list