[Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[steffenlarsen]

Summary

These changes make the Clang parser recognize expression parameter pack expansion and initializer lists in attribute arguments. Because expression parameter pack expansion requires additional handling while creating and instantiating templates, the support for them must be explicitly supported through the AcceptsExprPack flag.

Handling expression pack expansions may require a delay to when the arguments of an attribute are correctly populated. To this end, attributes that are set to accept these - through setting the AcceptsExprPack flag - will automatically have an additional variadic expression argument member named DelayedArgs. This member is not exposed the same way other arguments are but is set through the new CreateWithDelayedArgs creator function generated for applicable attributes.

To illustrate how to implement support for expression pack expansion support, clang::annotate is made to support pack expansions. This is done by making handleAnnotationAttr delay setting the actual attribute arguments until after template instantiation if it was unable to populate the arguments due to dependencies in the parsed expressions.

[erichkeane]

I think the safest bet is to be conservative and not allow mixing packs with variadics, and not allowing multiple packs. We should be able to diagnose that situation from ClangAttrEmitter.cpp to help attribute authors out. However, it'd be worth adding a FIXME comment to that diagnostic logic asking whether we want to relax the behavior at some point. But if you feel strongly that we should support these situations initially, I wouldn't be opposed.

From a practical perspective, the only difference between a list of ExprArgument and VariadicExprArgument is the enforced arity.... I would also consider seeing if we can just generalize the pack logic to work for BOTH cases. I'd be all for limiting the pack/ExprArgument list to be the 'last' argument to simplify it.

[steffenlarsen]

clang/include/clang/Basic/Attr.td

544–546

Having ParmExpansionArgsSupport in Attr would allow attributes to populate with parameter packs across argument boundaries. It seems more permissive, but frankly I have no attachment to it, so I am okay with tying it to Argument instead.

Of the mentioned options, I agree #2 and #3 are the preferred paths. For simplicity I prefer #2, though I will have to figure out how best to add a check on individual arguments, which @erichkeane's suggestion to limit the attributes to only allowing parameter packs in variadic arguments as the last argument would help with. On the other hand, the population logic for most of the relevant attributes are explicitly defined in clang/lib/Sema/SemaDeclAttr.cpp (which will have to be changed with #3) so being more permissive - like the current version of the patch is - gives more freedom to the attributes.

Does the tablegen design also seem likely to work when we want to introduce type packs instead of value packs?

I am not sure about how tablegen would take it, but the parser doesn't currently parse more than a single type argument, so I don't see a need for allowing type packs at the moment.

I think the safest bet is to be conservative and not allow mixing packs with variadics, and not allowing multiple packs. We should be able to diagnose that situation from ClangAttrEmitter.cpp to help attribute authors out. However, it'd be worth adding a FIXME comment to that diagnostic logic asking whether we want to relax the behavior at some point. But if you feel strongly that we should support these situations initially, I wouldn't be opposed.

If we limit packs to VariadicExprArgument - like annotate does in the current state of this patch - having multiple packs in the same argument is simple to handle. Both are represented as Expr and the variadic argument will simply contain two expressions until the templates are instantiated and the packs are expanded. This is also a feature I would like to keep, though it should be possible to work around the limitation if we conclude it can't stay.

clang/lib/Parse/ParseDecl.cpp

458

Do you have a test for something that isn't a pack followed by an ellipsis? What is the behavior there? I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?

Good question! I would expect it to fail in kind during template instantiation, or earlier if it is not an expression, but I will test it out once we converge on a new solution.

Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?). I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not particularly sure what is going to happen here?

I originally tried having the ActOnPackExpansion earlier, but it did indeed cause trouble. However, if my understanding of the previous two branches is correct, they are:

1. Type argument. This could be consuming the ellipsis as well, though I suspect it needs another path given that it does not seem to produce any expressions. Note however that it currently seems to stop argument parsing after reading the first type (confirmed by the FIXME) so parameter pack logic for this branch would probably be more suitable once it actually allows attributes to take multiple types.
2. Identifier argument. Correct me if I am wrong, but I don't think this can be populated by a parameter pack.

I could add the diagnostic to these if ellipsis is found, but if my assumptions are correct it doesn't make much sense to expand these branches beyond that.

clang/test/Parser/cxx0x-attributes.cpp

262

It makes more sense for the new test cases to get expressions rather than types and since Ts wasn't used for its contents it did not seem like an intrusive change.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

458

I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?

Sorry, I forgot to address this. I have not tested it, but I would expect complex-pack expansions like folds to be at expression-level. I will make sure to test this as well.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

544–546

Having ParmExpansionArgsSupport in Attr would allow attributes to populate with parameter packs across argument boundaries.

That's what I want to avoid. :-D I would prefer that the arguments have to opt into that behavior because the alternative could get ambiguous. I'd rather we do some extra work to explicitly support that situation once we have a specific attribute in mind for it.

... which @erichkeane's suggestion to limit the attributes to only allowing parameter packs in variadic arguments as the last argument would help with.

I agree with that suggestion, btw.

I am not sure about how tablegen would take it, but the parser doesn't currently parse more than a single type argument, so I don't see a need for allowing type packs at the moment.

Fair point, and yet another reason not to allow type packs initially. :-)

If we limit packs to VariadicExprArgument - like annotate does in the current state of this patch - having multiple packs in the same argument is simple to handle. Both are represented as Expr and the variadic argument will simply contain two expressions until the templates are instantiated and the packs are expanded.

Hmm, let's make sure we're on the same page. The situations I think we should avoid are:

// Mixing variadics and packs.
let Args = [VariadicExprArgument<"VarArgs">, VariadicExprArgument<"Pack", /*AllowPack*/1>];

// Multiple packs.
let Args = [VariadicExprArgument<"FirstPack", /*AllowPack*/1>, VariadicExprArgument<"SecondPack", /*AllowPack*/1>];

[erichkeane]

clang/include/clang/Basic/Attr.td

544–546

I agree, BOTH of those should be prohibited. Any amount of mixing of VariadicExprArgument (or mixing of any of the variadics IMO) need to be illegal. Otherwise the ambiguous nature of it is untenable.

While I like the possibility of
let Args = [ExprArgument<"Arg1">, ExprArgument<"Arg2">];

ALSO accepting a pack, I think we end up getting a little too 'hacky' with how to store this in the non-instantiated version.

[steffenlarsen]

Thank you both for the reviews! Consensus seems to be having support for pack expansion at argument level for now and let more complicated logic be added when there is an actual need. I have applied these changes as I understood them and have added/adjusted the requested test cases. Please have a look and let me know what you think. 😄

That's what I want to avoid. :-D I would prefer that the arguments have to opt into that behavior because the alternative could get ambiguous. I'd rather we do some extra work to explicitly support that situation once we have a specific attribute in mind for it.

I'm okay with that! I have made the changes to only allow it in the last argument if it is marked as supporting pack expansion. Note that it assumes that there are no other variadic parameters except the last one, as suggested.

Hmm, let's make sure we're on the same page. The situations I think we should avoid are:

// Mixing variadics and packs.
let Args = [VariadicExprArgument<"VarArgs">, VariadicExprArgument<"Pack", /*AllowPack*/1>];

// Multiple packs.
let Args = [VariadicExprArgument<"FirstPack", /*AllowPack*/1>, VariadicExprArgument<"SecondPack", /*AllowPack*/1>];

Oh, I see what you mean. Yeah I agree for sure. I think the only exceptions we currently have to this is omp attributes, but given that they are pragmas they should be nowhere close to this.

[erichkeane]

clang/include/clang/Basic/DiagnosticParseKinds.td

724

I don't really see why this is required? I would think the 722 would be all you would need.

clang/lib/Parse/ParseDecl.cpp

452

I don't think this should be diagnosed here, and I don't think it is 'right'. I think the ClangAttrEmitter should ensure that the VariadicExprArgument needs to be the 'last' thing, but I think that REALLY means "supports a pack anywhere inside of it".

See my test examples below, I don't think this parsing is sufficient for that.

clang/test/Parser/cxx0x-attributes.cpp

270

what about:
void foz [[clang::annotate("D", Is)]] ();

I would expect that to error.

Another test I'd like to see:

void foz[[clang::annotate("E", 1, 2, 3, Is...)]]

Also, I don't see why if THAT works, that:
void foz[[clang::annotate("E", 1, Is..., 2, 3)]]

shouldn't be allowed as well.

clang/utils/TableGen/ClangAttrEmitter.cpp

1166

The rule of 'only the last argument is allowed to support a pack' should be in the attribute emitter.

[steffenlarsen]

clang/include/clang/Basic/DiagnosticParseKinds.td

724

Intention was to make a distinction between the two cases, since we had the granularity to do so. I.e. clang::annotate would never use 722 as it explicitly states that the attribute doesn't support pack expansion (in the last argument) which is untrue for clang::annotate, but if a user was to do pack expansion on the first argument of clang::annotate they would get this error telling them that the given argument cannot accept the pack expansion.

clang/lib/Parse/ParseDecl.cpp

452

That is also the intention here. All it checks is that we are in or beyond the last argument of the attribute. For example, clang::annotate will return 2 from attributeNumberOfArguments as VariadicExprArgument only counts as a single argument. Thus, any pack expansion expressions parsed after the first will be accepted. I do agree though that the error message may be a little confusing for users.

I will add the suggested tests and rethink the diagnostics.

[steffenlarsen]

The new revision does the following:

1. Revisits the diagnostics. Now parameter packs will cause the same error on attributes that do not support parameter packs in arguments, while attributes that support it will cause an error if parameter packs are used in the incorrect argument, specifying which argument should be the earliest for it.
2. Adds additional tests to ensure that variadic arguments supporting packs also allow pack expansion intermingled with other arguments.
3. Adds assertions to attribute generation to check that attributes only ever support packs in the last variadic argument and that there are no other variadic arguments if they do.

[steffenlarsen]

clang/test/Parser/cxx0x-attributes.cpp

270

what about:
void foz [[clang::annotate("D", Is)]] ();

I would expect that to error.

So would I, but it seems that the parser and annotate is more than happy to consume this example, even in the current state of Clang: https://godbolt.org/z/rx64EKeeE
I am not sure as to why, but seeing as it is a preexisting bug I don't know if it makes sense to include it in the scope of this patch.

Another test I'd like to see:

void foz[[clang::annotate("E", 1, 2, 3, Is...)]]

Also, I don't see why if THAT works, that:
void foz[[clang::annotate("E", 1, Is..., 2, 3)]]

shouldn't be allowed as well.

They seem to work as expected. I have added these cases.

clang/utils/TableGen/ClangAttrEmitter.cpp

1166

The rule of 'only the last argument is allowed to support a pack' should be in the attribute emitter.

I have added some assertions around the area where attributes are generate, as that carries more surrounding information. Is that approximately what you had in mind?

[erichkeane]

clang/lib/Parse/ParseDecl.cpp

449

So I was thinking about this overnight... I wonder if this logic is inverted here? Aaron can better answer, but I wonder if we should be instead detecting when we are on the 'last' parameter and it is one of these VariadicExprArgument things (that accept a pack), then dropping the parser into a loop of:

while (Tok.is(tok::comma)){

Tok.Consume();
ArgExpr = CorrectTypos(ParseAssignmentExpr(...));

}
// finally, consume the closing paren.

WDYT?

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

449

The parsing of attribute arguments is already this lenient. It does not actually care how many arguments the attribute says it takes (unless it takes a type and then it will only currently supply _one_ argument). It simply consumes as many expressions it can before reaching the end parenthesis, then leaves the attributes to consume them in clang/lib/Sema/SemaDeclAttr.cpp.
As a result we do not need any additional work here to handle variadic arguments, but it also means that we have to introduce the restrictions that we can only allow packs in the last argument and allow only that argument to be variadic, as otherwise we have no way of knowing when and where the packs pass between argument boundaries.

[erichkeane]

clang/lib/Parse/ParseDecl.cpp

449

My point is, I don't think THIS function should be handling the ellipsis, the expression parsing functions we send the parsing of each component off to should do that.

We unfortunately cannot move the entirety of this parsing section to do that, since 'identifier'/'type', and 'Function' argument types end up needing special handling, but it would be nice if our 'expr' types would all be able to do this, and I suggested earlier that we could start with the VariadicExprArgument to make this an easier patch that didn't have to deal with the issues that come with that.

That said, it would be nice if we could get CLOSE to that.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

449

Sorry. I am still not sure I am following. Do you mean the consumption of pack expressions should be moved into the expression parsing so other places (not just attributes) would accept it? If so, I am not opposed to the idea though I have no idea which expressions these would be nor the full implications of such a change. For these we at least have the isolation of having to explicitly support the packs in the given attributes.

[erichkeane]

clang/lib/Parse/ParseDecl.cpp

449

I'm saying the other expression parsers should ALREADY properly handle packs, and we should just use those.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

449

I think I see what you mean. There is the Parser::ParseExpressionList which may fit the bill, though it seems to also accept initializer lists. But since it is an expression the attributes can decide if it is a valid expression for them, so maybe that is not a problem?

[erichkeane]

clang/lib/Parse/ParseDecl.cpp

449

Hmm... I don't have a good idea of that, Aaron has better visibility into that sort of thing.

Part of the advantage to doing it the way I suggest is that it ends up being a single call (that is, the parser should parse the commas!), so we should only have to consume the closing paren.

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

449

I think we want something more like Parser::ParseSimpleExpressionList() but... that doesn't handle packs, so those appear to be handled special: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059. But one of these two functions seems to be along the correct lines of what we want (with modification).

FWIW, I like the sound of Erich's approach better than what's in the current patch. I'd rather not try to specially handle the ellipsis so much as recognizing when we expect an arbitrary list of variadic expression arguments as the last "parameter" for an attribute.

[steffenlarsen]

I agree that reusing existing parser logic for this would be preferable, but as @aaron.ballman mentions Parser::ParseSimpleExpressionList does not give us the parameter pack expansion parsing we need. We could potentially unify it with the logic from https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059 but on the other hand there could be potential benefits to using Parser::ParseExpressionList instead.

The primary difference between the behavior of Parser::ParseExpressionList and what we are looking for is that it will allow initializer lists as arguments. Since most (if not all) attributes check the validity of their expression arguments, they would likely fail accordingly if they get an initializer list, likely specifying they expected an argument of type X. It may be a blessing in disguise to parse these arguments as well as we are more permissive w.r.t. attribute arguments. Please let me know if there is anything fundamentally wrong with this line of logic. One concern I have is that we currently run Actions.CorrectDelayedTyposInExpr on the expression right after parsing it, then we run pack expansion, whereas Parser::ParseExpressionList only runs the former action in failure case. I am unsure whether or not that might pose a problem.

One problem is that if we only use Parser::ParseExpressionList to parse the arguments of the last argument (if it is variadic) then we get the odd behavior that the initializer lists will only be permitted in variadic arguments, which arguably is a little obscure. However, upon further investigation the only case that stops us from using this for all expression arguments are when the attribute has either VariadicIdentifierArgument or VariadicParamOrParamIdxArgument as the first argument. If we assume that these variadic expressions are limited to the last argument as well (as they are variadic) leaving them as the only argument of the attributes we are parsing here, then we can parse expressions as we did before if we are parsing one of these arguments or switch to using e.g. Parser::ParseExpressionList for all expressions of the attribute (even non-variadic) if not. Then we would just have to iterate over the produced expressions to make sure no parameter pack expansions occur before the last (variadic) argument of the parsed attribute.

Summary of suggested changes:
Split attribute parsing into 3: First case parsing a type (currently exists). Second case keeping the current logic for parsing identifiers and single assignment-expressions if the attribute has a single argument of type VariadicIdentifierArgument or VariadicParamOrParamIdxArgument. Third case, applicable if none of the former ran, parses all expression parameters in one using Parser::ParseExpressionList and then checks that parameter packs only occur in the last variadic argument (if it allows it). Side effect is it allows initializer lists in the third case, but attributes are likely to complain about unexpected expression types if these are passed.

[steffenlarsen]

I have made the changes suggested in my previous comment. This makes significantly more changes to the parsing of attribute arguments as the old path was needed for attributes that allow both expressions and types. It also required some new controlling arguments for ParseExpressionList.

Because these changes also allow intializer lists in attribute arguments I have added a test case showing that clang::annotate parses these but will not accept them.

@erichkeane & @aaron.ballman - Is this in line with what you were thinking?

[aaron.ballman]

Thanks for this, I think it's shaping up well!

clang/include/clang/Basic/DiagnosticParseKinds.td

723

725

clang/lib/Parse/ParseDecl.cpp

426–428

I'm a bit surprised this logic moved -- are there no times when we'd need it within the new else clause?

451

Coding style nits.

clang/lib/Parse/ParseExpr.cpp

3360

or something along those lines (note, the logic has reversed meaning). "Fail immediately" doesn't quite convey the behavior to me because we fail immediately either way, it's more about whether we attempt to skip tokens to get to the end of the expression list. I'm not strongly tied to the name I suggested either, btw.

3374–3375

Not that I think this is a bad change, but it seems unrelated to the patch. Can you explain why you made the change?

clang/test/Parser/cxx0x-attributes.cpp

262

Non-type templates are different from type templates, so I don't really want to lose the test coverage for that (if nothing else, it ensures the parser doesn't explode when it encounters one). It's fine to add a variadic_nttp() function or something along those lines.

[steffenlarsen]

Thank you, @aaron.ballman ! I will update the revision in a moment with some of the suggested changes.

clang/lib/Parse/ParseDecl.cpp

426–428

This is done because ParseExpressionList wouldn't know what to do with identifiers, so it keeps the old logic. This means that attributes that consist of a variadic identifier argument cannot parse packs and initializer lists. It shouldn't be a problem for now, but it does kill some generality.

An alternative is to split the individual expression parsing from ParseExpressionList and use that in here in a similar way to how it parsed before. Would that be preferred?

clang/lib/Parse/ParseExpr.cpp

3360

"Fail immediately" doesn't quite convey the behavior to me because we fail immediately either way

Am I misunderstanding FailUntil or is the intention of SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch) to either stop at the next "," or ")"? If I am not misunderstanding, I believe it will continue parsing expressions after comma if SkipUntil stopped before it. As such I don't think FailImmediatelyOnInvalidExpr is inherently wrong, but I agree that skipping the skipping isn't very well conveyed by the name as is.

3374–3375

Admittedly I am not sure why it is needed, but in the previous version of the parsing for attribute parameters it does the typo-correction immediately after parsing the expression and if this isn't added a handful of tests fail.

[steffenlarsen]

Applied suggestions.

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

426–428

This is done because ParseExpressionList wouldn't know what to do with identifiers, so it keeps the old logic.

Ah, I think I confused myself into thinking {a, b, c} would be using a DeclRefExpr for each of the identifiers, but this code path is for identifiers that are *not* expressions. Got it.

An alternative is to split the individual expression parsing from ParseExpressionList and use that in here in a similar way to how it parsed before. Would that be preferred?

I think this is okay for now.

457

I think it'd be good to move this comment up closer to the else on line 461 so that it's clear what the else clause covers (and more clear that this is the common case).

clang/lib/Parse/ParseExpr.cpp

3360

You're correct about the behavior (I had missed that we stopped on a comma as well as closing paren, I was thinking we were stopping on the closing curly brace). So the parameter name isn't inherently wrong, just seems a bit unclear to me. I don't insist on a change, but if someone has a better name for the parameter, I won't be sad either.

3374–3375

Ah, thanks for the info! So this is basically doing the same work as what's done on line 3410 in the late failure case -- I wonder if this should actually be tied to the FailImmediatelyOnInvalidExpr parameter instead of having its own parameter?

[steffenlarsen]

Moved comment.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

457

I completely agree. It has been moved to right after the else.

clang/lib/Parse/ParseExpr.cpp

3374–3375

They could be unified in the current version, but I don't think there is an actual relation between them.

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

451

We don't typically use top-level const, so that should go as well.

clang/lib/Parse/ParseExpr.cpp

3374–3375

Do we fail tests if this isn't conditionalized at all (we always do the early typo correction)? (I'd like to avoid adding the new parameter because I'm not certain how a caller would determine whether they do or do not want that behavior. If we need the parameter, then so be it, but it seems like this is something generally useful.)

clang/test/Parser/cxx0x-attributes.cpp

262

You can back out the whitespace changes for an ever-so-slightly smaller patch. :-D

276–277

Shouldn't these cases also give the // expected-error {{pack expansion in 'annotate' may only be applied to the last argument}} diagnostic?

[steffenlarsen]

Removing top-level const and adjusting style.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

451

Completely missed that. My bad!

clang/lib/Parse/ParseExpr.cpp

3374–3375

11 tests fail if we don't make it conditional. The most telling one is probably SemaCXX/typo-correction-delayed.cpp which will have more errors if we do typo correction early, so it seems that the delay is intentional.

clang/test/Parser/cxx0x-attributes.cpp

262

Absolutely. Clang-format was very insistent, but since it isn't added by this patch I shouldn't worry. 😄

276–277

These should preferably pass. They are part of the "last" argument as annotate has one non-variadic argument and one variadic argument. I agree though that the error message is a little vague. Originally the error did specify that it had to be "argument %1 or later" to try and convey this intent, but maybe you have an idea for a clearer error message?

[steffenlarsen]

Upon offline sync with @aaron.ballman and @erichkeane I have changed the strategy of this patch to lift the restriction of pack expansion not spanning argument boundaries. This is implemented in clang::annotate by delaying arguments to after template-instantiation if any of the arguments are value dependent.

[erichkeane]

Took a look, it doesn't seem that this reflects the discussion we had yesterday.

clang/include/clang/AST/Attr.h

51

What is this supposed to be for? We should be able to tell if we weren't able to instantiate the expressions based on whether the expr-list has stuff in it, and if it is dependent.

clang/include/clang/Basic/Attr.td

208

I'm pretty against this 'fake', we should be putting this into the TableGen for every attribute that has AcceptsExprPack.

777

See above.

clang/lib/Parse/ParseDecl.cpp

460

So this all doesn't seem right either. The algorithm here is essentially:

if (AcceptsArgPack) {

ParseExpressionList(); // plus error handling

// Loop through args to see what matches correctly, if the non-VariadicExprList arguments are non-dependent, fill those in, and create as normal, else keep in the materialized collection.
}

clang/lib/Sema/SemaDeclAttr.cpp

4189

I think this behavior (adding an attribute with a dependent expression list) should be happening automatically.

In reality, whatever is doing the parsing or potentially-partial-instantiation should be checking the 'StringLiteral' value as a part of how it would without this being dependent.

That is: In the annotate case, the ONLY argument that we actually care about being dependent or not is the 'first' one. When we do the parsing/instantiation, we need to detect if the non-variadic parameters can be 'filled', then we can put the rest into the VariadicExpr list.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

196

So I don't think this is the right approach (teaching SemaDeclAttr about the delayed args). After substitution here, we should be using these to fill in the 'normal' arguments, then just passing these to the normal 'handleAnnotateAttr'.

[aaron.ballman]

I agree with the comments left by @erichkeane regarding some still needed tweaks to the approach.

clang/include/clang/Basic/Attr.td

208

Yeah, Fake is intended to be used for "internal details" and means that we won't pretty print out that data from the AST node. These arguments aren't really fake in that way -- they're things the user wrote in their source code.

clang/lib/Parse/ParseDecl.cpp

421–424

(Might need to re-flow comments to 80 col.) I don't think this is a FIXME so much as a "this just doesn't work like that" situation.

clang/lib/Parse/ParseExpr.cpp

3374–3375

Good to know, thank you for checking! I agree, it sounds like the delay is intentional then. I think it's okay to leave the parameter as you have it, but it is unfortunate that we've made the interface this much more complicated.

clang/test/Parser/cxx0x-attributes.cpp

276

We should also be adding test coverage for things like redeclarations, partial specializations, etc to make sure that the behavior there is correct. Those would be Sema tests rather than Parser tests, however.

clang/utils/TableGen/ClangAttrEmitter.cpp

2349–2357

Instead of making this an assert, I think we should use PrintFatalError() (this is how you effectively emit a diagnostic when compiling a .td file).

[steffenlarsen]

clang/include/clang/AST/Attr.h

51

You're right, it is redundant. I have removed it.

clang/include/clang/Basic/Attr.td

208

I see. I have changed it to automatically generate a variadic expression "argument" named DelayedArgs if an attribute has AcceptsExprPack set.

clang/lib/Parse/ParseDecl.cpp

460

I am not convinced this is the right place for that. These changes are simply to allow the additional parsing of packs and similar. It does not care much for the different arguments of the attribute, beyond whether or not it accepts types or identifiers as well (see the other cases) so having it make decisions about how the attributes should populate their arguments seems out of line with the current structure.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

421–424

I think it makes sense to have it be a FIXME because in theory it could be possible to have expression parameter packs expanded in an identifier list as it accepts expressions. I have reworded it slightly. Do you think this is better?

clang/lib/Sema/SemaDeclAttr.cpp

4189

I have changed it so it will only delay the arguments if the the first argument is dependent. Is this in line with what you were thinking? Since we still need to create the attribute I am not sure how we would do this automatically.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

196

With the new structure I have simplified this part a little. SemaDeclAttr still knows about the delayed args but solely to create the attribute, while the logic for creating the attribute with the "normal" arguments has been separated and made part of Sema so it is reachable from this function.

clang/test/Parser/cxx0x-attributes.cpp

276

Good point. I will add these tests ASAP.

clang/utils/TableGen/ClangAttrEmitter.cpp

2349–2357

I did not know! Thanks. It has been changed. 😄

[erichkeane]

I'm down to 1 little thing in how we handle the attribute itself, and I think @aaron.ballman understands the parsing section better than I do...

clang/lib/Parse/ParseDecl.cpp

386

So does this mean that our pack-parsing code is allowing identifiers? I really expected this patch to enforce via the clang-attr-emitter that ONLY the 'expr' types were allowed. So you'd have:

if (attributeAcceptsExprPack(AttrName)) {
... <where this ONLY accepts an expression pack>
} else if (Tok.is(tok::identifier)) {
...
}...

clang/lib/Sema/SemaDeclAttr.cpp

4200

Does this work with a partial specialization? I'd like to see some tests that ensure we work in that case (both where the partial specialization sets the string literal correctly, and where it doesnt).

4232

if AllArgs.size() == 0, this is an error case. Annotate requires at least 1 parameter.

4233

I would like @aaron.ballman to comment on this, but I think we probably want this case to be covered in the top of HandleDeclAttr, which would mean in the 'not all values filled' case, we skip the 'handleAnnotateAttr'.

WDYT Aaron? The downside is that this function couldn't check a 'partially filled in' attribute, but it would make us that much closer to this flag being a very simple thing to opt into.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

386

This will only consume the identifier if the argument is an identifier argument (variadic or non-variadic). As you say, clang-attr-emitter will make sure that no attribute with AcceptsExprPack has any identifier arguments, so if there is an identifier here it will not be able to consume it and then it will continue on to try and parse it as an expression and fail.

That said, the second part of the following parsing branches should probably also check attributeHasIdentifierArg(*AttrName), so I will add that shortly.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

386

That said, the second part of the following parsing branches should probably also check attributeHasIdentifierArg(*AttrName), so I will add that shortly.

Correction: Parsing any identifiers after this assumes there is a variadic identifier argument. Effectively it assumes that all non-variadic identifiers in an attribute are at the start of an attribute arguments, in which case they'll be parsed here. This is a little lackluster as it could simply allow identifiers in other places if there are variadic identifier arguments in the attribute, but I don't think fixing it should be in the scope of this patch.

[steffenlarsen]

Removed unnecessary test for arguments.

[steffenlarsen]

clang/lib/Sema/SemaDeclAttr.cpp

4232

I removed the check but thinking about it again I think it would be better to reintroduce it and let HandleAnnotateAttr call checkAtLeastNumArgs. That way it will also complain about missing arguments after template instantiation, e.g. if a lone pack expansion argument evaluates to an empty expression list.

[steffenlarsen]

Adds check and diagnostic for no arguments in annotate attribute.

[steffenlarsen]

clang/lib/Sema/SemaDeclAttr.cpp

4232

I have reintroduced the check for AllArgs.size() here. This means if AllArgs is empty it will continue to Sema::HandleAnnotateAttr which will check that there are at least one element in AllArgs and fail otherwise with a useful diagnostic. This diagnostic will also be issued if an empty parameter pack is given and that is the only argument of the annotation. Note that checkAtLeastNumArgs is a member of ParsedAttr which isn't available in Sema::HandleAnnotateAttr, so it replicates the check. It could potentially be moved, but given the size I don't think it's necessary in this patch.

I also added some tests for checking that the diagnostic is issued in the cases (pack expansion and explicitly empty).

@erichkeane - What do you think?

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

4232

Hmm... I would expect this to diagnose even in the 'delayed args' case as well. Otherwise:

template<typename T>
[[annotate()]]

is not an error, right?

[steffenlarsen]

clang/lib/Sema/SemaDeclAttr.cpp

4232

Both "delayed args" and immediate instantiation go through the diagnostics check. The check is intentionally in Sema::HandleAnnotateAttr as either this or template instantiation (if delayed args) goes through it.

template<typename T> [[annotate()]] ...

would fail because it does not have any arguments which will be identified here as "delayed args" will be skipped.

template<int... Is> [[annotate(Is...)]] ...

will fail once instantiated with no template arguments as the instantiating function will pass an empty list of expressions to Sema::HandleAnnotationAttr which will check and fail with the diagnostic. The latter is tested and I can add the former as a case while adding the other requested tests.

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

4232

Can you make sure both of those have a test? I missed that the 'no args' case would go right to HandleAnnotateAttr.

[steffenlarsen]

Added tests for redeclarations and template specialization using clang::annoate with packs.

[steffenlarsen]

clang/lib/Sema/SemaDeclAttr.cpp

4200

I have added such tests in clang/test/SemaTemplate/attributes.cpp. Let me know if they cover the scenario you were thinking about.

4232

For sure. It has been added.

clang/test/Parser/cxx0x-attributes.cpp

276

I have added such tests in clang/test/SemaTemplate/attributes.cpp. Are these along the line of what you were thinking?

[erichkeane]

I think I'm happy, though there are the opens for Aaron (deciding whether the 'create as delayed' should happen without entry into 'handleAnnotateAttr', and whether the 'parsing' looks correct, despite not being the way we discussed), so you'll need to wait on him.

[aaron.ballman]

I think this is heading in the right direction! I've got a few more comments and questions though.

clang/include/clang/Basic/Attr.td

545

You should probably add a release note about this new feature.

791

You should definitely add a release note about this new behavior for annotate.

792

Le sigh. We should update the documentation for this new feature, but we have no documentation for the feature *to* update. (This is not your problem to solve in this patch.)

clang/lib/Parse/ParseDecl.cpp

421–424

Maybe we're thinking about identifier lists differently. We only have two attributes that use those (cpu_specific and cpu_dispatch) and in both cases (and all cases I would expect), what's being received is effectively a list of enumerators (not enumerators in the C or C++ sense) that could not be mixed with expressions. Expressions would go through sema and do all the usual lookup work to turn them into a value, but these are not real objects and so the lookup would fail for them. e.g., we're not going to be able to support something like: [[clang::cpu_specific(generic, pentium, Ts..., atom)]]. So I don't think there's anything here to be fixed (I prefer my comment formulation as that makes it more clear).

clang/lib/Sema/SemaDeclAttr.cpp

4200–4203

Please double-check that the call to checkCommonAttributeFeatures() from ProcessDeclAttribute() doesn't already handle this for you. If it does, then I think this can be replaced with assert(!AllArgs.empty() && "expected at least one argument");

4233

Do you mean ProcessDeclAttribute()? I don't think we should have attribute-specific logic in there, but are you thinking of something more general than that (I'm not seeing how the suggestion makes the flag easier to opt into)?

clang/test/SemaTemplate/attributes.cpp

230–239

I was thrown by the CHECK and CHECK-NEXT mixtures here because I couldn't see that the inherited attributes were or were not also getting the expanded pack values. You should make sure to include all of the lines with CHECK-NEXT so we see a full picture of the AST dump (this file is not super consistent about it).

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

4233

Ah, yes, thats what I mean. The question for ME is whether there should be a generic "this supports variadic pack, so check to see if all the named non-expr arguments are fill-in-able. If not, do the 'delayed' version.

This would mean that HandleAnnotateAttr NEVER sees the "CreateWithDelayedArgs" case.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

4233

Thanks for clarifying -- yes, I think that would be preferable if it works out in a clean, generic way. I'd be fine with tablegen emitting something else (if necessary) to help generalize it.

[steffenlarsen]

Adjusted for comments and introduced common handling for creating attributes with delayed arguments.

[steffenlarsen]

clang/lib/Parse/ParseDecl.cpp

421–424

I see what you mean. I have applied your wording instead.

clang/lib/Sema/SemaDeclAttr.cpp

4200–4203

It does go through checkCommonAttributeFeatures but (as of the recent changes) it will skip size-checks if arguments are delayed as a pack expansion could potentially populate the seemingly missing expressions after template instantiation for some attribute.
For annotate we could also have a pack as the only expression, which would then evaluate to an empty list of expressions. Since this path is also taken by instantiateDependentAnnotationAttr if there are delayed args. In reality it is only really needed after template instantiations, given as you said checkCommonAttributeFeatures will do the checking in the other case, but I personally think it is cleaner to have it here. If you disagree I will move it into instantiateDependentAnnotationAttr instead and add the assert.

4233

handleAnnotateAttr is now oblivious to the concept of "delayed arguments". Instead tablegen generates a common handle function (handleAttrWithDelayedArgs) which will, based on the given ParsedAttr that supports delayed arguments, create and add the corresponding attribute with delayed arguments by calling the corresponding CreateWithDelayedArgs. The need for delaying arguments is decided as described in MustDelayAttributeArguments.

Is this approximately what you were thinking?

clang/test/SemaTemplate/attributes.cpp

230–239

I have done this. It is a little more verbose, but I agree with the full-picture sentiment.

[erichkeane]

1 question, otherwise happy.

clang/lib/Sema/SemaDeclAttr.cpp

8170

This still doesn't work if the VariadicExprArgument isn't last, right? Do we ensure that is the case in clang-attr-emitter?

[steffenlarsen]

clang/lib/Sema/SemaDeclAttr.cpp

8170

Good point. I don't think there's a check as there are select few attributes that use multiple variadic (OMPDeclareSimdDecl and OMPDeclareVariant are the only ones, I think.)

Since I don't think it's safe to check for all, should I make a check similar to the one for type/identifier arguments in attributes marked AcceptsExprPack? Would that suffice?

[erichkeane]

clang/lib/Sema/SemaDeclAttr.cpp

8170

I'm fine rejecting a case that has anything besides expression-arguments(and ones create-able from expression arguments) and limited-to-only-1-must-be-last variadic-expr-list in ClangAttrEmitter.

I believe we discussed that at one point, but I didn't see it here.

[steffenlarsen]

Added check restricting variadic expression arguments to the last argument of attributes set to support parameter packs.
Added release notes.

[steffenlarsen]

clang/include/clang/Basic/Attr.td

545

I have added a release note about this under "Internal API Changes". Let me know if you think the wording needs changes.

791

I have added a release note about this under "Attribute Changes in Clang". As with the other note, let me know if wording needs changes.

clang/lib/Sema/SemaDeclAttr.cpp

8170

I have added a check to ClangAttrEmitter next to the check ensuring that the relevant attributes don't have identifier or type arguments.

[steffenlarsen]

Added missing isVariadicExprArgument function.

[steffenlarsen]

Added a check to prevent duplicate "empty" constructors, i.e. if there are only optional arguments such as a single variadic argument.

[aaron.ballman]

clang/include/clang/Sema/ParsedAttr.h

52–57 ↗

(On Diff #406373)

Er, this means the bit packed part of the structure can no longer fit in a 32-bit allocation unit (we went from needing 30 bits to needing 35 bits). That's unfortunate. I don't see any other bits to reasonably steal from.

I took a look to see whether I thought this would have a significant performance impact. We definitely keep arrays of these things around and walk over the array, so I expect some performance loss from this change. But it seems like the sort of thing likely to fall out in the noise rather than be on the hot path, so I think this is fine. However, if we see a noticeable performance regression, we should keep this change in mind to see if there's something more we can do to keep us within a 32-bit allocation unit.

113 ↗

(On Diff #406373)

I don't think this API is needed. isArgExpr() already exists on the interface and I'm not certain how this differs. (If this API is needed, we need a far better name for it because I'd have no idea how to distinguish isExprArg() and isArgExpr().)

clang/include/clang/Sema/Sema.h

4379–4381

This name makes it sound like far more general of an API than it is. This function is specific to checking string literal arguments for an attribute. But then why does checkStringLiteralArgumentAttr() not suffice? It does the same thing, but with more bells and whistles.

clang/lib/Sema/SemaDeclAttr.cpp

4199–4211

This is a slight loss of functionality. We used to recommend identifiers be converted to string literals under the old interface. I don't think this refactoring is necessary any longer, right?

4200–4203

Ah, I think what caught me off-guard is that I was expecting checkCommonAttributeFeatures() to be called again from template instantiation time. Then there are less (possibly even no more) dependent arguments, so it can do more automated checking. However, I see that Sema::InstantiateAttrs() is crying out for refactoring to make that more useful, so it's outside the scope of this patch (what you're doing here is fine). Thanks!

4233

This is more along the lines of what I had in mind, yes.

clang/utils/TableGen/ClangAttrEmitter.cpp

2282

The comment doesn't seem grammatical and may need some expounding.

[steffenlarsen]

Recent changes:

• Replaces isExprArg with isArgMemberExprHolder which returns true if the "argument member" can hold one or more expressions.
• Renames checkStringLiteralExpr to checkASCIIStringLiteralExpr to better convey the intended semantics.
• Reverts changes to HandleAnnotateAttr and removes HandleAnnotationAttr, moving final-args checks of expressions into instantiateDependentAnnotationAttr.
• Removes nonsensical comment.

[steffenlarsen]

clang/include/clang/Sema/ParsedAttr.h

113 ↗

(On Diff #406373)

There is an important distinction between the two, specifically that isArgExpr checks if the parsed argument at the index is an expression, while isExprArg checks if the specified argument in the attribute is an "ExprArgument", i.e. one checks post-parsing the other reflects on the attribute's definition.

That said, I agree that with that naming there's little distinction between the two. I have therefore removed isExprArg and replaced it with isArgMemberExprHolder which is true if the "argument member" (referring to the argument defined in Attr.td) is able to hold an expression (i.e. it is either a ExprArgument or a VaridicExprArgument.) Functionally there is little change to it, as the only place we use the check is after checking if the expression is variadic, but it fit better with the actual intent of the check.

clang/include/clang/Sema/Sema.h

4379–4381

The problem is that checkStringLiteralArgumentAttr() requires the arguments to be extracted from the parsed attribute, which we do not have after template instantiation, so the split was for generality. However, as you mentioned in another comment, the generality is not that needed anymore so handleAnnotateAttr has been reverted to the old version.

That said, this is still needed for doing post-initalization check if arguments have been delayed, so I have renamed it to checkASCIIStringLiteralExpr which hopefully conveys the intent a little better?

clang/lib/Sema/SemaDeclAttr.cpp

4199–4211

You're absolutely right. There is actually no reason for this refactoring anymore, except for maybe some minor common code between this and delayed argument instantiation, but it is so little now that if we want to keep the hint (which I agree we should) then there's little reason not to revert the changes to handleAnnotateAttr. So that's what I've done! Similarly, handleAnnotateAttr is no longer needed so it has been removed and the excess logic has been moved to instantiateDependentAnnotationAttr. Thanks for pointing this out!

clang/utils/TableGen/ClangAttrEmitter.cpp

2282

I don't think a comment makes much sense here anyway, so I've removed it. This seems like fairly common practice for these kind of functions. If you disagree I can reintroduce it and expand on it.

[aaron.ballman]

I still have some naming issues that need to be corrected, but functionally I think this is good to go. I presume you don't have commit privileges @steffenlarsen? If that's accurate, what name and email address would you like me to use for patch attribution after you've fixed the last few bits?

clang/include/clang/Sema/ParsedAttr.h

113 ↗

(On Diff #406373)

Thanks for the clarification!

I think a better way to express this is by naming it isParamExpr() and adding comments along the lines of:

/// Returns true if the specified parameter index for this attribute in Attr.td is an ExprArgument
/// or VariadicExprArgument, or a subclass thereof; returns false otherwise.

(Note, the local style here is to use /// instead of //, so I'm matching that as well. You probably need to flow the comments to 80 cols as well.)

clang/include/clang/Sema/Sema.h

4379–4381

The new name is still a problem -- it sounds like a generic interface, but the diagnostic it emits is specific to attributes. That's going to be a maintenance issue.

I would make this an overload of checkStringLiteralArgumentAttr() instead of giving it a new name.

[steffenlarsen]

Recent changes:

• Renamed isArgMemberExprHolder to isParamExpr and added the comment suggested by @aaron.ballman .
• Made checkASCIIStringLiteralExpr an overload of checkStringLiteralArgumentAttr.

[steffenlarsen]

Thanks, @aaron.ballman ! I have applied the new suggestions.

I presume you don't have commit privileges @steffenlarsen? If that's accurate, what name and email address would you like me to use for patch attribution after you've fixed the last few bits?

That is correct. Name is "Steffen Larsen" and email is "[e-mail]". Thanks a ton. 😄

clang/include/clang/Sema/ParsedAttr.h

113 ↗

(On Diff #406373)

isParamExpr is fine by me! I have also added the suggested comment. Thanks! (Also, good eye on the // vs ///)

clang/include/clang/Sema/Sema.h

4379–4381

Overloading checkStringLiteralArgumentAttr is a good shout. I have done that.

[aaron.ballman]

LGTM! Thank you for all the good discussion and work on this! I'll commit on your behalf once the precommit CI pipeline comes back green.

[aaron.ballman]

I've commit on your behalf in ead1690d31f815c00fdd2bc23db4766191bbeabc, thank you!

clang/include/clang/AST/Attr.h

private:
   unsigned AttrKind : 16;
 
 protected:
   /// An index into the spelling list of an
   /// attribute defined in Attr.td file.
   unsigned Inherited : 1;
   unsigned IsPackExpansion : 1;
   unsigned Implicit : 1;
+  unsigned ArgsDelayed : 1;
   // FIXME: These are properties of the attribute kind, not state for this
   // instance of the attribute.
   unsigned IsLateParsed : 1;
   unsigned InheritEvenIfAlreadyPresent : 1;
 
   void *operator new(size_t bytes) noexcept {
     llvm_unreachable("Attrs cannot be allocated with regular 'new'.");
   }
public:
   void operator delete(void *Ptr, ASTContext &C, size_t Alignment) noexcept {
     return ::operator delete(Ptr, C, Alignment);
   }
 
 protected:
   Attr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
        attr::Kind AK, bool IsLateParsed)
       : AttributeCommonInfo(CommonInfo), AttrKind(AK), Inherited(false),
-        IsPackExpansion(false), Implicit(false), IsLateParsed(IsLateParsed),
-        InheritEvenIfAlreadyPresent(false) {}
+        IsPackExpansion(false), Implicit(false), ArgsDelayed(false),
+        IsLateParsed(IsLateParsed), InheritEvenIfAlreadyPresent(false) {}
 
 public:
   attr::Kind getKind() const { return static_cast<attr::Kind>(AttrKind); }
 
   unsigned getSpellingListIndex() const {
     return getAttributeSpellingListIndex();
   }
   const char *getSpelling() const;
 
   SourceLocation getLocation() const { return getRange().getBegin(); }
 
   bool isInherited() const { return Inherited; }
 
   /// Returns true if the attribute has been implicitly created instead
   /// of explicitly written by the user.
   bool isImplicit() const { return Implicit; }
   void setImplicit(bool I) { Implicit = I; }
 
   void setPackExpansion(bool PE) { IsPackExpansion = PE; }
   bool isPackExpansion() const { return IsPackExpansion; }
 
+  void setArgsDelayed(bool PE) { ArgsDelayed = PE; }
+  bool areArgsDelayed() const { return ArgsDelayed; }
+
   // Clone this attribute.
   Attr *clone(ASTContext &C) const;
 
   bool isLateParsed() const { return IsLateParsed; }
 
   // Pretty print this attribute.
   void printPretty(raw_ostream &OS, const PrintingPolicy &Policy) const;
 

clang/include/clang/Basic/Attr.td

 //   OMPTraitSelector := {Kind, Expr, list<OMPTraitProperty>}
 //   OMPTraitProperty := {Kind}
 //
 class OMPTraitInfoArgument<string name> : Argument<name, 0>;
 
 class TypeArgument<string name, bit opt = 0> : Argument<name, opt>;
 class UnsignedArgument<string name, bit opt = 0> : Argument<name, opt>;
 class VariadicUnsignedArgument<string name> : Argument<name, 1>;
-class VariadicExprArgument<string name> : Argument<name, 1>;
+class VariadicExprArgument<string name, bit fake = 0> : Argument<name, 1, fake>;
 class VariadicStringArgument<string name> : Argument<name, 1>;
 class VariadicIdentifierArgument<string name> : Argument<name, 1>;
 
 // Like VariadicUnsignedArgument except values are ParamIdx.
 class VariadicParamIdxArgument<string name> : Argument<name, 1>;
 
 // A list of identifiers matching parameters or ParamIdx indices.
 class VariadicParamOrParamIdxArgument<string name> : Argument<name, 1>;
class Attr {
   bit MeaningfulToClassTemplateDefinition = 0;
   // Set to true if this attribute can be used with '#pragma clang attribute'.
   // By default, an attribute is supported by the '#pragma clang attribute'
   // only when:
   // - It has a subject list whose subjects can be represented using subject
   //   match rules.
   // - It has GNU/CXX11 spelling and doesn't require delayed parsing.
   bit PragmaAttributeSupport;
+  // Set to true if this attribute accepts parameter pack expansion expressions.
+  bit AcceptsExprPack = 0;
   // Lists language options, one of which is required to be true for the
   // attribute to be applicable. If empty, no language options are required.
   list<LangOpt> LangOpts = [];
   // Any additional text that should be included verbatim in the class.
   // Note: Any additional data members will leak and should be constructed
   // externally on the ASTContext.
   code AdditionalMembers = [{}];
   // Any documentation that should be associated with the attribute. Since an
   // attribute may be documented under multiple categories, more than one
def AnalyzerNoReturn : InheritableAttr {
   // analyzer?
   let Spellings = [GNU<"analyzer_noreturn">];
   // TODO: Add subject list.
   let Documentation = [Undocumented];
 }
 
 def Annotate : InheritableParamAttr {
   let Spellings = [Clang<"annotate">];
-  let Args = [StringArgument<"Annotation">, VariadicExprArgument<"Args">];
+  let Args = [StringArgument<"Annotation">,
+              VariadicExprArgument<"Args">,
+              VariadicExprArgument<"DelayedArgs", /*fake=*/1>];
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
   let AdditionalMembers = [{
   static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, \
               const AttributeCommonInfo &CommonInfo) {
     return AnnotateAttr::Create(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   static AnnotateAttr *CreateImplicit(ASTContext &Ctx, llvm::StringRef Annotation, \
               const AttributeCommonInfo &CommonInfo = {SourceRange{}}) {
     return AnnotateAttr::CreateImplicit(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   }];
   let PragmaAttributeSupport = 1;
+  let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
 }
 
 def ARMInterrupt : InheritableAttr, TargetSpecificAttr<TargetARM> {
   // NOTE: If you add any additional spellings, M68kInterrupt's,
   // MSP430Interrupt's, MipsInterrupt's and AnyX86Interrupt's spellings
   // must match.
   let Spellings = [GCC<"interrupt">];
   let Args = [EnumArgument<"Interrupt", "InterruptType",

clang/include/clang/Basic/DiagnosticParseKinds.td

 def err_attributes_not_allowed : Error<"an attribute list cannot appear here">;
 def ext_cxx11_attr_placement : ExtWarn<
   "ISO C++ does not allow an attribute list to appear here">,
   InGroup<DiagGroup<"cxx-attribute-extension">>;
 def err_attributes_misplaced : Error<"misplaced attributes; expected attributes here">;
 def err_l_square_l_square_not_attribute : Error<
   "C++11 only allows consecutive left square brackets when "
   "introducing an attribute">;
+def err_attribute_argument_parm_pack_not_supported : Error<
+  "attribute %0 does not support argument pack expansion">;
 def err_ms_declspec_type : Error<
   "__declspec attributes must be an identifier or string literal">;
 def err_ms_property_no_getter_or_putter : Error<
   "property does not specify a getter or a putter">;
 def err_ms_property_unknown_accessor : Error<
   "expected 'get' or 'put' in property declaration">;
 def err_ms_property_has_set_accessor : Error<
   "putter for property must be specified as 'put', not 'set'">;
 def err_ms_property_missing_accessor_kind : Error<
   "missing 'get=' or 'put='">;

clang/include/clang/Parse/Parser.h

ExprResult ParseExprAfterUnaryExprOrTypeTrait(const Token &OpTok,
                                                      SourceRange &CastRange);
 
   typedef SmallVector<SourceLocation, 20> CommaLocsTy;
 
   /// ParseExpressionList - Used for C/C++ (argument-)expression-list.
   bool ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
                            SmallVectorImpl<SourceLocation> &CommaLocs,
                            llvm::function_ref<void()> ExpressionStarts =
-                               llvm::function_ref<void()>());
+                               llvm::function_ref<void()>(),
+                           bool FailImmediatelyOnInvalidExpr = false,
+                           bool EarlyTypoCorrection = false);
 
   /// ParseSimpleExpressionList - A simple comma-separated list of expressions,
   /// used for misc language extensions.
   bool ParseSimpleExpressionList(SmallVectorImpl<Expr*> &Exprs,
                                  SmallVectorImpl<SourceLocation> &CommaLocs);
 
 
   /// ParenParseOption - Control what ParseParenExpression will parse.

clang/include/clang/Sema/Sema.h

public:
   /// type as valid.
   bool isValidPointerAttrType(QualType T, bool RefOkay = false);
 
   bool CheckRegparmAttr(const ParsedAttr &attr, unsigned &value);
   bool CheckCallingConvAttr(const ParsedAttr &attr, CallingConv &CC,
                             const FunctionDecl *FD = nullptr);
   bool CheckAttrTarget(const ParsedAttr &CurrAttr);
   bool CheckAttrNoArgs(const ParsedAttr &CurrAttr);
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+                              StringRef &Str,
+                              SourceLocation *ArgLocation = nullptr);
   bool checkStringLiteralArgumentAttr(const ParsedAttr &Attr, unsigned ArgNum,
                                       StringRef &Str,
                                       SourceLocation *ArgLocation = nullptr);
   llvm::Error isValidSectionSpecifier(StringRef Str);
   bool checkSectionName(SourceLocation LiteralLoc, StringRef Str);
   bool checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);
   bool checkTargetClonesAttrString(SourceLocation LiteralLoc, StringRef Str,
                                    const StringLiteral *Literal,

   /// declaration.
   void AddAllocAlignAttr(Decl *D, const AttributeCommonInfo &CI,
                          Expr *ParamExpr);
 
   /// AddAlignValueAttr - Adds an align_value attribute to a particular
   /// declaration.
   void AddAlignValueAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E);
 
+  /// CheckAnnotationAttrArgExprs - Checks and returns the argument expressions
+  /// of an annotation attribute.
+  bool CheckAnnotationAttrArgExprs(const AttributeCommonInfo &CI,
+                                   MutableArrayRef<Expr *> ArgExprs,
+                                   StringRef &Annot,
+                                   SmallVector<Expr *, 4> &Args);
   /// AddAnnotationAttr - Adds an annotation Annot with Args arguments to D.
   void AddAnnotationAttr(Decl *D, const AttributeCommonInfo &CI,
                          StringRef Annot, MutableArrayRef<Expr *> Args);
+  /// AddAnnotationAttrWithDelayedArgs - Adds an annotation with delayed
+  /// arguments to D.
+  void AddAnnotationAttrWithDelayedArgs(Decl *D, const AttributeCommonInfo &CI,
+                                        MutableArrayRef<Expr *> DelayedArgs);
 
   /// AddLaunchBoundsAttr - Adds a launch_bounds attribute to a particular
   /// declaration.
   void AddLaunchBoundsAttr(Decl *D, const AttributeCommonInfo &CI,
                            Expr *MaxThreads, Expr *MinBlocks);
 
   /// AddModeAttr - Adds a mode attribute to a particular declaration.
   void AddModeAttr(Decl *D, const AttributeCommonInfo &CI, IdentifierInfo *Name,

clang/lib/Parse/ParseDecl.cpp

 static bool attributeTreatsKeywordThisAsIdentifier(const IdentifierInfo &II) {
 #define CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST
   return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
 #include "clang/Parse/AttrParserStringSwitches.inc"
            .Default(false);
 #undef CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST
 }
 
+/// Determine if an attribute accepts parameter packs.
+static bool attributeAcceptsExprPack(const IdentifierInfo &II) {
+#define CLANG_ATTR_ACCEPTS_EXPR_PACK
+  return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+#include "clang/Parse/AttrParserStringSwitches.inc"
+      .Default(false);
+#undef CLANG_ATTR_ACCEPTS_EXPR_PACK
+}
+
 /// Determine whether the given attribute parses a type argument.
 static bool attributeIsTypeArgAttr(const IdentifierInfo &II) {
 #define CLANG_ATTR_TYPE_ARG_LIST
   return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
 #include "clang/Parse/AttrParserStringSwitches.inc"
            .Default(false);
 #undef CLANG_ATTR_TYPE_ARG_LIST
 }
unsigned Parser::ParseAttributeArgsCommon(
     IdentifierInfo *AttrName, SourceLocation AttrNameLoc,
     ParsedAttributes &Attrs, SourceLocation *EndLoc, IdentifierInfo *ScopeName,
     SourceLocation ScopeLoc, ParsedAttr::Syntax Syntax) {
   // Ignore the left paren location for now.
   ConsumeParen();
 
   bool ChangeKWThisToIdent = attributeTreatsKeywordThisAsIdentifier(*AttrName);
   bool AttributeIsTypeArgAttr = attributeIsTypeArgAttr(*AttrName);
+  bool AttributeHasVariadicIdentifierArg =
+      attributeHasVariadicIdentifierArg(*AttrName);
 
   // Interpret "kw_this" as an identifier if the attributed requests it.
   if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
     Tok.setKind(tok::identifier);
 
   ArgsVector ArgExprs;
   if (Tok.is(tok::identifier)) {
     // If this attribute wants an 'identifier' argument, make it so.
-    bool IsIdentifierArg = attributeHasIdentifierArg(*AttrName) ||
-                           attributeHasVariadicIdentifierArg(*AttrName);
+    bool IsIdentifierArg = AttributeHasVariadicIdentifierArg ||
+                           attributeHasIdentifierArg(*AttrName);
     ParsedAttr::Kind AttrKind =
         ParsedAttr::getParsedKind(AttrName, ScopeName, Syntax);
 
     // If we don't know how to parse this attribute, but this is the only
     // token in this argument, assume it's meant to be an identifier.
     if (AttrKind == ParsedAttr::UnknownAttribute ||
         AttrKind == ParsedAttr::IgnoredAttribute) {
       const Token &Next = NextToken();
       IsIdentifierArg = Next.isOneOf(tok::r_paren, tok::comma);
     }
 
     if (IsIdentifierArg)
       ArgExprs.push_back(ParseIdentifierLoc());
   }
 
   ParsedType TheParsedType;
   if (!ArgExprs.empty() ? Tok.is(tok::comma) : Tok.isNot(tok::r_paren)) {
     // Eat the comma.
     if (!ArgExprs.empty())
       ConsumeToken();
 
-    // Parse the non-empty comma-separated list of expressions.
-    do {
-      // Interpret "kw_this" as an identifier if the attributed requests it.
-      if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
-        Tok.setKind(tok::identifier);
-
-      ExprResult ArgExpr;
-      if (AttributeIsTypeArgAttr) {
-        TypeResult T = ParseTypeName();
-        if (T.isInvalid()) {
-          SkipUntil(tok::r_paren, StopAtSemi);
-          return 0;
-        }
-        if (T.isUsable())
-          TheParsedType = T.get();
-        break; // FIXME: Multiple type arguments are not implemented.
-      } else if (Tok.is(tok::identifier) &&
-                 attributeHasVariadicIdentifierArg(*AttrName)) {
+      // FIXME: Multiple type arguments are not implemented.
+    } else if (AttributeHasVariadicIdentifierArg) {
+      // Parse variadic identifier arg. This can either consume identifiers or
+      // expressions.
+      // FIXME: Variadic identifier args do not currently support parameter
+      //        packs.
+      do {
+        // Interpret "kw_this" as an identifier if the attributed requests it.
+        if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+          Tok.setKind(tok::identifier);
+
+        ExprResult ArgExpr;
+        if (Tok.is(tok::identifier)) {
           ArgExprs.push_back(ParseIdentifierLoc());
         } else {
           bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
           EnterExpressionEvaluationContext Unevaluated(
               Actions,
               Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
                      : Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
           ExprResult ArgExpr(
               Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
           if (ArgExpr.isInvalid()) {
             SkipUntil(tok::r_paren, StopAtSemi);
             return 0;
           }
           ArgExprs.push_back(ArgExpr.get());
         }
         // Eat the comma, move to the next argument
       } while (TryConsumeToken(tok::comma));
+    } else {
+      // General case. Parse all available expressions.
+      bool Uneval = attributeParsedArgsUnevaluated(*AttrName);
+      EnterExpressionEvaluationContext Unevaluated(
+          Actions, Uneval
+                       ? Sema::ExpressionEvaluationContext::Unevaluated
+                       : Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
+      CommaLocsTy CommaLocs;
+      ExprVector ParsedExprs;
+      if (ParseExpressionList(ParsedExprs, CommaLocs,
+                              llvm::function_ref<void()>(),
+                              /*FailImmediatelyOnInvalidExpr=*/true,
+                              /*EarlyTypoCorrection=*/true)) {
+        SkipUntil(tok::r_paren, StopAtSemi);
+        return 0;
+      }
+
+      // Pack expansion must currently be explicitly supported by an attribute.
+      for (size_t I = 0; I < ParsedExprs.size(); ++I) {
+        if (!isa<PackExpansionExpr>(ParsedExprs[I]))
+          continue;
+
+        if (!attributeAcceptsExprPack(*AttrName)) {
+          Diag(Tok.getLocation(),
+               diag::err_attribute_argument_parm_pack_not_supported)
+              << AttrName;
+          SkipUntil(tok::r_paren, StopAtSemi);
+          return 0;
+        }
+      }
+
+      ArgExprs.insert(ArgExprs.end(), ParsedExprs.begin(), ParsedExprs.end());
+    }
   }
 
   SourceLocation RParen = Tok.getLocation();
   if (!ExpectAndConsume(tok::r_paren)) {
     SourceLocation AttrLoc = ScopeLoc.isValid() ? ScopeLoc : AttrNameLoc;
 
     if (AttributeIsTypeArgAttr && !TheParsedType.get().isNull()) {
       Attrs.addNewTypeAttr(AttrName, SourceRange(AttrNameLoc, RParen),

clang/lib/Parse/ParseExpr.cpp

 /// [C++0x]   initializer-list , initializer-clause ...[opt]
 ///
 /// [C++0x] initializer-clause:
 /// [C++0x]   assignment-expression
 /// [C++0x]   braced-init-list
 /// \endverbatim
 bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
                                  SmallVectorImpl<SourceLocation> &CommaLocs,
-                                 llvm::function_ref<void()> ExpressionStarts) {
+                                 llvm::function_ref<void()> ExpressionStarts,
+                                 bool FailImmediatelyOnInvalidExpr,
+                                 bool EarlyTypoCorrection) {
   bool SawError = false;
   while (true) {
     if (ExpressionStarts)
       ExpressionStarts();
 
     ExprResult Expr;
     if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace)) {
       Diag(Tok, diag::warn_cxx98_compat_generalized_initializer_lists);
       Expr = ParseBraceInitializer();
     } else
       Expr = ParseAssignmentExpression();
 
+    if (EarlyTypoCorrection)
+      Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
     if (Tok.is(tok::ellipsis))
       Expr = Actions.ActOnPackExpansion(Expr.get(), ConsumeToken());
     else if (Tok.is(tok::code_completion)) {
       // There's nothing to suggest in here as we parsed a full expression.
       // Instead fail and propogate the error since caller might have something
       // the suggest, e.g. signature help in function call. Note that this is
       // performed before pushing the \p Expr, so that signature help can report
       // current argument correctly.
       SawError = true;
       cutOffParsing();
       break;
     }
     if (Expr.isInvalid()) {
-      SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch);
       SawError = true;
+      if (FailImmediatelyOnInvalidExpr)
+        break;
+      SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch);
     } else {
       Exprs.push_back(Expr.get());
     }
 
     if (Tok.isNot(tok::comma))
       break;
     // Move to the next argument, remember where the comma was.
     Token Comma = Tok;

clang/lib/Sema/SemaDeclAttr.cpp

if (IdxSource == 1) {
       return false;
     }
   }
 
   Idx = ParamIdx(IdxSource, D);
   return true;
 }
 
+/// Check if the argument \p E is a ASCII string literal. If not emit an error
+/// and return false, otherwise set \p Str to the value of the string literal
+/// and return true.
+bool Sema::checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+                                  StringRef &Str, SourceLocation *ArgLocation) {
+  const auto *Literal = dyn_cast<StringLiteral>(E->IgnoreParenCasts());
+  if (ArgLocation)
+    *ArgLocation = E->getBeginLoc();
+
+  if (!Literal || !Literal->isAscii()) {
+    Diag(E->getBeginLoc(), diag::err_attribute_argument_type)
+        << CI << AANT_ArgumentString;
+    return false;
+  }
+
+  Str = Literal->getString();
+  return true;
+}
+
 /// Check if the argument \p ArgNum of \p Attr is a ASCII string literal.
 /// If not emit an error and return false. If the argument is an identifier it
 /// will emit an error with a fixit hint and treat it as if it was a string
 /// literal.
 bool Sema::checkStringLiteralArgumentAttr(const ParsedAttr &AL, unsigned ArgNum,
                                           StringRef &Str,
                                           SourceLocation *ArgLocation) {
   // Look for identifiers. If we have one emit a hint to fix it to a literal.
   if (AL.isArgIdent(ArgNum)) {
     IdentifierLoc *Loc = AL.getArgAsIdent(ArgNum);
     Diag(Loc->Loc, diag::err_attribute_argument_type)
         << AL << AANT_ArgumentString
         << FixItHint::CreateInsertion(Loc->Loc, "\"")
         << FixItHint::CreateInsertion(getLocForEndOfToken(Loc->Loc), "\"");
     Str = Loc->Ident->getName();
     if (ArgLocation)
       *ArgLocation = Loc->Loc;
     return true;
   }
 
   // Now check for an actual string literal.
   Expr *ArgExpr = AL.getArgAsExpr(ArgNum);
-  const auto *Literal = dyn_cast<StringLiteral>(ArgExpr->IgnoreParenCasts());
-  if (ArgLocation)
-    *ArgLocation = ArgExpr->getBeginLoc();
-
-  if (!Literal || !Literal->isAscii()) {
-    Diag(ArgExpr->getBeginLoc(), diag::err_attribute_argument_type)
-        << AL << AANT_ArgumentString;
-    return false;
-  }
-
-  Str = Literal->getString();
-  return true;
+  return checkStringLiteralExpr(AL, ArgExpr, Str, ArgLocation);
 }
 
 /// Applies the given attribute to the Decl without performing any
 /// additional semantic checking.
 template <typename AttrType>
 static void handleSimpleAttribute(Sema &S, Decl *D,
                                   const AttributeCommonInfo &CI) {
   D->addAttr(::new (S.Context) AttrType(S.Context, CI));
if (S.Context.getTypeSize(FieldType) != FirstSize ||
           << isSize << FirstBits;
       return;
     }
   }
 
   RD->addAttr(::new (S.Context) TransparentUnionAttr(S.Context, AL));
 }
 
+bool Sema::CheckAnnotationAttrArgExprs(const AttributeCommonInfo &CI,
+                                       MutableArrayRef<Expr *> ArgExprs,
+                                       StringRef &Annot,
+                                       SmallVector<Expr *, 4> &Args) {
+  assert(ArgExprs.size());
+  if (!checkStringLiteralExpr(CI, ArgExprs[0], Annot))
+    return false;
+  Args.reserve(ArgExprs.size() - 1);
+  for (unsigned I = 1; I < ArgExprs.size(); ++I)
+    Args.push_back(ArgExprs[I]);
+  return true;
+}
+
 void Sema::AddAnnotationAttr(Decl *D, const AttributeCommonInfo &CI,
                              StringRef Str, MutableArrayRef<Expr *> Args) {
   auto *Attr = AnnotateAttr::Create(Context, Str, Args.data(), Args.size(), CI);
   llvm::SmallVector<PartialDiagnosticAt, 8> Notes;
   for (unsigned Idx = 0; Idx < Attr->args_size(); Idx++) {
     Expr *&E = Attr->args_begin()[Idx];
     assert(E && "error are handled before");
     if (E->isValueDependent() || E->isTypeDependent())
if (!Result || !Notes.empty()) {
       return;
     }
     assert(Eval.Val.hasValue());
     E = ConstantExpr::Create(Context, E, Eval.Val);
   }
   D->addAttr(Attr);
 }
 
+void Sema::AddAnnotationAttrWithDelayedArgs(
+    Decl *D, const AttributeCommonInfo &CI,
+    MutableArrayRef<Expr *> DelayedArgs) {
+  auto *Attr = AnnotateAttr::Create(Context, "", nullptr, 0, DelayedArgs.data(),
+                                    DelayedArgs.size(), CI);
+  Attr->setArgsDelayed(true);
+  D->addAttr(Attr);
+}
+
 static void handleAnnotateAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  // Make sure that there is a string literal as the annotation's first
-  // argument.
-  StringRef Str;
-  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+  bool HasValueDependentArg = false;
+  llvm::SmallVector<Expr *, 4> AllArgs;
+  AllArgs.reserve(AL.getNumArgs());
+  for (unsigned I = 0; I < AL.getNumArgs(); ++I) {
+    assert(!AL.isArgIdent(I));
+    Expr *ArgExpr = AL.getArgAsExpr(I);
+    AllArgs.push_back(ArgExpr);
+    if (ArgExpr->isValueDependent())
+      HasValueDependentArg = true;
+  }
+
+  if (HasValueDependentArg) {
+    S.AddAnnotationAttrWithDelayedArgs(D, AL, AllArgs);
     return;
+  }
 
+  StringRef Str;
   llvm::SmallVector<Expr *, 4> Args;
-  Args.reserve(AL.getNumArgs() - 1);
-  for (unsigned Idx = 1; Idx < AL.getNumArgs(); Idx++) {
-    assert(!AL.isArgIdent(Idx));
-    Args.push_back(AL.getArgAsExpr(Idx));
-  }
+  if (!S.CheckAnnotationAttrArgExprs(AL, AllArgs, Str, Args))
+    return;
 
   S.AddAnnotationAttr(D, AL, Str, Args);
 }
 
 static void handleAlignValueAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   S.AddAlignValueAttr(D, AL, AL.getArgAsExpr(0));
 }
 
 void Sema::AddAlignValueAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E) {
   AlignValueAttr TmpAttr(Context, CI, E);
   SourceLocation AttrLoc = CI.getLoc();
 
   QualType T;
   if (const auto *TD = dyn_cast<TypedefNameDecl>(D))
     T = TD->getUnderlyingType();
   else if (const auto *VD = dyn_cast<ValueDecl>(D))
     T = VD->getType();
   else
     llvm_unreachable("Unknown decl type for align_value");
 
   if (!T->isDependentType() && !T->isAnyPointerType() &&
       !T->isReferenceType() && !T->isMemberPointerType()) {
     Diag(AttrLoc, diag::warn_attribute_pointer_or_reference_only)

 
 /// ProcessDeclAttribute - Apply the specific attribute to the specified decl if
 /// the attribute applies to decls.  If the attribute is a type attribute, just
 /// silently ignore it if a GNU attribute.
 static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
                                  const ParsedAttr &AL,
                                  bool IncludeCXX11Attributes) {
   if (AL.isInvalid() || AL.getKind() == ParsedAttr::IgnoredAttribute)
     return;
 
   // Ignore C++11 attributes on declarator chunks: they appertain to the type
   // instead.
   if (AL.isCXX11Attribute() && !IncludeCXX11Attributes)
     return;
 
   // Unknown attributes are automatically warned on. Target-specific attributes
   // which do not apply to the current target architecture are treated as

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

static void instantiateDependentAllocAlignAttr(
   S.AddAllocAlignAttr(New, *Align, Param);
 }
 
 static void instantiateDependentAnnotationAttr(
     Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
     const AnnotateAttr *Attr, Decl *New) {
   EnterExpressionEvaluationContext Unevaluated(
       S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+
+  StringRef Annotation;
   SmallVector<Expr *, 4> Args;
-  Args.reserve(Attr->args_size());
-  for (auto *E : Attr->args()) {
-    ExprResult Result = S.SubstExpr(E, TemplateArgs);
-    if (!Result.isUsable())
+  if (Attr->areArgsDelayed()) {
+    SmallVector<Expr *, 4> InstantiatedArgs;
+    if (S.SubstExprs(ArrayRef<Expr *>{Attr->delayedArgs_begin(),
+                                      Attr->delayedArgs_end()},
+                     /*IsCall=*/false, TemplateArgs, InstantiatedArgs))
+      return;
+    if (!S.CheckAnnotationAttrArgExprs(*Attr, InstantiatedArgs, Annotation,
+                                       Args))
+      return;
+  } else {
+    Annotation = Attr->getAnnotation();
+    if (S.SubstExprs(ArrayRef<Expr *>{Attr->args_begin(), Attr->args_end()},
+                     /*IsCall=*/false, TemplateArgs, Args))
       return;
-    Args.push_back(Result.get());
   }
-  S.AddAnnotationAttr(New, *Attr, Attr->getAnnotation(), Args);
+  S.AddAnnotationAttr(New, *Attr, Annotation, Args);
 }
 
 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);

clang/test/Parser/cxx0x-attributes.cpp

void foo () {
 
   void bar [[noreturn]] ([[]] int i, [[]] int j);
   using FuncType = void ([[]] int);
   void baz([[]]...); // expected-error {{expected parameter declarator}}
 
   [[]] return;
 }
 
 template<typename...Ts> void variadic() {
   void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot be used as an attribute pack}}
 }
 
+template <int... Is> void variadic_nttp() {
+  void bar [[noreturn...]] ();                        // expected-error {{attribute 'noreturn' cannot be used as an attribute pack}}
+  void baz [[clang::no_sanitize(Is...)]] ();          // expected-error {{attribute 'no_sanitize' does not support argument pack expansion}}
+  void bor [[clang::annotate("A", "V" ...)]] ();      // expected-error {{pack expansion does not contain any unexpanded parameter packs}}
+  void bir [[clang::annotate("B", {1, 2, 3, 4})]] (); // expected-error {{'annotate' attribute requires parameter 1 to be a constant expression}} expected-note {{subexpression not valid in a constant expression}}
+  void boo [[unknown::foo(Is...)]] ();                // expected-warning {{unknown attribute 'foo' ignored}}
+  void faz [[clang::annotate("C", (Is + ...))]] ();   // expected-warning {{pack fold expression is a C++17 extension}}
+  void far [[clang::annotate("D", Is...)]] ();
+  void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
+
 // Expression tests
 void bar () {
   // FIXME: GCC accepts [[gnu::noreturn]] on a lambda, even though it appertains
   // to the operator()'s type, and GCC does not otherwise accept attributes
   // applied to types. Use that to test this.
   [] () [[gnu::noreturn]] { return; } (); // expected-warning {{attribute 'noreturn' ignored}} FIXME-error {{should not return}}
   [] () [[gnu::noreturn]] { throw; } (); // expected-warning {{attribute 'noreturn' ignored}}
   new int[42][[]][5][[]]{};

clang/test/SemaTemplate/attributes.cpp

 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 // CHECK: FunctionDecl {{.*}} HasAnnotations
 // CHECK:   TemplateArgument type 'int'
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_FOO"
 // CHECK:   AnnotateAttr {{.*}} "ANNOTATE_BAR"
 template<typename T> [[clang::annotate("ANNOTATE_FOO"), clang::annotate("ANNOTATE_BAR")]] void HasAnnotations();
 void UseAnnotations() { HasAnnotations<int>(); }
 
+// CHECK:      FunctionTemplateDecl {{.*}} HasPackAnnotations
+// CHECK:        AnnotateAttr
+// CHECK-NEXT:     StringLiteral {{.*}} "ANNOTATE_BAZ"
+// CHECK:      FunctionDecl {{.*}} HasPackAnnotations
+// CHECK:        TemplateArgument{{.*}} pack
+// CHECK-NEXT:     TemplateArgument{{.*}} integral 1
+// CHECK-NEXT:     TemplateArgument{{.*}} integral 2
+// CHECK-NEXT:     TemplateArgument{{.*}} integral 3
+// CHECK:        AnnotateAttr {{.*}} "ANNOTATE_BAZ"
+// CHECK:          ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:       value: Int 1
+// CHECK:          ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:       value: Int 2
+// CHECK:          ConstantExpr {{.*}} 'int'
+// CHECK-NEXT:       value: Int 3
+template <int... Is> [[clang::annotate("ANNOTATE_BAZ", Is...)]] void HasPackAnnotations();
+void UsePackAnnotations() { HasPackAnnotations<1, 2, 3>(); }
+
 namespace preferred_name {
   int x [[clang::preferred_name("frank")]]; // expected-error {{expected a type}}
   int y [[clang::preferred_name(int)]]; // expected-warning {{'preferred_name' attribute only applies to class templates}}
   struct [[clang::preferred_name(int)]] A; // expected-warning {{'preferred_name' attribute only applies to class templates}}
   template<typename T> struct [[clang::preferred_name(int)]] B; // expected-error {{argument 'int' to 'preferred_name' attribute is not a typedef for a specialization of 'B'}}
   template<typename T> struct C;
   using X = C<int>; // expected-note {{'X' declared here}}
   typedef C<float> Y;
namespace preferred_name {
   auto it = desugar(MemberTemplate<int>::Iter<const int>());
   int n = it; // expected-error {{no viable conversion from 'preferred_name::MemberTemplate<int>::const_iterator' to 'int'}}
 
   template<int A, int B, typename ...T> struct Foo;
   template<typename ...T> using Bar = Foo<1, 2, T...>;
   template<int A, int B, typename ...T> struct [[clang::preferred_name(::preferred_name::Bar<T...>)]] Foo {};
   Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' in 'preferred_name::Bar<int, float>'}}
 }
 ::preferred_name::Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' in 'preferred_name::Bar<int, float>'}}

clang/utils/TableGen/ClangAttrEmitter.cpp

public:
 
     void writeDumpChildren(raw_ostream &OS) const override {
       OS << "    Visit(SA->get" << getUpperName() << "());\n";
     }
 
     void writeHasChildren(raw_ostream &OS) const override { OS << "true"; }
   };
 
   class VariadicExprArgument : public VariadicArgument {
   public:
     VariadicExprArgument(const Record &Arg, StringRef Attr)
       : VariadicArgument(Arg, Attr, "Expr *")
     {}
 
     void writeASTVisitorTraversal(raw_ostream &OS) const override {
       OS << "  {\n";
       OS << "    " << getType() << " *I = A->" << getLowerName()
static void forEachUniqueSpelling(const Record &Attr, Fn &&F) {
   std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(Attr);
   SmallDenseSet<StringRef, 8> Seen;
   for (const FlattenedSpelling &S : Spellings) {
     if (Seen.insert(S.name()).second)
       F(S);
   }
 }
 
+static bool isTypeArgument(const Record *Arg) {
+  return !Arg->getSuperClasses().empty() &&
+         Arg->getSuperClasses().back().first->getName() == "TypeArgument";
+}
+
 /// Emits the first-argument-is-type property for attributes.
 static void emitClangAttrTypeArgList(RecordKeeper &Records, raw_ostream &OS) {
   OS << "#if defined(CLANG_ATTR_TYPE_ARG_LIST)\n";
   std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
 
   for (const auto *Attr : Attrs) {
     // Determine whether the first argument is a type.
     std::vector<Record *> Args = Attr->getValueAsListOfDefs("Args");
     if (Args.empty())
       continue;
 
-    if (Args[0]->getSuperClasses().back().first->getName() != "TypeArgument")
+    if (!isTypeArgument(Args[0]))
       continue;
 
     // All these spellings take a single type argument.
     forEachUniqueSpelling(*Attr, [&](const FlattenedSpelling &S) {
       OS << ".Case(\"" << S.name() << "\", " << "true" << ")\n";
     });
   }
   OS << "#endif // CLANG_ATTR_TYPE_ARG_LIST\n\n";
for (const auto &I : Attrs) {
     // All these spellings take are parsed unevaluated.
     forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {
       OS << ".Case(\"" << S.name() << "\", " << "true" << ")\n";
     });
   }
   OS << "#endif // CLANG_ATTR_ARG_CONTEXT_LIST\n\n";
 }
 
-static bool isIdentifierArgument(Record *Arg) {
+static bool isIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
     llvm::StringSwitch<bool>(Arg->getSuperClasses().back().first->getName())
     .Case("IdentifierArgument", true)
     .Case("EnumArgument", true)
     .Case("VariadicEnumArgument", true)
     .Default(false);
 }
 
-static bool isVariadicIdentifierArgument(Record *Arg) {
+static bool isVariadicIdentifierArgument(const Record *Arg) {
   return !Arg->getSuperClasses().empty() &&
          llvm::StringSwitch<bool>(
              Arg->getSuperClasses().back().first->getName())
              .Case("VariadicIdentifierArgument", true)
              .Case("VariadicParamOrParamIdxArgument", true)
              .Default(false);
 }
 
forEachUniqueSpelling(*A, [&](const FlattenedSpelling &S) {
       OS << ".Case(\"" << S.name() << "\", "
          << "true"
          << ")\n";
     });
   }
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
+static void emitClangAttrAcceptsExprPack(RecordKeeper &Records,
+                                         raw_ostream &OS) {
+  OS << "#if defined(CLANG_ATTR_ACCEPTS_EXPR_PACK)\n";
+  ParsedAttrMap Attrs = getParsedAttrList(Records);
+  for (const auto &I : Attrs) {
+    const Record &Attr = *I.second;
+
+    if (!Attr.getValueAsBit("AcceptsExprPack"))
+      continue;
+
+    // All these spellings take are parsed unevaluated.
+    forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {
+      OS << ".Case(\"" << S.name() << "\", "
+         << "true"
+         << ")\n";
+    });
+  }
+  OS << "#endif // CLANG_ATTR_ACCEPTS_EXPR_PACK\n\n";
+}
+
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
                            bool Header) {
   std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr");
   ParsedAttrMap AttrMap = getParsedAttrList(Records);
 
   // Helper to print the starting character of an attribute argument. If there
   // hasn't been an argument yet, it prints an opening parenthese; otherwise it
   // prints a comma.
if (Header)
       OS << "class " << R.getName() << "Attr : public " << SuperName << " {\n";
     else
       OS << "\n// " << R.getName() << "Attr implementation\n\n";
 
     std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args");
     std::vector<std::unique_ptr<Argument>> Args;
     Args.reserve(ArgRecords.size());
 
+    bool AttrAcceptsExprPack = Attr->getValueAsBit("AcceptsExprPack");
+    assert((!AttrAcceptsExprPack ||
+            std::none_of(ArgRecords.begin(), ArgRecords.end(),
+                         [&](const Record *ArgR) {
+                           return isIdentifierArgument(ArgR) ||
+                                  isVariadicIdentifierArgument(ArgR) ||
+                                  isTypeArgument(ArgR);
+                         })) &&
+           "Attributes accepting packs cannot also have identifier or type "
+           "arguments.");
+
     bool HasOptArg = false;
     bool HasFakeArg = false;
     for (const auto *ArgRecord : ArgRecords) {
       Args.emplace_back(createArgument(*ArgRecord, R.getName()));
       if (Header) {
         Args.back()->writeDeclarations(OS);
         OS << "\n\n";
       }
if (Header) {
       for (auto const &ai : Args) {
         OS << ", ";
         ai->writeCloneArgs(OS);
       }
       OS << ");\n";
       OS << "  A->Inherited = Inherited;\n";
       OS << "  A->IsPackExpansion = IsPackExpansion;\n";
       OS << "  A->setImplicit(Implicit);\n";
+      OS << "  A->setArgsDelayed(ArgsDelayed);\n";
       OS << "  return A;\n}\n\n";
 
       writePrettyPrintFunction(R, Args, OS);
       writeGetSpellingFunction(R, OS);
     }
   }
 }
 // Emits the class definitions for attributes.
for (const auto *Attr : Attrs) {
     if (!R.getValueAsBit("ASTNode"))
       continue;
 
     OS << "  case attr::" << R.getName() << ": {\n";
     if (R.isSubClassOf(InhClass))
       OS << "    bool isInherited = Record.readInt();\n";
     OS << "    bool isImplicit = Record.readInt();\n";
     OS << "    bool isPackExpansion = Record.readInt();\n";
+    OS << "    bool areArgsDelayed = Record.readInt();\n";
     ArgRecords = R.getValueAsListOfDefs("Args");
     Args.clear();
     for (const auto *Arg : ArgRecords) {
       Args.emplace_back(createArgument(*Arg, R.getName()));
       Args.back()->writePCHReadDecls(OS);
     }
     OS << "    New = new (Context) " << R.getName() << "Attr(Context, Info";
     for (auto const &ri : Args) {
       OS << ", ";
       ri->writePCHReadArgs(OS);
     }
     OS << ");\n";
     if (R.isSubClassOf(InhClass))
       OS << "    cast<InheritableAttr>(New)->setInherited(isInherited);\n";
     OS << "    New->setImplicit(isImplicit);\n";
     OS << "    New->setPackExpansion(isPackExpansion);\n";
+    OS << "    New->setArgsDelayed(areArgsDelayed);\n";
     OS << "    break;\n";
     OS << "  }\n";
   }
   OS << "  }\n";
 }
 
 // Emits the code to write an attribute to a precompiled header.
 void EmitClangAttrPCHWrite(RecordKeeper &Records, raw_ostream &OS) {
for (const auto *Attr : Attrs) {
     Args = R.getValueAsListOfDefs("Args");
     if (R.isSubClassOf(InhClass) || !Args.empty())
       OS << "    const auto *SA = cast<" << R.getName()
          << "Attr>(A);\n";
     if (R.isSubClassOf(InhClass))
       OS << "    Record.push_back(SA->isInherited());\n";
     OS << "    Record.push_back(A->isImplicit());\n";
     OS << "    Record.push_back(A->isPackExpansion());\n";
+    OS << "    Record.push_back(A->areArgsDelayed());\n";
 
     for (const auto *Arg : Args)
       createArgument(*Arg, R.getName())->writePCHWrite(OS);
     OS << "    break;\n";
     OS << "  }\n";
   }
   OS << "  }\n";
 }

 
 void EmitClangAttrParserStringSwitches(RecordKeeper &Records,
                                        raw_ostream &OS) {
   emitSourceFileHeader("Parser-related llvm::StringSwitch cases", OS);
   emitClangAttrArgContextList(Records, OS);
   emitClangAttrIdentifierArgList(Records, OS);
   emitClangAttrVariadicIdentifierArgList(Records, OS);
   emitClangAttrThisIsaIdentifierArgList(Records, OS);
+  emitClangAttrAcceptsExprPack(Records, OS);
   emitClangAttrTypeArgList(Records, OS);
   emitClangAttrLateParsedList(Records, OS);
 }
 
 void EmitClangAttrSubjectMatchRulesParserStringSwitches(RecordKeeper &Records,
                                                         raw_ostream &OS) {
   getPragmaAttributeSupport(Records).generateParsingHelpers(OS);
 }