[PATCH] Support the assume_aligned function attribute

[Aaron Ballman]

On Mon, Jul 21, 2014 at 4:37 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
> On Mon, Jul 21, 2014 at 4:36 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Mon, Jul 21, 2014 at 9:22 AM, Aaron Ballman <aaron.ballman at gmail.com>
>> wrote:
>>>
>>> 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.
>>>
>>> >
>>> > ================
>>> > Comment at: lib/Sema/SemaDeclAttr.cpp:1232-1235
>>> > @@ +1231,6 @@
>>> > +    return;
>>> > +  if (Attr.getNumArgs() > 1 && !checkUInt32Argument(S, Attr,
>>> > +
>>> > Attr.getArgAsExpr(1),
>>> > +                                                    Offset, 2))
>>> > +    return;
>>> > +
>>> > ----------------
>>> > Do you really need to check that the `Offset` fits into an `uint32_t`?
>>> > Truncating the offset to the width of `Alignment` seems correct in all
>>> > cases.
>>>
>>> This doesn't check that it fits into a uint32_t. It checks that it's a
>>> valid integer constant expression and errs if it's not. It then gets
>>> you the value as a uint32_t.
>>
>>
>> Huh. That's a bug in the existing function, then; it should be checking for
>> truncation.
>
> I'd agree with that and can add the functionality to do it (it makes
> sense). I was only pointing out that it wasn't the (original) purpose
> of the function.

Implemented in r213658, thank you for the suggestion!

~Aaron