Clang Changes for alloc_align

[erichkeane]

Summary

GCC has the alloc_align attribute, which is similar to assume_aligned, except the attribute's parameter is the index of the integer parameter that needs aligning to.

[erichkeane]

Requires this LLVM commit: https://reviews.llvm.org/D29598

[ahatanak]

Can this attribute be used on c++ template methods? Is the following code valid?

template<class T>
struct S {
  T foo(int a)  __attribute__((alloc_align(1)));
};

[erichkeane]

In D29599#674772, @ahatanak wrote:

Can this attribute be used on c++ template methods? Is the following code valid?

template<class T>
struct S {
  T foo(int a)  __attribute__((alloc_align(1)));
};

Yes it can, however that example will NOT compile. First, on member functions you cannot use alloc_align to refer to the 'this' function. Second, the return value must be a pointer, so if remove_pointer<T> == T, this will fail.

[ahatanak]

My question probably wasn't clear, but I wasn't sure how template functions in general (not just member functions) should be handled.

I see a warning when the following function is compiled:

template<class T>
T  __attribute__((alloc_align(1))) foo0(int a) {
  typedef typename std::remove_pointer<T>::type T2;
  return new T2();
}

void foo1() {
  foo0<int*>(1);
}

clang complains that the attribute only applies to functions returning pointers or references, but foo0<int*> does return a pointer. Is that intended?

[erichkeane]

Ah, I see! Sorry for missing that. I don't see a reason why we cannot support that, but I wasn't really considering it. In general, this attribute is a compiler hint for some C standard library stuff in glibc.

I've been playing with it a few hours now, and it seems that GCC ONLY complains about the attribute parameter being in range (same error for the type of the parameter as well!), so we DO a bit extra SEMA here, though I think that is correct. I'll dig into this a bit more and see if I can get the template return type working correctly.

[erichkeane]

I was able to get the templated versions working in response to the discussion with Akira. Note the added test file which shows off all of the combos I could think of.

It required a little bit of surgery inside the SemaDeclAttr.cpp, since the SemaTemplateInstatiateDecl.cpp no longer has "AttributeList" info anymore, so getting the error messages in the existing functions required a little template-writing of my own!

I decided to explicitly forbid the following case, because I cannot see a valid usecase for this, or for making 'Which' below a dependent value.

template<int Which>
void* foo(int a, int b, int c, int d) __attribute__((alloc_align(Which)));

[erichkeane]

Ping!

[myatsina]

test/CodeGen/alloc-align-attr.c

19 ↗

(On Diff #88602)

Where exactly do we see this test2 param casting?
I think you have a missing check before the m1 call

[erichkeane]

Thanks for the review! Tests updated to clarify the cast htat is happening.

[erichkeane]

Update test based on the corresponding LLVM change.

[erichkeane]

FWIW, the LLVM changes for this have been committed.

[aaron.ballman]

include/clang/Basic/Attr.td

1224 ↗

(On Diff #92814)

Extra spaces in the declaration that do not match the style of the rest of the file (same happens below).

1225 ↗

(On Diff #92814)

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?

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #92814)

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.

256 ↗

(On Diff #92814)

I think you need a newline above this code block to not trigger sphinx diagnostics.

lib/CodeGen/CGCall.cpp

4374 ↗

(On Diff #92814)

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?

4377 ↗

(On Diff #92814)

Spurious newline should be removed.

lib/Sema/SemaDeclAttr.cpp

222 ↗

(On Diff #92814)

Missing a full stop at the end of the comment.

223 ↗

(On Diff #92814)

s/class/typename (same below).

Also, add some newlines between the function definitions and run the patch through clang-format (pointers and references are bound to the wrong thing and the formatting seems a bit off).

232 ↗

(On Diff #92814)

Missing a full stop at the end of the comment.

775 ↗

(On Diff #92814)

Should be removed.

793 ↗

(On Diff #92814)

Can elide the braces.

805 ↗

(On Diff #92814)

s/inbounds/in bounds

806 ↗

(On Diff #92814)

Remove the "some".

1599 ↗

(On Diff #92814)

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()?

test/Sema/alloc-align-attr.c

5 ↗

(On Diff #92814)

This test doesn't really add value.

11 ↗

(On Diff #92814)

Same with this one.

16 ↗

(On Diff #92814)

Same.

test/SemaCXX/alloc-align-attr.cpp

11 ↗

(On Diff #92814)

Spurious newline.

[erichkeane]

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

include/clang/Basic/Attr.td

1224 ↗

(On Diff #92814)

Strangely, this is ClangFormat at work :/

1225 ↗

(On Diff #92814)

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.

lib/CodeGen/CGCall.cpp

4374 ↗

(On Diff #92814)

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.

lib/Sema/SemaDeclAttr.cpp

806 ↗

(On Diff #92814)

This is what I get for just C&P'ing the existing comment :)

1599 ↗

(On Diff #92814)

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.

[aaron.ballman]

include/clang/Basic/Attr.td

1230 ↗

(On Diff #93158)

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.

1224 ↗

(On Diff #92814)

Ah, I guess clang-format doesn't quite understand .td files.

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #92814)

This does not appear to have been handled?

lib/CodeGen/CGCall.cpp

4374 ↗

(On Diff #92814)

Thank you for the explanation, that makes sense to me.

lib/Sema/SemaDeclAttr.cpp

1599 ↗

(On Diff #92814)

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().

[erichkeane]

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #92814)

Ah, I missed that you wanted it in its own sentence, not just its own line. I'll expand on this text in the next patch.

[erichkeane]

Made the changes as requested. checkFunctionOrMethodParameterIndex corrects for 1->0 index and implicit this, which requires undoing, otherwise templates create a big hassle. Additionally, please note the AttrDocs changes, I think I've got it acceptable but would love a second english-smith.

[aaron.ballman]

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #93404)

Use single spacing instead of double spacing.

253 ↗

(On Diff #93404)

s/one-index/one-based?

member functions -> nonstatic member functions

254 ↗

(On Diff #93404)

is considered as as the first -> is the first

258 ↗

(On Diff #93404)

Missing full stop.

261 ↗

(On Diff #93404)

Same here.

262 ↗

(On Diff #93404)

Pointer should bind to v (same below).

lib/Sema/SemaDeclAttr.cpp

809 ↗

(On Diff #93404)

This can be hoisted up to the previous line.

1530 ↗

(On Diff #93404)

There's no real need for E.

1532 ↗

(On Diff #93404)

This formatting looks off (the args should align).

1608–1612 ↗

(On Diff #93404)

Hmmm, I think this might be a bit more clean as:

QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
if (!Ty->isIntegralType(Context)) {
  Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only) << TmpAttr <<
     FuncDecl->getParamDecl(IndexVal)->getSourceRange();
  return;
}

// We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
// because that has corrected for the implicit this parameter, and is zero-
// based.  The attribute expects what the user wrote explicitly.
llvm::APSInt Val;
ParamExpr->EvaluateAsInt(Val, S.Context);

// Use Val.getZExtValue() when you need what the user wrote.

This matches more closely with how other attributes handle the situation where the Attr object needs what the user wrote (like format_arg). I hadn't noticed that checkParamIsIntegerType() turns around and calls checkFunctionOrMethodParameterIndex() again, and this would simplify your patch a bit.

What do you think?

[erichkeane]

lib/Sema/SemaDeclAttr.cpp

1608–1612 ↗

(On Diff #93404)

Looks better, I hadn't realized checkParamIsIntegerType was redoing work either.

I had to make a pair of small changes to this (including omitting DependentType from the integral type check), but it otherwise works nicely. Thanks!

[rjmccall]

I see that GCC is up to its same parameter-indexing shenanigans again.

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #93409)

"that the return value of the function ... is at least as aligned as the
value of the indicated parameter. The parameter is given by its index
in the list of formal parameters; the first parameter has index 1 unless
the function is a C++ non-static member function, in which case the
first parameter has index 2 to account for the implicit `this` parameter."

What's the behavior of this attribute if the given attribute is not a power of 2?
Does it silently have no effect, or is it U.B.?

265 ↗

(On Diff #93409)

"The returned pointer has the alignment specified by the second formal
parameter, 'align'. However, for the purposes of the alloc_align attribute,
the parameter index is 3 because of the implicit 'this' parameter."

270 ↗

(On Diff #93409)

"Note that this attribute merely informs the compiler that a function always
returns a sufficiently aligned pointer. It does not cause the compiler to emit
code to enforce that alignment. The behavior is undefined if the returned
pointer is not sufficiently aligned."

lib/CodeGen/CGCall.cpp

4352 ↗

(On Diff #93409)

IRCallArgs is not one-to-one with the list of formal parameters. You need to be taking the value out of CallArgList. Three test cases come to mind:

1. There's an earlier struct argument which expands to no IR arguments.
2. There's an earlier struct argument which expands to multiple IR arguments.
3. The alignment argument itself expands to multiple IR arguments. For example, __int128_t on x86-64.

[erichkeane]

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #93409)

Thanks for the wording, I like this.

It isn't checked for that, so I'll say UB. A little bit of a caveat emptor here.

270 ↗

(On Diff #93409)

Thanks again!

lib/CodeGen/CGCall.cpp

4352 ↗

(On Diff #93409)

Awesome, thanks for the info! I'll change that.

[erichkeane]

Fixes based on John's comments. A little bit of extra work was required to get the correct Value from the attribute, but impact was minimal.

[rjmccall]

include/clang/Basic/AttrDocs.td

252 ↗

(On Diff #93421)

"value *of the* indicated parameter"

254 ↗

(On Diff #93421)

"has"

lib/CodeGen/CGCall.cpp

4363 ↗

(On Diff #93421)

Your old code was fine, you just needed to get the value with CallArgs[index].second.getScalarVal() instead of IRCallArgs[index].

[erichkeane]

Thanks for the feedback John!

[rjmccall]

lib/CodeGen/CGCall.cpp

4363 ↗

(On Diff #93421)

Please add the test cases I suggested here. You might have to make part of the test target-specific.

[erichkeane]

Added tests as requested by John.

[rjmccall]

Thanks. With that, LGTM.

[aaron.ballman]

LGTM!

Can you drop the svn props on the new files when you commit? I don't think we usually set them, and I've seen commits specifically removing the eol-style props before.

[erichkeane]

Thanks guys. I THINK I properly removed the svn properties properly, though, I actually didn't know they existed until just now!

cfe/trunk/include/clang/Basic/Attr.td

 
 def AssumeAligned : InheritableAttr {
   let Spellings = [GCC<"assume_aligned">];
   let Subjects = SubjectList<[ObjCMethod, Function]>;
   let Args = [ExprArgument<"Alignment">, ExprArgument<"Offset", 1>];
   let Documentation = [AssumeAlignedDocs];
 }
 
+def AllocAlign : InheritableAttr {
+  let Spellings = [GCC<"alloc_align">];
+  let Subjects = SubjectList<[HasFunctionProto], WarnDiag,
+                             "ExpectedFunctionWithProtoType">;
+  let Args = [IntArgument<"ParamIndex">];
+  let Documentation = [AllocAlignDocs];
+}
+
 def NoReturn : InheritableAttr {
   let Spellings = [GCC<"noreturn">, Declspec<"noreturn">];
   // FIXME: Does GCC allow this on the function instead?
   let Documentation = [Undocumented];
 }
 
 def NoInstrumentFunction : InheritableAttr {
   let Spellings = [GCC<"no_instrument_function">];

cfe/trunk/include/clang/Basic/AttrDocs.td

.. Note:: This attribute works differently in clang than it does in GCC.
   Specifically, clang will only trace ``const`` pointers (as above); we give up
   on pointers that are not marked as ``const``. In the vast majority of cases,
   this is unimportant, because LLVM has support for the ``alloc_size``
   attribute. However, this may cause mildly unintuitive behavior when used with
   other attributes, such as ``enable_if``.
   }];
 }
 
+def AllocAlignDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use ``__attribute__((alloc_align(<alignment>))`` on a function
+declaration to specify that the return value of the function (which must be a
+pointer type) is at least as aligned as the value of the indicated parameter. The 
+parameter is given by its index in the list of formal parameters; the first
+parameter has index 1 unless the function is a C++ non-static member function,
+in which case the first parameter has index 2 to account for the implicit ``this``
+parameter.
+
+.. code-block:: c++
+
+  // The returned pointer has the alignment specified by the first parameter.
+  void *a(size_t align) __attribute__((alloc_align(1)));
+
+  // The returned pointer has the alignment specified by the second parameter.
+  void *b(void *v, size_t align) __attribute__((alloc_align(2)));
+
+  // The returned pointer has the alignment specified by the second visible
+  // parameter, however it must be adjusted for the implicit 'this' parameter.
+  void *Foo::b(void *v, size_t align) __attribute__((alloc_align(3)));
+
+Note that this attribute merely informs the compiler that a function always
+returns a sufficiently aligned pointer. It does not cause the compiler to 
+emit code to enforce that alignment.  The behavior is undefined if the returned
+poitner is not sufficiently aligned.
+  }];
+}
+
 def EnableIfDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 .. Note:: Some features of this attribute are experimental. The meaning of
   multiple enable_if attributes on a single declaration is subject to change in
   a future version of clang. Also, the ABI is not standardized and the name
   mangling may change in future versions. To avoid that, use asm labels.
 

cfe/trunk/include/clang/Sema/Sema.h

   void AddAlignedAttr(SourceRange AttrRange, Decl *D, TypeSourceInfo *T,
                       unsigned SpellingListIndex, bool IsPackExpansion);
 
   /// AddAssumeAlignedAttr - Adds an assume_aligned attribute to a particular
   /// declaration.
   void AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E, Expr *OE,
                             unsigned SpellingListIndex);
 
+  /// AddAllocAlignAttr - Adds an alloc_align attribute to a particular
+  /// declaration.
+  void AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
+                         unsigned SpellingListIndex);
+
   /// AddAlignValueAttr - Adds an align_value attribute to a particular
   /// declaration.
   void AddAlignValueAttr(SourceRange AttrRange, Decl *D, Expr *E,
                          unsigned SpellingListIndex);
 
   /// AddLaunchBoundsAttr - Adds a launch_bounds attribute to a particular
   /// declaration.
   void AddLaunchBoundsAttr(SourceRange AttrRange, Decl *D, Expr *MaxThreads,

cfe/trunk/lib/CodeGen/CGCall.cpp

if (const auto *AA = TargetDecl->getAttr<AssumeAlignedAttr>()) {
       llvm::Value *OffsetValue = nullptr;
       if (const auto *Offset = AA->getOffset())
         OffsetValue = EmitScalarExpr(Offset);
 
       llvm::Value *Alignment = EmitScalarExpr(AA->getAlignment());
       llvm::ConstantInt *AlignmentCI = cast<llvm::ConstantInt>(Alignment);
       EmitAlignmentAssumption(Ret.getScalarVal(), AlignmentCI->getZExtValue(),
                               OffsetValue);
+    } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
+      llvm::Value *ParamVal =
+          CallArgs[AA->getParamIndex() - 1].RV.getScalarVal();
+      EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
     }
   }
 
   return Ret;
 }
 
 /* VarArg handling */
 

cfe/trunk/lib/CodeGen/CodeGenFunction.h

#endif
   /// Pass the result to unprotectFromPeepholes to declare that
   /// protection is no longer required.
   ///
   /// There's no particular reason why this shouldn't apply to
   /// l-values, it's just that no existing peepholes work on pointers.
   PeepholeProtection protectFromPeepholes(RValue rvalue);
   void unprotectFromPeepholes(PeepholeProtection protection);
 
+  void EmitAlignmentAssumption(llvm::Value *PtrValue, llvm::Value *Alignment,
+                               llvm::Value *OffsetValue = nullptr) {
+    Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
+                                      OffsetValue);
+  }
+
   //===--------------------------------------------------------------------===//
   //                             Statement Emission
   //===--------------------------------------------------------------------===//
 
   /// EmitStopPoint - Emit a debug stoppoint if we are emitting debug info.
   void EmitStopPoint(const Stmt *S);
 
   /// EmitStmt - Emit the code for the statement \arg S. It is legal to call

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

 /// output an error.
 static bool checkAttributeAtMostNumArgs(Sema &S, const AttributeList &Attr,
                                          unsigned Num) {
   return checkAttributeNumArgsImpl(S, Attr, Num,
                                    diag::err_attribute_too_many_arguments,
                                    std::greater<unsigned>());
 }
 
+/// \brief A helper function to provide Attribute Location for the Attr types
+/// AND the AttributeList.
+template <typename AttrInfo>
+static typename std::enable_if<std::is_base_of<clang::Attr, AttrInfo>::value,
+                               SourceLocation>::type
+getAttrLoc(const AttrInfo &Attr) {
+  return Attr.getLocation();
+}
+static SourceLocation getAttrLoc(const clang::AttributeList &Attr) {
+  return Attr.getLoc();
+}
+
+/// \brief A helper function to provide Attribute Name for the Attr types
+/// AND the AttributeList.
+template <typename AttrInfo>
+static typename std::enable_if<std::is_base_of<clang::Attr, AttrInfo>::value,
+                               const AttrInfo *>::type
+getAttrName(const AttrInfo &Attr) {
+  return &Attr;
+}
+const IdentifierInfo *getAttrName(const clang::AttributeList &Attr) {
+  return Attr.getName();
+}
+
 /// \brief If Expr is a valid integer constant, get the value of the integer
 /// expression and return success or failure. May output an error.
-static bool checkUInt32Argument(Sema &S, const AttributeList &Attr,
-                                const Expr *Expr, uint32_t &Val,
-                                unsigned Idx = UINT_MAX) {
+template<typename AttrInfo>
+static bool checkUInt32Argument(Sema &S, const AttrInfo& Attr, const Expr *Expr,
+                                uint32_t &Val, unsigned Idx = UINT_MAX) {
   llvm::APSInt I(32);
   if (Expr->isTypeDependent() || Expr->isValueDependent() ||
       !Expr->isIntegerConstantExpr(I, S.Context)) {
     if (Idx != UINT_MAX)
-      S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-        << Attr.getName() << Idx << AANT_ArgumentIntegerConstant
+      S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+        << getAttrName(Attr) << Idx << AANT_ArgumentIntegerConstant
         << Expr->getSourceRange();
     else
-      S.Diag(Attr.getLoc(), diag::err_attribute_argument_type)
-        << Attr.getName() << AANT_ArgumentIntegerConstant
+      S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_type)
+        << getAttrName(Attr) << AANT_ArgumentIntegerConstant
         << Expr->getSourceRange();
     return false;
   }
 
   if (!I.isIntN(32)) {
     S.Diag(Expr->getExprLoc(), diag::err_ice_too_large)
         << I.toString(10, false) << 32 << /* Unsigned */ 1;
     return false;
   }
 
   Val = (uint32_t)I.getZExtValue();
   return true;
 }
 
 /// \brief Wrapper around checkUInt32Argument, with an extra check to be sure
 /// that the result will fit into a regular (signed) int. All args have the same
 /// purpose as they do in checkUInt32Argument.
-static bool checkPositiveIntArgument(Sema &S, const AttributeList &Attr,
-                                     const Expr *Expr, int &Val,
-                                     unsigned Idx = UINT_MAX) {
+template<typename AttrInfo>
+static bool checkPositiveIntArgument(Sema &S, const AttrInfo& Attr, const Expr *Expr,
+                                     int &Val, unsigned Idx = UINT_MAX) {
   uint32_t UVal;
   if (!checkUInt32Argument(S, Attr, Expr, UVal, Idx))
     return false;
 
   if (UVal > (uint32_t)std::numeric_limits<int>::max()) {
     llvm::APSInt I(32); // for toString
     I = UVal;
     S.Diag(Expr->getExprLoc(), diag::err_ice_too_large)
static bool checkAttrMutualExclusion(Sema &S, Decl *D, SourceRange Range,
   }
   return false;
 }
 
 /// \brief Check if IdxExpr is a valid parameter index for a function or
 /// instance method D.  May output an error.
 ///
 /// \returns true if IdxExpr is a valid index.
-static bool checkFunctionOrMethodParameterIndex(Sema &S, const Decl *D,
-                                                const AttributeList &Attr,
-                                                unsigned AttrArgNum,
-                                                const Expr *IdxExpr,
-                                                uint64_t &Idx) {
+template <typename AttrInfo>
+static bool checkFunctionOrMethodParameterIndex(
+    Sema &S, const Decl *D, const AttrInfo& Attr,
+    unsigned AttrArgNum, const Expr *IdxExpr, uint64_t &Idx) {
   assert(isFunctionOrMethodOrBlock(D));
 
   // In C++ the implicit 'this' function parameter also counts.
   // Parameters are counted from one.
   bool HP = hasFunctionProto(D);
   bool HasImplicitThisParam = isInstanceMethod(D);
   bool IV = HP && isFunctionOrMethodVariadic(D);
   unsigned NumParams =
       (HP ? getFunctionOrMethodNumParams(D) : 0) + HasImplicitThisParam;
 
   llvm::APSInt IdxInt;
   if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent() ||
       !IdxExpr->isIntegerConstantExpr(IdxInt, S.Context)) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_argument_n_type)
-      << Attr.getName() << AttrArgNum << AANT_ArgumentIntegerConstant
+    S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_n_type)
+      << getAttrName(Attr) << AttrArgNum << AANT_ArgumentIntegerConstant
       << IdxExpr->getSourceRange();
     return false;
   }
 
   Idx = IdxInt.getLimitedValue();
   if (Idx < 1 || (!IV && Idx > NumParams)) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_bounds)
-      << Attr.getName() << AttrArgNum << IdxExpr->getSourceRange();
+    S.Diag(getAttrLoc(Attr), diag::err_attribute_argument_out_of_bounds)
+      << getAttrName(Attr) << AttrArgNum << IdxExpr->getSourceRange();
     return false;
   }
   Idx--; // Convert to zero-based.
   if (HasImplicitThisParam) {
     if (Idx == 0) {
-      S.Diag(Attr.getLoc(),
+      S.Diag(getAttrLoc(Attr),
              diag::err_attribute_invalid_implicit_this_argument)
-        << Attr.getName() << IdxExpr->getSourceRange();
+        << getAttrName(Attr) << IdxExpr->getSourceRange();
       return false;
     }
     --Idx;
   }
 
   return true;
 }
 
static void handleAssertExclusiveLockAttr(Sema &S, Decl *D,
   unsigned Size = Args.size();
   Expr **StartArg = Size == 0 ? nullptr : &Args[0];
   D->addAttr(::new (S.Context)
              AssertExclusiveLockAttr(Attr.getRange(), S.Context,
                                      StartArg, Size,
                                      Attr.getAttributeSpellingListIndex()));
 }
 
-/// \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
+/// \brief Checks to be sure that the given parameter number is in bounds, and is
+/// an integral type. Will emit appropriate diagnostics if this returns
 /// false.
 ///
 /// FuncParamNo is expected to be from the user, so is base-1. AttrArgNo is used
 /// to actually retrieve the argument, so it's base-0.
+template <typename AttrInfo>
 static bool checkParamIsIntegerType(Sema &S, const FunctionDecl *FD,
-                                    const AttributeList &Attr,
-                                    unsigned FuncParamNo, unsigned AttrArgNo) {
-  assert(Attr.isArgExpr(AttrArgNo) && "Expected expression argument");
+                                    const AttrInfo &Attr, Expr *AttrArg,
+                                    unsigned FuncParamNo, unsigned AttrArgNo,
+                                    bool AllowDependentType = false) {
   uint64_t Idx;
-  if (!checkFunctionOrMethodParameterIndex(S, FD, Attr, FuncParamNo,
-                                           Attr.getArgAsExpr(AttrArgNo), Idx))
+  if (!checkFunctionOrMethodParameterIndex(S, FD, Attr, FuncParamNo, AttrArg,
+                                           Idx))
     return false;
 
   const ParmVarDecl *Param = FD->getParamDecl(Idx);
+  if (AllowDependentType && Param->getType()->isDependentType())
+    return true;
   if (!Param->getType()->isIntegerType() && !Param->getType()->isCharType()) {
-    SourceLocation SrcLoc = Attr.getArgAsExpr(AttrArgNo)->getLocStart();
+    SourceLocation SrcLoc = AttrArg->getLocStart();
     S.Diag(SrcLoc, diag::err_attribute_integers_only)
-        << Attr.getName() << Param->getSourceRange();
+        << getAttrName(Attr) << Param->getSourceRange();
     return false;
   }
   return true;
 }
 
+/// \brief Checks to be sure that the given parameter number is in bounds, and is
+/// an integral type. Will emit appropriate diagnostics if this returns false.
+///
+/// FuncParamNo is expected to be from the user, so is base-1. AttrArgNo is used
+/// to actually retrieve the argument, so it's base-0.
+static bool checkParamIsIntegerType(Sema &S, const FunctionDecl *FD,
+                                    const AttributeList &Attr,
+                                    unsigned FuncParamNo, unsigned AttrArgNo,
+                                    bool AllowDependentType = false) {
+  assert(Attr.isArgExpr(AttrArgNo) && "Expected expression argument");
+  return checkParamIsIntegerType(S, FD, Attr, Attr.getArgAsExpr(AttrArgNo),
+                                 FuncParamNo, AttrArgNo, AllowDependentType);
+}
+
 static void handleAllocSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1) ||
       !checkAttributeAtMostNumArgs(S, Attr, 2))
     return;
 
   const auto *FD = cast<FunctionDecl>(D);
   if (!FD->getReturnType()->isPointerType()) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_return_pointers_only)

 static void handleAssumeAlignedAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
   Expr *E = Attr.getArgAsExpr(0),
        *OE = Attr.getNumArgs() > 1 ? Attr.getArgAsExpr(1) : nullptr;
   S.AddAssumeAlignedAttr(Attr.getRange(), D, E, OE,
                          Attr.getAttributeSpellingListIndex());
 }
 
+static void handleAllocAlignAttr(Sema &S, Decl *D,
+                                   const AttributeList &Attr) {
+  S.AddAllocAlignAttr(Attr.getRange(), D, Attr.getArgAsExpr(0),
+                      Attr.getAttributeSpellingListIndex());
+}
+
 void Sema::AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
                                 Expr *OE, unsigned SpellingListIndex) {
   QualType ResultType = getFunctionOrMethodResultType(D);
   SourceRange SR = getFunctionOrMethodResultSourceRange(D);
 
   AssumeAlignedAttr TmpAttr(AttrRange, Context, E, OE, SpellingListIndex);
   SourceLocation AttrLoc = AttrRange.getBegin();
 
if (!OE->isValueDependent()) {
       }
     }
   }
 
   D->addAttr(::new (Context)
             AssumeAlignedAttr(AttrRange, Context, E, OE, SpellingListIndex));
 }
 
+void Sema::AddAllocAlignAttr(SourceRange AttrRange, Decl *D, Expr *ParamExpr,
+                             unsigned SpellingListIndex) {
+  QualType ResultType = getFunctionOrMethodResultType(D);
+
+  AllocAlignAttr TmpAttr(AttrRange, Context, 0, SpellingListIndex);
+  SourceLocation AttrLoc = AttrRange.getBegin();
+
+  if (!ResultType->isDependentType() &&
+      !isValidPointerAttrType(ResultType, /* RefOkay */ true)) {
+    Diag(AttrLoc, diag::warn_attribute_return_pointers_refs_only)
+        << &TmpAttr << AttrRange << getFunctionOrMethodResultSourceRange(D);
+    return;
+  }
+
+  uint64_t IndexVal;
+  const auto *FuncDecl = cast<FunctionDecl>(D);
+  if (!checkFunctionOrMethodParameterIndex(*this, FuncDecl, TmpAttr,
+                                           /*AttrArgNo=*/1, ParamExpr,
+                                           IndexVal))
+    return;
+
+  QualType Ty = getFunctionOrMethodParamType(D, IndexVal);
+  if (!Ty->isDependentType() && !Ty->isIntegralType(Context)) {
+    Diag(ParamExpr->getLocStart(), diag::err_attribute_integers_only)
+        << &TmpAttr << FuncDecl->getParamDecl(IndexVal)->getSourceRange();
+    return;
+  }
+
+  // We cannot use the Idx returned from checkFunctionOrMethodParameterIndex
+  // because that has corrected for the implicit this parameter, and is zero-
+  // based.  The attribute expects what the user wrote explicitly.
+  llvm::APSInt Val;
+  ParamExpr->EvaluateAsInt(Val, Context);
+
+  D->addAttr(::new (Context) AllocAlignAttr(
+      AttrRange, Context, Val.getZExtValue(), SpellingListIndex));
+}
+
 /// Normalize the attribute, __foo__ becomes foo.
 /// Returns true if normalization was applied.
 static bool normalizeName(StringRef &AttrName) {
   if (AttrName.size() > 4 && AttrName.startswith("__") &&
       AttrName.endswith("__")) {
     AttrName = AttrName.drop_front(2).drop_back(2);
     return true;
   }
else
       handleNonNullAttr(S, D, Attr);
     break;
   case AttributeList::AT_ReturnsNonNull:
     handleReturnsNonNullAttr(S, D, Attr);
     break;
   case AttributeList::AT_AssumeAligned:
     handleAssumeAlignedAttr(S, D, Attr);
     break;
+  case AttributeList::AT_AllocAlign:
+    handleAllocAlignAttr(S, D, Attr);
+    break;
   case AttributeList::AT_Overloadable:
     handleSimpleAttribute<OverloadableAttr>(S, D, Attr);
     break;
   case AttributeList::AT_Ownership:
     handleOwnershipAttr(S, D, Attr);
     break;
   case AttributeList::AT_Cold:
     handleColdAttr(S, D, Attr);

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

static void instantiateDependentAlignValueAttr(
   // The alignment expression is a constant expression.
   EnterExpressionEvaluationContext Unevaluated(S, Sema::ConstantEvaluated);
   ExprResult Result = S.SubstExpr(Aligned->getAlignment(), TemplateArgs);
   if (!Result.isInvalid())
     S.AddAlignValueAttr(Aligned->getLocation(), New, Result.getAs<Expr>(),
                         Aligned->getSpellingListIndex());
 }
 
+static void instantiateDependentAllocAlignAttr(
+    Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
+    const AllocAlignAttr *Align, Decl *New) {
+  Expr *Param = IntegerLiteral::Create(
+      S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+      S.getASTContext().UnsignedLongLongTy, Align->getLocation());
+  S.AddAllocAlignAttr(Align->getLocation(), New, Param,
+                      Align->getSpellingListIndex());
+}
+
 static Expr *instantiateDependentFunctionAttrCondition(
     Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
     const Attr *A, Expr *OldCond, const Decl *Tmpl, FunctionDecl *New) {
   Expr *Cond = nullptr;
   {
     Sema::ContextRAII SwitchContext(S, New);
     EnterExpressionEvaluationContext Unevaluated(S, Sema::ConstantEvaluated);
     ExprResult Result = S.SubstExpr(OldCond, TemplateArgs);
for (const auto *TmplAttr : Tmpl->attrs()) {
     }
 
     const AlignValueAttr *AlignValue = dyn_cast<AlignValueAttr>(TmplAttr);
     if (AlignValue) {
       instantiateDependentAlignValueAttr(*this, TemplateArgs, AlignValue, New);
       continue;
     }
 
+    if (const auto *AllocAlign = dyn_cast<AllocAlignAttr>(TmplAttr)) {
+      instantiateDependentAllocAlignAttr(*this, TemplateArgs, AllocAlign, New);
+      continue;
+    }
+
+
     if (const auto *EnableIf = dyn_cast<EnableIfAttr>(TmplAttr)) {
       instantiateDependentEnableIfAttr(*this, TemplateArgs, EnableIf, Tmpl,
                                        cast<FunctionDecl>(New));
       continue;
     }
 
     if (const auto *DiagnoseIf = dyn_cast<DiagnoseIfAttr>(TmplAttr)) {
       instantiateDependentDiagnoseIfAttr(*this, TemplateArgs, DiagnoseIf, Tmpl,

cfe/trunk/test/CodeGen/alloc-align-attr.c

+// RUN: %clang_cc1 -triple x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+__INT32_TYPE__*m1(__INT32_TYPE__ i) __attribute__((alloc_align(1)));
+
+// Condition where parameter to m1 is not size_t.
+__INT32_TYPE__ test1(__INT32_TYPE__ a) {
+// CHECK: define i32 @test1
+  return *m1(a);
+// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]]) 
+// CHECK: [[ALIGNCAST1:%.+]] = sext i32 [[PARAM1]] to i64
+// CHECK: [[ISPOS1:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
+// CHECK: [[POSMASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
+// CHECK: [[MASK1:%.+]] = select i1 [[ISPOS1]], i64 [[POSMASK1]], i64 0
+// CHECK: [[PTRINT1:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR1:%.+]] = and i64 [[PTRINT1]], [[MASK1]]
+// CHECK: [[MASKCOND1:%.+]] = icmp eq i64 [[MASKEDPTR1]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND1]])
+}
+// Condition where test2 param needs casting.
+__INT32_TYPE__ test2(__SIZE_TYPE__ a) {
+// CHECK: define i32 @test2
+  return *m1(a);
+// CHECK: [[CONV2:%.+]] = trunc i64 %{{.+}} to i32
+// CHECK: call i32* @m1(i32 [[CONV2]])
+// CHECK: [[ALIGNCAST2:%.+]] = sext i32 [[CONV2]] to i64
+// CHECK: [[ISPOS2:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
+// CHECK: [[POSMASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
+// CHECK: [[MASK2:%.+]] = select i1 [[ISPOS2]], i64 [[POSMASK2]], i64 0
+// CHECK: [[PTRINT2:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR2:%.+]] = and i64 [[PTRINT2]], [[MASK2]]
+// CHECK: [[MASKCOND2:%.+]] = icmp eq i64 [[MASKEDPTR2]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND2]])
+}
+__INT32_TYPE__ *m2(__SIZE_TYPE__ i) __attribute__((alloc_align(1)));
+
+// test3 param needs casting, but 'm2' is correct.
+__INT32_TYPE__ test3(__INT32_TYPE__ a) {
+// CHECK: define i32 @test3
+  return *m2(a);
+// CHECK: [[CONV3:%.+]] = sext i32 %{{.+}} to i64
+// CHECK: call i32* @m2(i64 [[CONV3]])
+// CHECK: [[ISPOS3:%.+]] = icmp sgt i64 [[CONV3]], 0
+// CHECK: [[POSMASK3:%.+]] = sub i64 [[CONV3]], 1
+// CHECK: [[MASK3:%.+]] = select i1 [[ISPOS3]], i64 [[POSMASK3]], i64 0
+// CHECK: [[PTRINT3:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR3:%.+]] = and i64 [[PTRINT3]], [[MASK3]]
+// CHECK: [[MASKCOND3:%.+]] = icmp eq i64 [[MASKEDPTR3]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND3]])
+}
+
+// Every type matches, canonical example.
+__INT32_TYPE__ test4(__SIZE_TYPE__ a) {
+// CHECK: define i32 @test4
+  return *m2(a);
+// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]]) 
+// CHECK: [[ISPOS4:%.+]] = icmp sgt i64 [[PARAM4]], 0
+// CHECK: [[POSMASK4:%.+]] = sub i64 [[PARAM4]], 1
+// CHECK: [[MASK4:%.+]] = select i1 [[ISPOS4]], i64 [[POSMASK4]], i64 0
+// CHECK: [[PTRINT4:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR4:%.+]] = and i64 [[PTRINT4]], [[MASK4]]
+// CHECK: [[MASKCOND4:%.+]] = icmp eq i64 [[MASKEDPTR4]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND4]])
+}
+
+
+struct Empty {};
+struct MultiArgs { __INT64_TYPE__ a, b;};
+// Struct parameter doesn't take up an IR parameter, 'i' takes up 2.
+// Truncation to i64 is permissible, since alignments of greater than 2^64 are insane.
+__INT32_TYPE__ *m3(struct Empty s, __int128_t i) __attribute__((alloc_align(2)));
+__INT32_TYPE__ test5(__int128_t a) {
+// CHECK: define i32 @test5
+  struct Empty e;
+  return *m3(e, a);
+// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: [[ALIGNCAST5:%.+]] = trunc i128 %{{.*}} to i64
+// CHECK: [[ISPOS5:%.+]] = icmp sgt i64 [[ALIGNCAST5]], 0
+// CHECK: [[POSMASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
+// CHECK: [[MASK5:%.+]] = select i1 [[ISPOS5]], i64 [[POSMASK5]], i64 0
+// CHECK: [[PTRINT5:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR5:%.+]] = and i64 [[PTRINT5]], [[MASK5]]
+// CHECK: [[MASKCOND5:%.+]] = icmp eq i64 [[MASKEDPTR5]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND5]])
+}
+// Struct parameter takes up 2 parameters, 'i' takes up 2.
+__INT32_TYPE__ *m4(struct MultiArgs s, __int128_t i) __attribute__((alloc_align(2)));
+__INT32_TYPE__ test6(__int128_t a) {
+// CHECK: define i32 @test6
+  struct MultiArgs e;
+  return *m4(e, a);
+// CHECK: call i32* @m4(i64 %{{.*}}, i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: [[ALIGNCAST6:%.+]] = trunc i128 %{{.*}} to i64
+// CHECK: [[ISPOS6:%.+]] = icmp sgt i64 [[ALIGNCAST6]], 0
+// CHECK: [[POSMASK6:%.+]] = sub i64 [[ALIGNCAST6]], 1
+// CHECK: [[MASK6:%.+]] = select i1 [[ISPOS6]], i64 [[POSMASK6]], i64 0
+// CHECK: [[PTRINT6:%.+]] = ptrtoint
+// CHECK: [[MASKEDPTR6:%.+]] = and i64 [[PTRINT6]], [[MASK6]]
+// CHECK: [[MASKCOND6:%.+]] = icmp eq i64 [[MASKEDPTR6]], 0
+// CHECK: call void @llvm.assume(i1 [[MASKCOND6]])
+}
+

cfe/trunk/test/Sema/alloc-align-attr.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// return values
+void test_void_alloc_align(void) __attribute__((alloc_align(1))); // expected-warning {{'alloc_align' attribute only applies to return values that are pointers}}
+void *test_ptr_alloc_align(int a) __attribute__((alloc_align(1))); // no-warning
+
+int j __attribute__((alloc_align(1))); // expected-warning {{'alloc_align' attribute only applies to non-K&R-style functions}}
+void *test_no_params_zero(void) __attribute__((alloc_align(0))); // expected-error {{'alloc_align' attribute parameter 1 is out of bounds}}
+void *test_no_params(void) __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute parameter 1 is out of bounds}}
+void *test_incorrect_param_type(float a) __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
+
+// argument type
+void *test_bad_param_type(void) __attribute((alloc_align(1.1))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
+
+// argument count
+void *test_no_fn_proto() __attribute__((alloc_align)); // expected-error {{'alloc_align' attribute takes one argument}}
+void *test_no_fn_proto() __attribute__((alloc_align())); // expected-error {{'alloc_align' attribute takes one argument}}
+void *test_no_fn_proto() __attribute__((alloc_align(32, 45, 37))); // expected-error {{'alloc_align' attribute takes one argument}}
+

cfe/trunk/test/SemaCXX/alloc-align-attr.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct param_num {
+  void* Foo(int a) __attribute__((alloc_align(1))); // expected-error {{'alloc_align' attribute is invalid for the implicit this argument}}
+};
+
+
+template <typename T>
+struct dependent_ret {
+  T* Foo(int a) __attribute__((alloc_align(2)));// no-warning, ends up being int**.
+  T Foo2(int a) __attribute__((alloc_align(2)));// expected-warning {{'alloc_align' attribute only applies to return values that are pointers or references}}
+};
+
+// Following 2 errors associated only with the 'float' versions below.
+template <typename T>
+struct dependent_param_struct {
+  void* Foo(T param) __attribute__((alloc_align(2))); // expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
+};
+
+template <typename T>
+void* dependent_param_func(T param) __attribute__((alloc_align(1)));// expected-error {{'alloc_align' attribute argument may only refer to a function parameter of integer type}}
+
+template <int T>
+void* illegal_align_param(int p) __attribute__((alloc_align(T))); // expected-error {{'alloc_align' attribute requires parameter 1 to be an integer constant}}
+
+void dependent_impl() {
+  dependent_ret<int> a; // expected-note {{in instantiation of template class 'dependent_ret<int>' requested here}}
+  a.Foo(1);
+  a.Foo2(1);
+  dependent_ret<int*> b; 
+  a.Foo(1);
+  a.Foo2(1);
+
+  dependent_param_struct<int> c; 
+  c.Foo(1);
+  dependent_param_struct<float> d; // expected-note {{in instantiation of template class 'dependent_param_struct<float>' requested here}}
+  d.Foo(1.0);
+  dependent_param_func<int>(1);
+  dependent_param_func<float>(1); // expected-note {{in instantiation of function template specialization 'dependent_param_func<float>' requested here}}
+}