[cfe-dev] Ownership attribute for malloc etc. checking

[Jordy Rose]

A few last comments (along with what Ted had to say). Also, I second being
able to put the attributes on the arguments themselves. (Why didn't the GCC
people do nonnull like that?)


>> Here's the dispatch from EvalCallExpr(). I think even though it's
>> technically a little less efficient, you should just simplify this and
>> use
>> FD->getAttr<OwnershipTakesAttr>(), etc. It's a little simpler, and it
>> lets
>> you order the attributes.
>>
> 
> However, I did it this way because I wanted to allow two or more
attributes
> of the same kind (for the benefit of macros).
> 
> In other words, __attribute((ownership_holds, 1))
> __attribute((ownership_holds, 2)) should be the same
> as __attribute((ownership_holds, 1, 2)).
> 

Ah, good point. Okay, as long as return values are still handled (see
below).


>> Why does that matter? Because an ownership_takes() or ownership_holds()
>> function might have a return value! If it's also ownership_returns(),
>> then
>> you're fine, but otherwise you need to set a symbolic return value.
(You
>> can see how this is done in MallocMemAux(), or in StreamChecker.cpp
with
>> fopen.) There should probably be a test for this as well.
> 
> 
> It isn't necessarily the case that an ownership_takes() function that
> returns a pointer generated that pointer from an ownership_takes
argument,
> or allocated anything, so what would you set the region to, in the
absence
> of other information?  They default to Unknown, yes?

I believe so, but for return values from function calls we prefer to use a
"conjured symbol value". This is a kind of value that can be reasoned about
symbolically in later statements -- for example, we can bound its value if
it's an integer, or mark that it's null or non-null if it's a pointer.


>> What does it mean to have multiple size fields? There can still only be
>> one return value. Maybe you should only allow zero or one positions for
>> ownership_returns (which would make this section simpler).
>>
> 
> The intention here was to implement something for calloc-like functions
and
> matrix allocators; one argument is a special case, more than one the
size
> of
> the region is the product of the arguments times sizeof(pointee of the
> return value), taking sizeof(void) as 1.  If it isn't worth doing that,
> fair
> enough, but that's what I was thinking this could lead to.

It's a good idea, but consider the following examples:

Node *getNode (size_t extra) {
  return malloc(sizeof(Node)+extra);
}

Node *getNode (size_t total) {
  assert(total >= extra);
  return malloc(total);
}

Node *getNodes (size_t count) {
  return malloc(sizeof(Node)*count);
}

Your proposal is to use the second or third meaning, based on whether one
or more arguments are included. I think we should just pick one meaning and
stick with it (i.e. not special-case the one-argument case). Or just take
it out, though as long as we allow the zero-argument case it doesn't hurt
anyone to handle arguments in one particular way.

> However, the parser only allows the one argument at the moment, so this
is
> redundant.