Page MenuHomePhabricator

Clang Changes for alloc_align
ClosedPublic

Authored by erichkeane on Feb 6 2017, 11:33 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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)));
};

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.

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?

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 updated this revision to Diff 88602.Feb 15 2017, 2:09 PM

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)));
myatsina added inline comments.Mar 19 2017, 2:32 AM
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 updated this revision to Diff 92709.Mar 22 2017, 2:21 PM
erichkeane marked an inline comment as done.

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

erichkeane updated this revision to Diff 92814.Mar 23 2017, 9:06 AM

Update test based on the corresponding LLVM change.

FWIW, the LLVM changes for this have been committed.

aaron.ballman added inline comments.Mar 26 2017, 7:40 AM
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 marked 15 inline comments as done.Mar 27 2017, 10:52 AM

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.

erichkeane marked 2 inline comments as done.
aaron.ballman added inline comments.Mar 29 2017, 12:00 PM
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 marked 2 inline comments as done.Mar 29 2017, 1:27 PM
erichkeane added inline comments.
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 updated this revision to Diff 93404.Mar 29 2017, 1:29 PM

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 added inline comments.Mar 29 2017, 2:06 PM
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 marked 9 inline comments as done.Mar 29 2017, 2:48 PM
erichkeane added inline comments.
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!

erichkeane updated this revision to Diff 93409.Mar 29 2017, 2:49 PM

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 marked 2 inline comments as done.Mar 29 2017, 3:47 PM
erichkeane added inline comments.
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 updated this revision to Diff 93421.Mar 29 2017, 4:25 PM
erichkeane marked 2 inline comments as done.

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 added inline comments.Mar 29 2017, 6:11 PM
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 updated this revision to Diff 93491.Mar 30 2017, 9:21 AM
erichkeane marked 3 inline comments as done.

Thanks for the feedback John!

rjmccall added inline comments.Mar 30 2017, 12:51 PM
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 updated this revision to Diff 93537.Mar 30 2017, 2:10 PM

Added tests as requested by John.

rjmccall accepted this revision.Mar 30 2017, 2:31 PM

Thanks. With that, LGTM.

This revision is now accepted and ready to land.Mar 30 2017, 2:31 PM
aaron.ballman accepted this revision.Mar 30 2017, 2:50 PM

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.

This revision was automatically updated to reflect the committed changes.

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