[cfe-dev] __attribute__((alloc_size(foo))

[Nuno Lopes]

Alright, I'll commit a cleaned up version in a minute.

Thanks for your review!
Nuno


Citando Jordy Rose <jediknil at belkadan.com>:

> This is /very/ similar to Xi's code! ...
>
> Your patch allows users to use any number of arguments for  
> alloc_size; we should at least have a warning case for 0, but do we  
> want to allow more than 2? (I see no reason not to except that we  
> have to be very careful handling overflow, but "simplicity" isn't a  
> bad reason, and we could always change it later.)
>
> If we do go with just two args, I would suggest messing with  
> Attr.td's tablegen to add a DefaultUnsignedArgument, so you can use  
> [UnsignedArgument<"First">, DefaultUnsignedArgument<"Second",1>]  
> instead of [VariadicUnsignedArgument<"Args">].
>
> +    // check if the function argument is of an integer type
> +    QualType T = getFunctionOrMethodArgType(D, x).getNonReferenceType();
> +    if (!T->isIntegerType()) {
> +      S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_not_int)
> +      << "alloc_size" << x+1 << Ex->getSourceRange();
> +      continue;
> +    }
>
> Why continue here? We have to make sure not to attach the attribute  
> if any argument fails anyway, or we might get weird errors in Sema  
> later on.
>
> Eli's comments on the original patch apply here too:
>
>> +  unsigned* start = &AllocSizeArgs[0];
>> +  unsigned size = AllocSizeArgs.size();
>> +  llvm::array_pod_sort(start, start + size);
>>
>> Don't you need to check for duplicates?  Also, for an alloc_size
>> attribute which doesn't specify any indexes?
>>
>> +    // Is the function argument an integer type?
>> +    QualType T = getFunctionOrMethodArgType(D, x).getNonReferenceType();
>>
>> I don't think you want to allow applying alloc_size to arguments of  
>> type "int&".
>>
>> +    unsigned x = (unsigned) ArgNum.getZExtValue();
>>
>> This is unsafe; there's no guarantee ArgNum fits into a 64-bit integer
>> (which will cause an assert), and you're masking off the top 32 bits
>> of that 64-bit integer without any additional checks.  You should
>> perform the bounds checking on the APSInt, and then perform whatever
>> conversion is necessary.  (And if this is copied from existing cod
>> which does the same thing, please fix that as well.)
>
> Xi's second iteration addresses these concerns, noting that  
> handleNonNullAttr() has the same problems.
>
> Other than that it looks fairly good, and sorry, Xi, yours was  
> fairly good too, if a little different. I can't see why this  
> shouldn't go in after cleanup.
>
> Jordy