[PATCH] Support the assume_aligned function attribute

Richard Smith richard at metafoo.co.uk
Sun Jul 20 16:25:34 PDT 2014


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.

================
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.

http://reviews.llvm.org/D4601






More information about the cfe-commits mailing list