This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Parse GNU fallthrough attributes
AbandonedPublic

Authored by xbolva00 on Jun 13 2019, 2:46 PM.

Diff Detail

Repository
rC Clang

Event Timeline

xbolva00 created this revision.Jun 13 2019, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 2:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 marked an inline comment as done.Jun 14 2019, 4:39 AM

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 marked an inline comment as done.Jun 14 2019, 4:43 AM
xbolva00 added inline comments.
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 added inline comments.Jun 18 2019, 4:22 AM
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 abandoned this revision.Jun 18 2019, 4:40 AM
xbolva00 reclaimed this revision.Jun 18 2019, 6:57 AM

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 ?

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 abandoned this revision.Jun 18 2019, 8:46 AM

Right :(