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

[Andrew McGregor]

So, here it is again.  This version addresses all your comments, I think.  I
went a bit further in deleting unnecessary methods etc.

Passes all its tests on OS X (previous patch was tested on Linux).

Andrew

On Fri, Jul 23, 2010 at 8:25 PM, Ted Kremenek <kremenek at apple.com> wrote:

>
> 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;
> +public:
> +  enum OwnershipKind { Holds, Takes, Returns, None };
> +  OwnershipKind OKind;
> +  attr::Kind AKind;
> +public:
> +  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!
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100727/644180b8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-ownership-withtests-r4.patch
Type: application/octet-stream
Size: 34231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100727/644180b8/attachment.obj>