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

[Andrew McGregor]

On Tue, Jul 27, 2010 at 5:02 PM, Ted Kremenek <kremenek at apple.com> wrote:

> Looking good.  Here are a few comments related to your recent changes:
>
> +  // If ptr is NULL, no operation is performed.
> +  if (nullState) {
> +    if (!notNullState) {
> +  return;
> + }
> +  }
>
> There are some tabs in here.  Also, this can be written more succinctly:
>
> if (nullState && !notNullState)
>   return;
>

Yes, of course.  As for the tabs... I don't get that, I think I have to
spend some time with editor documentation to figure that one out.


>
> +
> +  SymbolRef Sym = val.getLocSymbolInBase();
> +  if (Sym) {
> +    const RefState *RS = state->get<RegionState>(Sym);
>
> 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.
>

Not sure I understand... the RegionState is used because I'm getting at and
assuming forward the semantics of the memory region being assigned away, and
not touching the null or otherwise assumptions.  Should I be using
notNullState to assert that the assignee is not null?


>
> Minor style nit:
>
>  state->set<RegionState> (S...)
>  isa<HeapSpaceRegion> (MS)
>
> 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.
>

Ok.  Not sure how that happened... too much fiddling around with editors
again (this has been touched by Eclipse, Emacs and TextMate... things have
been a bit chaotic... I think this one is Eclipse's fault).


>
> On Jul 27, 2010, at 8:23 AM, Andrew McGregor wrote:
>
> 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!
>>
>>
>>
>>
> <clang-ownership-withtests-r4.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100727/c1701f35/attachment.html>