[PATCH] Attribute parsing following labels

[Aaron Ballman]

On Mon, Oct 7, 2013 at 4:41 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Mon, Oct 7, 2013 at 1:21 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, Oct 7, 2013 at 4:16 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > +  // Read label attributes, if present. TempAttrs will contain both
>> > C++11
>> > and
>> > +  // GNU attributes (if present) after this point.
>> >
>> > This isn't true: we're only parsing GNU attributes here.
>>
>> Sorry, that's a holdover from the existing comment.  I'll correct it.
>> >
>> > +    } else {
>> > +      StmtVector Stmts;
>> > +      SubStmt = ParseStatementOrDeclarationAfterAttributes(Stmts, true,
>> > 0,
>> > +                                                           TempAttrs);
>> >
>> > This doesn't look like it'll correctly handle the case where
>> > ParseStatementOrDeclarationAfterAttributes returns null (because we
>> > parsed a
>> > pragma or the like). If you're relying on this being impossible (because
>> > an
>> > attribute can't apply to a #pragma), please insert an assert here (that
>> > SubStmt is invalid or usable).
>>
>> Doesn't that get handled slightly further down?
>>
>>   // Broken substmt shouldn't prevent the label from being added to the
>> AST.
>>   if (SubStmt.isInvalid())
>>     SubStmt = Actions.ActOnNullStmt(ColonLoc);
>
>
> This does not handle the case where SubStmt is valid but null.
> ParseStatementOrDeclarationAfterAttributes returns such a value if it hits a
> #pragma.

What I am finding in practice is that ConsumeToken for the colon is
also consuming the #pragma in this test case:

void f() {
// Invalid
D: // expected-warning {{unused label 'D'}}
#pragma message "stuff"  // expected-warning {{stuff}}
__attribute__((unused))  // expected-error {{expected expression}}
;
}

So ParseStatementOrDeclarationAfterAttributes does not return a null
statement because it never sees the #pragma.  I'm not certain of the
proper way to proceed as there appears to be no way to tell
ConsumeToken to not consume the pragma.

~Aaron