[PATCH] D29599: Clang Changes for alloc_align

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:1230
+  let Spellings = [GCC<"alloc_align">];
+  let Subjects = SubjectList<[ Function]>;
+  let Args = [IntArgument<"ParamIndex">];
----------------
There's a spurious space between [ and Function.

If we want to behave like GCC, then your subject list is fine. If we want to tighten up what we allow rather than silently accept the attribute and do nothing with it, this should probably be HasFunctionProto because the attribute doesn't seem to make sense on an unprototyped function.


================
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+  let Spellings = [ GCC<"alloc_align"> ];
+  let Subjects = SubjectList<[Function]>;
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Extra spaces in the declaration that do not match the style of the rest of the file (same happens below).
> Strangely, this is ClangFormat at work :/
Ah, I guess clang-format doesn't quite understand .td files.


================
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter, starting
+with 1.
----------------
aaron.ballman wrote:
> I would split the "starting with 1" off into its own (full) sentence for clarity purposes. Also, please spell out one instead of 1.
> 
> You may also want to clarify how member functions do/do not impact this index.
This does not appear to have been handled?


================
Comment at: lib/CodeGen/CGCall.cpp:4374
+    } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
+      llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Instead of hoping all of the callers of `getParamIndex()` will remember that this is a weird one-based thing, why not give the semantic attribute the correct index when attaching the attribute to the declaration?
> I played with this for a while, and the difficulty is that AddAllocAlignAttr requires the 1-based index, since it needs to error based on that number.  Additionally, decrementing the value BEFORE that function becomes difficult, since it is an Expr object at that time (which would get messy in the template case).
> 
> I cannot alter it in the AddAllocAlignAttr function, since that function actually gets called TWICE in the template instantiation case, so I'd likely need some sort of flag that told whether to treat it as decremented or not.  In general, this gets really ugly really quickly.
> 
> Basically, I don't see a good place to decrement the value anywhere without causing a much more significant change.  
Thank you for the explanation, that makes sense to me.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+  int IndexVal;
+  if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+                                /*Index=*/1))
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > It seems strange to me that you check that it's a positive integer argument before checking the param is an integer type.
> > 
> > Why not use `checkFunctionOrMethodParameterIndex()`?
> I'm unaware of checkFunctionOrMethodParameterIndex, there are a ton of odd free fucntions around here, I just copied from some of the surrounding functions.
> 
> That said, these two poorly named functions are actually checking 2 different pieces of data.  So in the case of:
> void func(int foo) __attribute__((alloc_align(1));
> 
> The "checkPositiveIntArgument" checks to ensure that '1' is a positive integer.  "checkParamIsIntegerType" checks that "foo" (the corresponding parameter) is an integer, and that '1' is in range.
Ah, I see (and yes, that function name is rather ambiguous). I think you should be using `checkFunctionOrMethodParameterIndex()` in place of `checkPositiveIntArgument()` and leave in the `checkParamIsIntegerType()`.


https://reviews.llvm.org/D29599