<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 21, 2014 at 9:22 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sun, Jul 20, 2014 at 7:25 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> Somewhat unrelated to this patch in particular, but we should add a UBSan check to catch failed assumptions.<br>
><br>
> ================<br>
> Comment at: include/clang/Basic/Attr.td:867<br>
> @@ +866,3 @@<br>
> +                             "ExpectedFunctionOrMethod">;<br>
> +  let Args = [IntArgument<"Alignment">, DefaultIntArgument<"Offset", 0>];<br>
> +  let Documentation = [Undocumented];<br>
> ----------------<br>
> These should probably both be `ExprArgument`s, in order to support dependent alignment/offset expressions in templates.<br>
><br>
> ================<br>
> Comment at: include/clang/Basic/Attr.td:868<br>
> @@ +867,3 @@<br>
> +  let Args = [IntArgument<"Alignment">, DefaultIntArgument<"Offset", 0>];<br>
> +  let Documentation = [Undocumented];<br>
> +}<br>
> ----------------<br>
> Please add documentation.<br>
><br>
> ================<br>
> Comment at: lib/CodeGen/CGExpr.cpp:3352<br>
> @@ +3351,3 @@<br>
> +<br>
> +  if (Ret.isScalar() && TargetDecl)<br>
> +    if (const AssumeAlignedAttr *AA =<br>
> ----------------<br>
> Please add parens around this `if`.<br>
><br>
> ================<br>
> Comment at: lib/Sema/SemaDeclAttr.cpp:1219-1227<br>
> @@ +1218,11 @@<br>
> +<br>
> +  if (Attr.getNumArgs() > 2) {<br>
> +    S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)<br>
> +      << Attr.getName() << 2;<br>
> +    return;<br>
> +  } else if (Attr.getNumArgs() < 1) {<br>
> +    S.Diag(Attr.getLoc(), diag::err_attribute_too_few_arguments)<br>
> +      << Attr.getName() << 1;<br>
> +    return;<br>
> +  }<br>
> +<br>
> ----------------<br>
> I think there's a helper function for this sequence of checks/diagnostics. If not, there should be.<br>
<br>
</div></div>They aren't required -- the common handler takes care of that stuff.<br>
<div class=""><br>
><br>
> ================<br>
> Comment at: lib/Sema/SemaDeclAttr.cpp:1232-1235<br>
> @@ +1231,6 @@<br>
> +    return;<br>
> +  if (Attr.getNumArgs() > 1 && !checkUInt32Argument(S, Attr,<br>
> +                                                    Attr.getArgAsExpr(1),<br>
> +                                                    Offset, 2))<br>
> +    return;<br>
> +<br>
> ----------------<br>
> 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.<br>
<br>
</div>This doesn't check that it fits into a uint32_t. It checks that it's a<br>
valid integer constant expression and errs if it's not. It then gets<br>
you the value as a uint32_t.</blockquote><div><br></div><div>Huh. That's a bug in the existing function, then; it should be checking for truncation. </div></div></div></div>