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

[Jordy Rose]

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