[PATCH] Attribute parsing following labels
Richard Smith
richard at metafoo.co.uk
Fri Nov 15 14:49:29 PST 2013
I made a few tweaks to the patch as I was reviewing, and I figured it'd be
easier to just check that in than have you make the same tweaks and check
in, so: committed with fixes for the below review comments as r194869.
--- lib/Parse/ParseStmt.cpp (revision 193647)
+++ lib/Parse/ParseStmt.cpp (working copy)
[...]
+ // attribute is immediately followed by a semi-colon to disambiguate it
from
+ // attributes on declarations.
This isn't quite right: in C, the attributes *always* apply to the label
(because a label cannot apply to a declaration). GCC only requires a
semicolon after the label in C++ mode.
+ StmtResult SubStmt;
+ if (Tok.is(tok::kw___attribute)) {
+ ParsedAttributesWithRange TempAttrs(AttrFactory);
+ ParseGNUAttributes(TempAttrs);
- StmtResult SubStmt(ParseStatement());
+ if (!TempAttrs.empty()) {
With this check, "A: __attribute__(( )) int n;" won't parse the "int n;",
and "A: __attribute__(( )) f();" won't trigger an error as it should. We
should do what follows regardless, because we know we saw a kw___attribute.
+ if (Tok.is(tok::semi))
+ attrs.takeAllFrom(TempAttrs);
We need to actually parse the substatement in this case.
+ } else
+ Diag(Tok, diag::err_expected_semi_after) << "__attribute__";
Again, we need to parse the substatement in this case.
On Wed, Oct 30, 2013 at 3:16 PM, Aaron Ballman <aaron at aaronballman.com>wrote:
> Finally have some spare time to look into this again, thank you for
> your patience. :-)
>
> On Tue, Oct 15, 2013 at 12:46 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > + SourceLocation Loc = Tok.getLocation();
> >
> > Unused variable.
> >
> > + // If the SubStmt is invalid, we want the attributes to attach to
> the
> > + // label instead.
> > + if (!SubStmt.isUsable() || SubStmt.isInvalid())
> > + attrs.takeAllFrom(TempAttrs);
> >
> > I thought your experiments showed the attributes were ill-formed in this
> > case.
>
> Since it's been a while, I went back to re-test my experiments, and I
> think I made a mistake.
>
> void f() {
> A:
> __attribute__((unused)) int i; // attribute applies to variable
> B:
> __attribute__((unused)); // attribute applies to label
>
> C:
> __attribute__((unused))
> #pragma weak unused_local_static
> ;
>
> D:
> #pragma weak unused_local_static
> __attribute__((unused))
> ;
> }
>
> *Both* of these cases generate errors in gcc. What I didn't realize is
> that gcc stopped trying to report errors with D because C was
> ill-formed.
>
> With D commented out, I get:
>
> prog.cpp: In function ‘void f()’:
> prog.cpp:8:7: error: expected primary-expression before ‘__attribute__’
> __attribute__((unused))
> ^
> prog.cpp:8:7: error: expected ‘;’ before ‘__attribute__’
> prog.cpp:9:39: error: expected ‘}’ before end of line
> #pragma weak unused_local_static
> ^
> prog.cpp:2:5: warning: label ‘A’ defined but not used [-Wunused-label]
> A:
> ^
> prog.cpp:7:5: warning: label ‘C’ defined but not used [-Wunused-label]
> C:
> ^
> prog.cpp: At global scope:
> prog.cpp:9:39: error: expected declaration before end of line
> #pragma weak unused_local_static
>
> With C commented out, I get:
>
> prog.cpp: In function ‘void f()’:
> prog.cpp:14:7: error: expected primary-expression before ‘__attribute__’
> __attribute__((unused))
> ^
> prog.cpp:14:7: error: expected ‘;’ before ‘__attribute__’
> prog.cpp:2:5: warning: label ‘A’ defined but not used [-Wunused-label]
> A:
> ^
> prog.cpp:12:5: warning: label ‘D’ defined but not used [-Wunused-label]
> D:
> ^
>
> So I've created a new patch that I believe has the correct behavior.
> If there's a semi-colon, the attributes are applied to the label. If
> there's a declaration, it is handled. Any other form of statement
> causes a specific error.
>
> Thanks!
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131115/c746cc79/attachment.html>
-------------- next part --------------
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 193546)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -327,6 +327,7 @@
return StmtEmpty();
case tok::annot_pragma_fp_contract:
+ ProhibitAttributes(Attrs);
Diag(Tok, diag::err_pragma_fp_contract_scope);
ConsumeToken();
return StmtError();
@@ -515,12 +516,37 @@
// identifier ':' statement
SourceLocation ColonLoc = ConsumeToken();
- // Read label attributes, if present. attrs will contain both C++11 and GNU
- // attributes (if present) after this point.
- MaybeParseGNUAttributes(attrs);
+ // Read label attributes, if present.
+ StmtResult SubStmt;
+ if (Tok.is(tok::kw___attribute)) {
+ ParsedAttributesWithRange TempAttrs(AttrFactory);
+ ParseGNUAttributes(TempAttrs);
- StmtResult SubStmt(ParseStatement());
+ // In C++, GNU attributes only apply to the label if they are followed by a
+ // semicolon, to disambiguate label attributes from attributes on a labeled
+ // declaration.
+ //
+ // This doesn't quite match what GCC does; if the attribute list is empty
+ // and followed by a semicolon, GCC will reject (it appears to parse the
+ // attributes as part of a statement in that case). That looks like a bug.
+ if (!getLangOpts().CPlusPlus || Tok.is(tok::semi))
+ attrs.takeAllFrom(TempAttrs);
+ else if (isDeclarationStatement()) {
+ StmtVector Stmts;
+ SubStmt = ParseStatementOrDeclarationAfterAttributes(
+ Stmts, /*OnlyStmts*/ true, 0, TempAttrs);
+ if (!TempAttrs.empty() && !SubStmt.isInvalid())
+ SubStmt = Actions.ProcessStmtAttributes(
+ SubStmt.get(), TempAttrs.getList(), TempAttrs.Range);
+ } else {
+ Diag(Tok, diag::err_expected_semi_after) << "__attribute__";
+ }
+ }
+ // If we've not parsed a statement yet, parse one now.
+ if (!SubStmt.isInvalid() && !SubStmt.isUsable())
+ SubStmt = ParseStatement();
+
// Broken substmt shouldn't prevent the label from being added to the AST.
if (SubStmt.isInvalid())
SubStmt = Actions.ActOnNullStmt(ColonLoc);
@@ -556,7 +582,7 @@
// out of stack space in our recursive descent parser. As a special case,
// flatten this recursion into an iterative loop. This is complex and gross,
// but all the grossness is constrained to ParseCaseStatement (and some
- // wierdness in the actions), so this is just local grossness :).
+ // weirdness in the actions), so this is just local grossness :).
// TopLevelCase - This is the highest level we have parsed. 'case 1' in the
// example above.
Index: test/Misc/ast-dump-attr.cpp
===================================================================
--- test/Misc/ast-dump-attr.cpp (revision 193546)
+++ test/Misc/ast-dump-attr.cpp (working copy)
@@ -95,3 +95,10 @@
void *TestVariadicUnsigned2(int, int) __attribute__((alloc_size(1,2)));
// CHECK: FunctionDecl{{.*}}TestVariadicUnsigned2
// CHECK: AllocSizeAttr{{.*}} 0 1
+
+void TestLabel() {
+L: __attribute__((unused)) int i;
+// CHECK: LabelStmt{{.*}}'L'
+// CHECK: VarDecl{{.*}}i 'int'
+// CHECK-NEXT: UnusedAttr{{.*}}
+}
\ No newline at end of file
Index: test/SemaCXX/warn-unused-variables.cpp
===================================================================
--- test/SemaCXX/warn-unused-variables.cpp (revision 193546)
+++ test/SemaCXX/warn-unused-variables.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -verify %s
template<typename T> void f() {
T t;
t = 17;
@@ -128,3 +128,26 @@
}
#include "Inputs/warn-unused-variables.h"
+
+namespace PR8455 {
+ void f() {
+ A: // expected-warning {{unused label 'A'}}
+ __attribute__((unused)) int i; // attribute applies to variable
+ B:
+ __attribute__((unused)); // attribute applies to label
+ }
+
+ void g() {
+ C: // unused label 'C' will not appear here because an error occurs
+ __attribute__((unused))
+ #pragma weak unused_local_static // expected-error {{expected ';' after __attribute__}}
+ ;
+ }
+
+ void h() {
+ D: // expected-warning {{unused label 'D'}}
+ #pragma weak unused_local_static
+ __attribute__((unused)) // expected-warning {{declaration does not declare anything}}
+ ;
+ }
+}
More information about the cfe-commits
mailing list