[PATCH] D29599: Clang Changes for alloc_align

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 15 inline comments as done.
erichkeane added a comment.

Comments on 2 cases, otherwise a Patch incoming that fixes the rest of Aaron's comments.



================
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+  let Spellings = [ GCC<"alloc_align"> ];
+  let Subjects = SubjectList<[Function]>;
----------------
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 :/


================
Comment at: include/clang/Basic/Attr.td:1225
+  let Spellings = [ GCC<"alloc_align"> ];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [ IntArgument<"ParamIndex"> ];
----------------
aaron.ballman wrote:
> Is this subject line correct, or should it be using `HasFunctionProto` instead? How does GCC handle something like `void *blah() __attribute__((alloc_align(1)));` in C code?
I'm not sure what HasFunctionProto means in this case.

Either way, GCC does basically ZERO SEMA here, so GCC would allow that to happen, then simply ignore the attribute.  We're enforcing that in the SEMA changes below.


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


================
Comment at: lib/Sema/SemaDeclAttr.cpp:806
+/// \brief Checks to be sure that the given parameter number is inbounds, and is
+/// an some integral type. Will emit appropriate diagnostics if this returns
+/// false.
----------------
aaron.ballman wrote:
> Remove the "some".
This is what I get for just C&P'ing the existing comment :)


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+  int IndexVal;
+  if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+                                /*Index=*/1))
----------------
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.


https://reviews.llvm.org/D29599