[Clang] Parse GNU fallthrough attributes

[xbolva00]

Summary

Please https://reviews.llvm.org/D63260

[xbolva00]

As we see. this is not good place to parse attribute((fallthough), which blocks D63260. Maybe @aaron.ballman could help us since he knows this code better..

[xbolva00]

isDeclarationStatement() returns true for attribute((fallthough)) ;

lib/Parse/ParseStmt.cpp

102

Maybe we check if Tok is kw__attribute and look ahead a few tokens to check if attribute name is fallthough in ParseStatementOrDeclarationAfterAttributes.

Now, we always fall into

if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||

       (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
           ParsedStmtContext()) &&
      isDeclarationStatement()) {
    SourceLocation DeclStart = Tok.getLocation(), DeclEnd;
    DeclGroupPtrTy Decl = ParseDeclaration(DeclaratorContext::BlockContext,
                                           DeclEnd, Attrs);
    return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
}

[xbolva00]

lib/Parse/ParseStmt.cpp

102

Then we go to "ParseSimpleDeclaration" -> "ParseDeclarationSpecifier". What is quite strange for me, we do not set "Attrs" in ParseSimpleDeclaration from DS.getAttributes()..

[aaron.ballman]

lib/Parse/ParseStmt.cpp

102

Maybe we check if Tok is kw__attribute and look ahead a few tokens to check if attribute name is fallthough in ParseStatementOrDeclarationAfterAttributes.

Why not check whether the attribute is a statement attribute or a declaration attribute, rather than tying it to fallthrough specifically?

But the result then would be: if it's a statement attribute, we should be trying to parse it as a statement rather than declaration statement; if it's a declaration attribute, we should try to parse as a declaration. This seems very much like spooky action at a distance -- attributes should not dictate whether something is parsed as a decl or a stmt.

That said, this really is the right place for that code to go, I think. We've never had a GNU null attribute statement before, so the parser may be written in an inconvenient way to support that (IIRC, we have similar deficiencies with things like attributes appertaining to declaration specifiers -- we don't have attributes that need it yet, so support was never added for it).

This may require a bit more surgery to fix up, unfortunately.

[xbolva00]

Ok, If we peek a few tokens ahead and check attribute if it is stmt attribute and then we call MaybeParseGNUAttr - this probably would work but are you fine with it as a workaround ?

[aaron.ballman]

In D63299#1548306, @xbolva00 wrote:

Ok, If we peek a few tokens ahead and check attribute if it is stmt attribute and then we call MaybeParseGNUAttr - this probably would work but are you fine with it as a workaround ?

That still seems like the same spooky action at a distance because it means a statement vs declaration attribute will change the parsing behavior.

[xbolva00]

Right :(

lib/Parse/ParseStmt.cpp

 StmtResult
 Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
                                     ParsedStmtContext StmtCtx,
                                     SourceLocation *TrailingElseLoc) {
 
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
   ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseGNUAttributes(Attrs);
   MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
   if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
     return StmtError();
 
   StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
       Stmts, StmtCtx, TrailingElseLoc, Attrs);
 
   assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) &&

test/Sema/address_spaces.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify
 
 #define _AS1 __attribute__((address_space(1)))
 #define _AS2 __attribute__((address_space(2)))
 #define _AS3 __attribute__((address_space(3)))
 
 void bar(_AS2 int a); // expected-error {{parameter may not be qualified with an address space}}
 
 void foo(_AS3 float *a,
          _AS1 float b) // expected-error {{parameter may not be qualified with an address space}}
 {
-  _AS2 *x;// expected-warning {{type specifier missing, defaults to 'int'}}
+  _AS2 *x;// // expected-warning {{use of undeclared identifier 'x'}}
   _AS1 float * _AS2 *B;
 
   int _AS1 _AS2 *Y;   // expected-error {{multiple address spaces specified for type}}
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified for type}}
   int *_AS1 _AS1 *M;  // expected-warning {{multiple identical address spaces specified for type}}
 
   _AS1 int local;     // expected-error {{automatic variable qualified with an address space}}
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}

test/Sema/block-literal.c

takeblock(^{
         goto foo; // expected-error {{use of undeclared label 'foo'}}
         a: goto a;       // ok
       });
     break;
   }
 
   foo:
   takeblock(^{ x = 4; });  // expected-error {{variable is not assignable (missing __block type specifier)}}
-  __block y = 7;    // expected-warning {{type specifier missing, defaults to 'int'}}
-  takeblock(^{ y = 8; });
+  __block y = 7;    // expected-warning {{use of undeclared identifier 'y'}}
+  takeblock(^{ y = 8; }); // expected-warning {{use of undeclared identifier 'y'}}
 }
 
 
 void (^test3())(void) { 
   return ^{};
 }
 
 void test4() {