diff options
author | Richard Smith <richard-llvm@metafoo.co.uk> | 2019-02-15 00:27:53 +0000 |
---|---|---|
committer | Richard Smith <richard-llvm@metafoo.co.uk> | 2019-02-15 00:27:53 +0000 |
commit | d1f4353f6e59e74948fb8442a8fda9b0bf379700 (patch) | |
tree | 757834eaede0e8913ca27f0c43fd1c8de7d3f2be /lib/Parse | |
parent | 7ff8f050d264a0ed5d52063d500216eb36021fe5 (diff) |
PR40642: Fix determination of whether the final statement of a statement
expression is a discarded-value expression.
Summary:
We used to get this wrong in three ways:
1) During parsing, an expression-statement followed by the }) ending a
statement expression was always treated as producing the value of the
statement expression. That's wrong for ({ if (1) expr; })
2) During template instantiation, various kinds of statement (most
statements not appearing directly in a compound-statement) were not
treated as discarded-value expressions, resulting in missing volatile
loads (etc).
3) In all contexts, an expression-statement with attributes was not
treated as producing the value of the statement expression, eg
({ [[attr]] expr; }).
Also fix incorrect enforcement of OpenMP rule that directives can "only
be placed in the program at a position where ignoring or deleting the
directive would result in a program with correct syntax". In particular,
a label (be it goto, case, or default) should not affect whether
directives are permitted.
Reviewers: aaron.ballman, rjmccall
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D57984
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@354090 91177308-0d34-0410-b5e6-96231b3b80d8
Diffstat (limited to 'lib/Parse')
-rw-r--r-- | lib/Parse/ParseObjc.cpp | 5 | ||||
-rw-r--r-- | lib/Parse/ParseOpenMP.cpp | 14 | ||||
-rw-r--r-- | lib/Parse/ParseStmt.cpp | 113 |
3 files changed, 78 insertions, 54 deletions
diff --git a/lib/Parse/ParseObjc.cpp b/lib/Parse/ParseObjc.cpp index 0a1dbf7bff..5ac01f6f0a 100644 --- a/lib/Parse/ParseObjc.cpp +++ b/lib/Parse/ParseObjc.cpp @@ -2703,7 +2703,8 @@ Decl *Parser::ParseObjCMethodDefinition() { return MDecl; } -StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) { +StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc, + ParsedStmtContext StmtCtx) { if (Tok.is(tok::code_completion)) { Actions.CodeCompleteObjCAtStatement(getCurScope()); cutOffParsing(); @@ -2740,7 +2741,7 @@ StmtResult Parser::ParseObjCAtStatement(SourceLocation AtLoc) { // Otherwise, eat the semicolon. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - return Actions.ActOnExprStmt(Res, isExprValueDiscarded()); + return handleExprStmt(Res, StmtCtx); } ExprResult Parser::ParseObjCAtExpression(SourceLocation AtLoc) { diff --git a/lib/Parse/ParseOpenMP.cpp b/lib/Parse/ParseOpenMP.cpp index 5b8670e928..d0463416f5 100644 --- a/lib/Parse/ParseOpenMP.cpp +++ b/lib/Parse/ParseOpenMP.cpp @@ -1132,8 +1132,8 @@ Parser::DeclGroupPtrTy Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl( /// 'target teams distribute simd' {clause} /// annot_pragma_openmp_end /// -StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective( - AllowedConstructsKind Allowed) { +StmtResult +Parser::ParseOpenMPDeclarativeOrExecutableDirective(ParsedStmtContext StmtCtx) { assert(Tok.is(tok::annot_pragma_openmp) && "Not an OpenMP directive!"); ParenBraceBracketBalancer BalancerRAIIObj(*this); SmallVector<OMPClause *, 5> Clauses; @@ -1152,7 +1152,9 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective( switch (DKind) { case OMPD_threadprivate: { - if (Allowed != ACK_Any) { + // FIXME: Should this be permitted in C++? + if ((StmtCtx & ParsedStmtContext::AllowDeclarationsInC) == + ParsedStmtContext()) { Diag(Tok, diag::err_omp_immediate_directive) << getOpenMPDirectiveName(DKind) << 0; } @@ -1219,7 +1221,8 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective( case OMPD_target_enter_data: case OMPD_target_exit_data: case OMPD_target_update: - if (Allowed == ACK_StatementsOpenMPNonStandalone) { + if ((StmtCtx & ParsedStmtContext::AllowStandaloneOpenMPDirectives) == + ParsedStmtContext()) { Diag(Tok, diag::err_omp_immediate_directive) << getOpenMPDirectiveName(DKind) << 0; } @@ -1322,7 +1325,8 @@ StmtResult Parser::ParseOpenMPDeclarativeOrExecutableDirective( // If the depend clause is specified, the ordered construct is a stand-alone // directive. if (DKind == OMPD_ordered && FirstClauses[OMPC_depend].getInt()) { - if (Allowed == ACK_StatementsOpenMPNonStandalone) { + if ((StmtCtx & ParsedStmtContext::AllowStandaloneOpenMPDirectives) == + ParsedStmtContext()) { Diag(Loc, diag::err_omp_immediate_directive) << getOpenMPDirectiveName(DKind) << 1 << getOpenMPClauseName(OMPC_depend); diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp index 3fc29253f0..267003b518 100644 --- a/lib/Parse/ParseStmt.cpp +++ b/lib/Parse/ParseStmt.cpp @@ -29,17 +29,14 @@ using namespace clang; /// Parse a standalone statement (for instance, as the body of an 'if', /// 'while', or 'for'). StmtResult Parser::ParseStatement(SourceLocation *TrailingElseLoc, - bool AllowOpenMPStandalone) { + ParsedStmtContext StmtCtx) { StmtResult Res; // We may get back a null statement if we found a #pragma. Keep going until // we get an actual statement. do { StmtVector Stmts; - Res = ParseStatementOrDeclaration( - Stmts, AllowOpenMPStandalone ? ACK_StatementsOpenMPAnyExecutable - : ACK_StatementsOpenMPNonStandalone, - TrailingElseLoc); + Res = ParseStatementOrDeclaration(Stmts, StmtCtx, TrailingElseLoc); } while (!Res.isInvalid() && !Res.get()); return Res; @@ -96,7 +93,7 @@ StmtResult Parser::ParseStatement(SourceLocation *TrailingElseLoc, /// StmtResult Parser::ParseStatementOrDeclaration(StmtVector &Stmts, - AllowedConstructsKind Allowed, + ParsedStmtContext StmtCtx, SourceLocation *TrailingElseLoc) { ParenBraceBracketBalancer BalancerRAIIObj(*this); @@ -107,7 +104,7 @@ Parser::ParseStatementOrDeclaration(StmtVector &Stmts, return StmtError(); StmtResult Res = ParseStatementOrDeclarationAfterAttributes( - Stmts, Allowed, TrailingElseLoc, Attrs); + Stmts, StmtCtx, TrailingElseLoc, Attrs); assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) && "attributes on empty statement"); @@ -147,10 +144,9 @@ private: }; } -StmtResult -Parser::ParseStatementOrDeclarationAfterAttributes(StmtVector &Stmts, - AllowedConstructsKind Allowed, SourceLocation *TrailingElseLoc, - ParsedAttributesWithRange &Attrs) { +StmtResult Parser::ParseStatementOrDeclarationAfterAttributes( + StmtVector &Stmts, ParsedStmtContext StmtCtx, + SourceLocation *TrailingElseLoc, ParsedAttributesWithRange &Attrs) { const char *SemiError = nullptr; StmtResult Res; @@ -165,7 +161,7 @@ Retry: { ProhibitAttributes(Attrs); // TODO: is it correct? AtLoc = ConsumeToken(); // consume @ - return ParseObjCAtStatement(AtLoc); + return ParseObjCAtStatement(AtLoc, StmtCtx); } case tok::code_completion: @@ -177,7 +173,7 @@ Retry: Token Next = NextToken(); if (Next.is(tok::colon)) { // C99 6.8.1: labeled-statement // identifier ':' statement - return ParseLabeledStatement(Attrs); + return ParseLabeledStatement(Attrs, StmtCtx); } // Look up the identifier, and typo-correct it to a keyword if it's not @@ -207,7 +203,8 @@ Retry: default: { if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt || - Allowed == ACK_Any) && + (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) != + ParsedStmtContext()) && isDeclarationStatement()) { SourceLocation DeclStart = Tok.getLocation(), DeclEnd; DeclGroupPtrTy Decl = ParseDeclaration(DeclaratorContext::BlockContext, @@ -220,13 +217,13 @@ Retry: return StmtError(); } - return ParseExprStatement(); + return ParseExprStatement(StmtCtx); } case tok::kw_case: // C99 6.8.1: labeled-statement - return ParseCaseStatement(); + return ParseCaseStatement(StmtCtx); case tok::kw_default: // C99 6.8.1: labeled-statement - return ParseDefaultStatement(); + return ParseDefaultStatement(StmtCtx); case tok::l_brace: // C99 6.8.2: compound-statement return ParseCompoundStatement(); @@ -363,7 +360,7 @@ Retry: case tok::annot_pragma_openmp: ProhibitAttributes(Attrs); - return ParseOpenMPDeclarativeOrExecutableDirective(Allowed); + return ParseOpenMPDeclarativeOrExecutableDirective(StmtCtx); case tok::annot_pragma_ms_pointers_to_members: ProhibitAttributes(Attrs); @@ -382,7 +379,7 @@ Retry: case tok::annot_pragma_loop_hint: ProhibitAttributes(Attrs); - return ParsePragmaLoopHint(Stmts, Allowed, TrailingElseLoc, Attrs); + return ParsePragmaLoopHint(Stmts, StmtCtx, TrailingElseLoc, Attrs); case tok::annot_pragma_dump: HandlePragmaDump(); @@ -407,7 +404,7 @@ Retry: } /// Parse an expression statement. -StmtResult Parser::ParseExprStatement() { +StmtResult Parser::ParseExprStatement(ParsedStmtContext StmtCtx) { // If a case keyword is missing, this is where it should be inserted. Token OldToken = Tok; @@ -433,12 +430,12 @@ StmtResult Parser::ParseExprStatement() { << FixItHint::CreateInsertion(OldToken.getLocation(), "case "); // Recover parsing as a case statement. - return ParseCaseStatement(/*MissingCase=*/true, Expr); + return ParseCaseStatement(StmtCtx, /*MissingCase=*/true, Expr); } // Otherwise, eat the semicolon. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - return Actions.ActOnExprStmt(Expr, isExprValueDiscarded()); + return handleExprStmt(Expr, StmtCtx); } /// ParseSEHTryBlockCommon @@ -577,10 +574,15 @@ StmtResult Parser::ParseSEHLeaveStatement() { /// identifier ':' statement /// [GNU] identifier ':' attributes[opt] statement /// -StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs) { +StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs, + ParsedStmtContext StmtCtx) { assert(Tok.is(tok::identifier) && Tok.getIdentifierInfo() && "Not an identifier!"); + // The substatement is always a 'statement', not a 'declaration', but is + // otherwise in the same context as the labeled-statement. + StmtCtx &= ~ParsedStmtContext::AllowDeclarationsInC; + Token IdentTok = Tok; // Save the whole token. ConsumeToken(); // eat the identifier. @@ -610,9 +612,8 @@ StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs) { // statement, but that doesn't work correctly (because ProhibitAttributes // can't handle GNU attributes), so only call it in the one case where // GNU attributes are allowed. - SubStmt = ParseStatementOrDeclarationAfterAttributes( - Stmts, /*Allowed=*/ACK_StatementsOpenMPNonStandalone, nullptr, - TempAttrs); + SubStmt = ParseStatementOrDeclarationAfterAttributes(Stmts, StmtCtx, + nullptr, TempAttrs); if (!TempAttrs.empty() && !SubStmt.isInvalid()) SubStmt = Actions.ProcessStmtAttributes(SubStmt.get(), TempAttrs, TempAttrs.Range); @@ -623,7 +624,7 @@ StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs) { // If we've not parsed a statement yet, parse one now. if (!SubStmt.isInvalid() && !SubStmt.isUsable()) - SubStmt = ParseStatement(); + SubStmt = ParseStatement(nullptr, StmtCtx); // Broken substmt shouldn't prevent the label from being added to the AST. if (SubStmt.isInvalid()) @@ -643,9 +644,14 @@ StmtResult Parser::ParseLabeledStatement(ParsedAttributesWithRange &attrs) { /// 'case' constant-expression ':' statement /// [GNU] 'case' constant-expression '...' constant-expression ':' statement /// -StmtResult Parser::ParseCaseStatement(bool MissingCase, ExprResult Expr) { +StmtResult Parser::ParseCaseStatement(ParsedStmtContext StmtCtx, + bool MissingCase, ExprResult Expr) { assert((MissingCase || Tok.is(tok::kw_case)) && "Not a case stmt!"); + // The substatement is always a 'statement', not a 'declaration', but is + // otherwise in the same context as the labeled-statement. + StmtCtx &= ~ParsedStmtContext::AllowDeclarationsInC; + // It is very very common for code to contain many case statements recursively // nested, as in (but usually without indentation): // case 1: @@ -737,8 +743,7 @@ StmtResult Parser::ParseCaseStatement(bool MissingCase, ExprResult Expr) { // continue parsing the sub-stmt. if (Case.isInvalid()) { if (TopLevelCase.isInvalid()) // No parsed case stmts. - return ParseStatement(/*TrailingElseLoc=*/nullptr, - /*AllowOpenMPStandalone=*/true); + return ParseStatement(/*TrailingElseLoc=*/nullptr, StmtCtx); // Otherwise, just don't add it as a nested case. } else { // If this is the first case statement we parsed, it becomes TopLevelCase. @@ -758,8 +763,7 @@ StmtResult Parser::ParseCaseStatement(bool MissingCase, ExprResult Expr) { StmtResult SubStmt; if (Tok.isNot(tok::r_brace)) { - SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, - /*AllowOpenMPStandalone=*/true); + SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, StmtCtx); } else { // Nicely diagnose the common error "switch (X) { case 4: }", which is // not valid. If ColonLoc doesn't point to a valid text location, there was @@ -789,8 +793,13 @@ StmtResult Parser::ParseCaseStatement(bool MissingCase, ExprResult Expr) { /// 'default' ':' statement /// Note that this does not parse the 'statement' at the end. /// -StmtResult Parser::ParseDefaultStatement() { +StmtResult Parser::ParseDefaultStatement(ParsedStmtContext StmtCtx) { assert(Tok.is(tok::kw_default) && "Not a default stmt!"); + + // The substatement is always a 'statement', not a 'declaration', but is + // otherwise in the same context as the labeled-statement. + StmtCtx &= ~ParsedStmtContext::AllowDeclarationsInC; + SourceLocation DefaultLoc = ConsumeToken(); // eat the 'default'. SourceLocation ColonLoc; @@ -811,8 +820,7 @@ StmtResult Parser::ParseDefaultStatement() { StmtResult SubStmt; if (Tok.isNot(tok::r_brace)) { - SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, - /*AllowOpenMPStandalone=*/true); + SubStmt = ParseStatement(/*TrailingElseLoc=*/nullptr, StmtCtx); } else { // Diagnose the common error "switch (X) {... default: }", which is // not valid. @@ -943,7 +951,8 @@ bool Parser::ConsumeNullStmt(StmtVector &Stmts) { EndLoc = Tok.getLocation(); // Don't just ConsumeToken() this tok::semi, do store it in AST. - StmtResult R = ParseStatementOrDeclaration(Stmts, ACK_Any); + StmtResult R = + ParseStatementOrDeclaration(Stmts, ParsedStmtContext::SubStmt); if (R.isUsable()) Stmts.push_back(R.get()); } @@ -957,14 +966,18 @@ bool Parser::ConsumeNullStmt(StmtVector &Stmts) { return true; } -bool Parser::isExprValueDiscarded() { - if (Actions.isCurCompoundStmtAStmtExpr()) { - // Look to see if the next two tokens close the statement expression; +StmtResult Parser::handleExprStmt(ExprResult E, ParsedStmtContext StmtCtx) { + bool IsStmtExprResult = false; + if ((StmtCtx & ParsedStmtContext::InStmtExpr) != ParsedStmtContext()) { + // Look ahead to see if the next two tokens close the statement expression; // if so, this expression statement is the last statement in a // statment expression. - return Tok.isNot(tok::r_brace) || NextToken().isNot(tok::r_paren); + IsStmtExprResult = Tok.is(tok::r_brace) && NextToken().is(tok::r_paren); } - return true; + + if (IsStmtExprResult) + E = Actions.ActOnStmtExprResult(E); + return Actions.ActOnExprStmt(E, /*DiscardedValue=*/!IsStmtExprResult); } /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the @@ -1022,6 +1035,10 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) { Stmts.push_back(R.get()); } + ParsedStmtContext SubStmtCtx = + ParsedStmtContext::Compound | + (isStmtExpr ? ParsedStmtContext::InStmtExpr : ParsedStmtContext()); + while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) { if (Tok.is(tok::annot_pragma_unused)) { @@ -1034,7 +1051,7 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) { StmtResult R; if (Tok.isNot(tok::kw___extension__)) { - R = ParseStatementOrDeclaration(Stmts, ACK_Any); + R = ParseStatementOrDeclaration(Stmts, SubStmtCtx); } else { // __extension__ can start declarations and it can also be a unary // operator for expressions. Consume multiple __extension__ markers here @@ -1067,11 +1084,12 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) { continue; } - // FIXME: Use attributes? // Eat the semicolon at the end of stmt and convert the expr into a // statement. ExpectAndConsumeSemi(diag::err_expected_semi_after_expr); - R = Actions.ActOnExprStmt(Res, isExprValueDiscarded()); + R = handleExprStmt(Res, SubStmtCtx); + if (R.isUsable()) + R = Actions.ProcessStmtAttributes(R.get(), attrs, attrs.Range); } } @@ -2001,7 +2019,7 @@ StmtResult Parser::ParseReturnStatement() { } StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, - AllowedConstructsKind Allowed, + ParsedStmtContext StmtCtx, SourceLocation *TrailingElseLoc, ParsedAttributesWithRange &Attrs) { // Create temporary attribute list. @@ -2024,7 +2042,7 @@ StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, MaybeParseCXX11Attributes(Attrs); StmtResult S = ParseStatementOrDeclarationAfterAttributes( - Stmts, Allowed, TrailingElseLoc, Attrs); + Stmts, StmtCtx, TrailingElseLoc, Attrs); Attrs.takeAllFrom(TempAttrs); return S; @@ -2329,7 +2347,8 @@ void Parser::ParseMicrosoftIfExistsStatement(StmtVector &Stmts) { // Condition is true, parse the statements. while (Tok.isNot(tok::r_brace)) { - StmtResult R = ParseStatementOrDeclaration(Stmts, ACK_Any); + StmtResult R = + ParseStatementOrDeclaration(Stmts, ParsedStmtContext::Compound); if (R.isUsable()) Stmts.push_back(R.get()); } |