[PATCH] Support the assume_aligned function attribute

[Aaron Ballman]

On Mon, Jul 21, 2014 at 12:25 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Aaron Ballman" <aaron.ballman at gmail.com>
>> To: reviews+D4601+public+521bc0d2736e59d3 at reviews.llvm.org
>> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Richard Smith" <richard at metafoo.co.uk>, "llvm cfe" <cfe-commits at cs.uiuc.edu>
>> Sent: Monday, July 21, 2014 11:22:01 AM
>> Subject: Re: [PATCH] Support the assume_aligned function attribute
>>
>> On Sun, Jul 20, 2014 at 7:25 PM, Richard Smith
>> <richard at metafoo.co.uk> wrote:
>> > Somewhat unrelated to this patch in particular, but we should add a
>> > UBSan check to catch failed assumptions.
>> >
>> > ================
>> > Comment at: include/clang/Basic/Attr.td:867
>> > @@ +866,3 @@
>> > +                             "ExpectedFunctionOrMethod">;
>> > +  let Args = [IntArgument<"Alignment">,
>> > DefaultIntArgument<"Offset", 0>];
>> > +  let Documentation = [Undocumented];
>> > ----------------
>> > These should probably both be `ExprArgument`s, in order to support
>> > dependent alignment/offset expressions in templates.
>> >
>> > ================
>> > Comment at: include/clang/Basic/Attr.td:868
>> > @@ +867,3 @@
>> > +  let Args = [IntArgument<"Alignment">,
>> > DefaultIntArgument<"Offset", 0>];
>> > +  let Documentation = [Undocumented];
>> > +}
>> > ----------------
>> > Please add documentation.
>> >
>> > ================
>> > Comment at: lib/CodeGen/CGExpr.cpp:3352
>> > @@ +3351,3 @@
>> > +
>> > +  if (Ret.isScalar() && TargetDecl)
>> > +    if (const AssumeAlignedAttr *AA =
>> > ----------------
>> > Please add parens around this `if`.
>> >
>> > ================
>> > Comment at: lib/Sema/SemaDeclAttr.cpp:1219-1227
>> > @@ +1218,11 @@
>> > +
>> > +  if (Attr.getNumArgs() > 2) {
>> > +    S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
>> > +      << Attr.getName() << 2;
>> > +    return;
>> > +  } else if (Attr.getNumArgs() < 1) {
>> > +    S.Diag(Attr.getLoc(), diag::err_attribute_too_few_arguments)
>> > +      << Attr.getName() << 1;
>> > +    return;
>> > +  }
>> > +
>> > ----------------
>> > I think there's a helper function for this sequence of
>> > checks/diagnostics. If not, there should be.
>>
>> They aren't required -- the common handler takes care of that stuff.
>
> No, I put them in specifically because the common handler did not. I noticed because when I was writing the regression tests, if I passed too few parameters (specifically none), the later call to getArg would assert.

Ahhhh shoot, the optional argument is why that's needed (see
handleCommonAttributeFeatures()). Sorry for leading you astray there;
I forgot that I hadn't gotten around to implementing optional argument
handlers there yet (which is where this logic really should live).

~Aaron