Re: r197553 - [OPENMP] Fix for parsing OpenMP directives with extra braces, brackets and parens

[Alexey Bataev]

Hi Alp, Aaron,
Thanks for the patches.
Sorry for all the troubles with my patch. My apologies.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team
Intel Corp.

18.12.2013 23:17, Alp Toker пишет:

Reviewed by Aaron, r197597 and r197598.

Alp.


On 18/12/2013 18:48, Alp Toker wrote:

Hi,

SkipUntil and BalancedDelimiterTracker are meant to track balanced tokens so this was inherently an incorrect change to the Parser.

The same or better recovery can be achieved, and the tests made to pass, by reverting r197553 and applying this one line fix to SkipUntil():

--- a/lib/Parse/Parser.cpp
+++ b/lib/Parse/Parser.cpp

@@ -298,6 +298,7 @@ bool Parser::SkipUntil(ArrayRef<tok::TokenKind> Toks, SkipUntilFlags Flags) {

// Ran out of tokens.
return false;

+ case tok::annot_pragma_openmp_end:
case tok::annot_module_begin:
case tok::annot_module_end:
case tok::annot_module_include:


Alp.



On 18/12/2013 18:03, Alp Toker wrote:

On 18/12/2013 17:30, Alp Toker wrote:

Hi Alexey,

There's a lot I don't understand about your commit so I'm somewhat uncomfortable here.

I tried reverting your changes to BalancedDelimiterTracker locally and the tests still pass.

It seems to go against the main purpose of ExpectAndConsume() and BalancedDelimiterTracker to skip counting braces, and I don't like the added complexity impacting key C/C++ parse facilities.

Would a simple custom consume-to-end-of-annotation while loop serve your purpose better?

Also, if you want to diagnose and provide recovery for this error it shouldn't be specific to 'omp' pragmas. There's a strong expectation that recovery should work the same and I think that'll fall into place naturally if you implement this properly instead of the way you did.

So this patch has enough complexity (and issues in its current form, including the other ones Aaron cited) to roll it out and put it through pre-commit review. It's entirely possible though that I missed something -- what do you think?

Joey pointed out that Hal did in fact review some of these changes last month. We're discussing it now to see if the less intrusive approach can work.

Alp.

Alp.




On 18/12/2013 08:46, Alexey Bataev wrote:

Author: abataev
Date: Wed Dec 18 02:46:25 2013
New Revision: 197553

URL: http://llvm.org/viewvc/llvm-project?rev=197553&view=rev
Log:

[OPENMP] Fix for parsing OpenMP directives with extra braces, brackets and parens

Modified:
cfe/trunk/include/clang/Parse/Parser.h
cfe/trunk/lib/Parse/ParseOpenMP.cpp
cfe/trunk/lib/Parse/Parser.cpp
cfe/trunk/lib/Parse/RAIIObjectsForParser.h
cfe/trunk/test/OpenMP/parallel_messages.cpp
cfe/trunk/test/OpenMP/threadprivate_messages.cpp

Modified: cfe/trunk/include/clang/Parse/Parser.h

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/include/clang/Parse/Parser.h (original)
+++ cfe/trunk/include/clang/Parse/Parser.h Wed Dec 18 02:46:25 2013
@@ -684,9 +684,13 @@ private:

/// If the input is malformed, this emits the specified diagnostic. Next, if /// SkipToTok is specified, it calls SkipUntil(SkipToTok). Finally, true is

/// returned.

+ /// If NoCount is true, it ignores parens/brackets/braces as regular tokens + /// and does not count them. By default it recursively skips properly-nested

+ /// parens/brackets/braces.
bool ExpectAndConsume(tok::TokenKind ExpectedTok, unsigned Diag,
const char *DiagMsg = "",
- tok::TokenKind SkipToTok = tok::unknown);
+ tok::TokenKind SkipToTok = tok::unknown,
+ bool NoCount = false);

/// \brief The parser expects a semicolon and, if present, will consume it.

///
@@ -789,7 +793,9 @@ public:
StopAtSemi = 1 << 0, ///< Stop skipping at semicolon

/// \brief Stop skipping at specified token, but don't skip the token itself

StopBeforeMatch = 1 << 1,
- StopAtCodeCompletion = 1 << 2 ///< Stop at code completion
+ StopAtCodeCompletion = 1 << 2, ///< Stop at code completion

+ NoBracketsCount = 1 << 3 /// \brief Don't count braces/brackets/parens

+ /// to skip balanced pairs
};
friend LLVM_CONSTEXPR SkipUntilFlags operator|(SkipUntilFlags L,

Modified: cfe/trunk/lib/Parse/ParseOpenMP.cpp

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseOpenMP.cpp?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/lib/Parse/ParseOpenMP.cpp (original)
+++ cfe/trunk/lib/Parse/ParseOpenMP.cpp Wed Dec 18 02:46:25 2013
@@ -48,7 +48,7 @@ Parser::DeclGroupPtrTy Parser::ParseOpen
if (Tok.isNot(tok::annot_pragma_openmp_end)) {
Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
<< getOpenMPDirectiveName(OMPD_threadprivate);
- SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);

+ SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);

}
// Skip the last annot_pragma_openmp_end.
ConsumeToken();
@@ -66,7 +66,7 @@ Parser::DeclGroupPtrTy Parser::ParseOpen
<< getOpenMPDirectiveName(DKind);
break;
}
- SkipUntil(tok::annot_pragma_openmp_end);
+ SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
return DeclGroupPtrTy();
}
@@ -105,14 +105,14 @@ StmtResult Parser::ParseOpenMPDeclarativ
if (Tok.isNot(tok::annot_pragma_openmp_end)) {
Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
<< getOpenMPDirectiveName(OMPD_threadprivate);
- SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);

+ SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);

}
DeclGroupPtrTy Res =
Actions.ActOnOpenMPThreadprivateDirective(Loc,
Identifiers);
Directive = Actions.ActOnDeclStmt(Res, Loc, Tok.getLocation());
}
- SkipUntil(tok::annot_pragma_openmp_end);
+ SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
break;
case OMPD_parallel: {
ConsumeToken();
@@ -171,13 +171,13 @@ StmtResult Parser::ParseOpenMPDeclarativ
break;
case OMPD_unknown:
Diag(Tok, diag::err_omp_unknown_directive);
- SkipUntil(tok::annot_pragma_openmp_end);
+ SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
break;
case OMPD_task:
case NUM_OPENMP_DIRECTIVES:
Diag(Tok, diag::err_omp_unexpected_directive)
<< getOpenMPDirectiveName(DKind);
- SkipUntil(tok::annot_pragma_openmp_end);
+ SkipUntil(tok::annot_pragma_openmp_end, NoBracketsCount);
break;
}
return Directive;
@@ -194,7 +194,8 @@ bool Parser::ParseOpenMPSimpleVarList(Op
bool AllowScopeSpecifier) {
VarList.clear();
// Parse '('.

- BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end); + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,

+ true);
if (T.expectAndConsume(diag::err_expected_lparen_after,
getOpenMPDirectiveName(Kind)))
return true;
@@ -214,17 +215,17 @@ bool Parser::ParseOpenMPSimpleVarList(Op
ParseOptionalCXXScopeSpecifier(SS, ParsedType(), false)) {
IsCorrect = false;
SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
- StopBeforeMatch);
+ StopBeforeMatch | NoBracketsCount);
} else if (ParseUnqualifiedId(SS, false, false, false, ParsedType(),
TemplateKWLoc, Name)) {
IsCorrect = false;
SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
- StopBeforeMatch);
+ StopBeforeMatch | NoBracketsCount);
} else if (Tok.isNot(tok::comma) && Tok.isNot(tok::r_paren) &&
Tok.isNot(tok::annot_pragma_openmp_end)) {
IsCorrect = false;
SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
- StopBeforeMatch);
+ StopBeforeMatch | NoBracketsCount);
Diag(PrevTok.getLocation(), diag::err_expected_ident)
<< SourceRange(PrevTok.getLocation(), PrevTokLocation);
} else {
@@ -287,13 +288,13 @@ OMPClause *Parser::ParseOpenMPClause(Ope
case OMPC_unknown:
Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
<< getOpenMPDirectiveName(DKind);
- SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);

+ SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);

break;
case OMPC_threadprivate:
case NUM_OPENMP_CLAUSES:
Diag(Tok, diag::err_omp_unexpected_clause)
<< getOpenMPClauseName(CKind) << getOpenMPDirectiveName(DKind);

- SkipUntil(tok::comma, tok::annot_pragma_openmp_end, StopBeforeMatch); + SkipUntil(tok::comma, tok::annot_pragma_openmp_end, StopBeforeMatch | NoBracketsCount);

break;
}
return ErrorFound ? 0 : Clause;
@@ -308,7 +309,8 @@ OMPClause *Parser::ParseOpenMPSimpleClau
SourceLocation Loc = Tok.getLocation();
SourceLocation LOpen = ConsumeToken();
// Parse '('.

- BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end); + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,

+ true);
if (T.expectAndConsume(diag::err_expected_lparen_after,
getOpenMPClauseName(Kind)))
return 0;
@@ -342,7 +344,8 @@ OMPClause *Parser::ParseOpenMPVarListCla
SourceLocation Loc = Tok.getLocation();
SourceLocation LOpen = ConsumeToken();
// Parse '('.

- BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end); + BalancedDelimiterTracker T(*this, tok::l_paren, tok::annot_pragma_openmp_end,

+ true);
if (T.expectAndConsume(diag::err_expected_lparen_after,
getOpenMPClauseName(Kind)))
return 0;
@@ -357,7 +360,7 @@ OMPClause *Parser::ParseOpenMPVarListCla
Vars.push_back(VarExpr.take());
} else {
SkipUntil(tok::comma, tok::r_paren, tok::annot_pragma_openmp_end,
- StopBeforeMatch);
+ StopBeforeMatch | NoBracketsCount);
}
// Skip ',' if any
IsComma = Tok.is(tok::comma);

Modified: cfe/trunk/lib/Parse/Parser.cpp

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Parser.cpp?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/lib/Parse/Parser.cpp (original)
+++ cfe/trunk/lib/Parse/Parser.cpp Wed Dec 18 02:46:25 2013
@@ -159,7 +159,8 @@ static bool IsCommonTypo(tok::TokenKind

/// SkipToTok is specified, it calls SkipUntil(SkipToTok). Finally, true is

/// returned.

bool Parser::ExpectAndConsume(tok::TokenKind ExpectedTok, unsigned DiagID,

- const char *Msg, tok::TokenKind SkipToTok) {
+ const char *Msg, tok::TokenKind SkipToTok,
+ bool NoCount) {
if (Tok.is(ExpectedTok) || Tok.is(tok::code_completion)) {
ConsumeAnyToken();
return false;
@@ -189,8 +190,12 @@ bool Parser::ExpectAndConsume(tok::Token
} else
Diag(Tok, DiagID) << Msg;
- if (SkipToTok != tok::unknown)
- SkipUntil(SkipToTok, StopAtSemi);
+ if (SkipToTok != tok::unknown) {
+ SkipUntilFlags Flags = StopAtSemi;
+ if (NoCount)
+ Flags = Flags | NoBracketsCount;
+ SkipUntil(SkipToTok, Flags);
+ }
return true;
}
@@ -314,26 +319,32 @@ bool Parser::SkipUntil(ArrayRef<tok::Tok
case tok::l_paren:
// Recursively skip properly-nested parens.
ConsumeParen();
- if (HasFlagsSet(Flags, StopAtCodeCompletion))
- SkipUntil(tok::r_paren, StopAtCodeCompletion);
- else
- SkipUntil(tok::r_paren);
+ if (!HasFlagsSet(Flags, NoBracketsCount)) {
+ if (HasFlagsSet(Flags, StopAtCodeCompletion))
+ SkipUntil(tok::r_paren, StopAtCodeCompletion);
+ else
+ SkipUntil(tok::r_paren);
+ }
break;
case tok::l_square:
// Recursively skip properly-nested square brackets.
ConsumeBracket();
- if (HasFlagsSet(Flags, StopAtCodeCompletion))
- SkipUntil(tok::r_square, StopAtCodeCompletion);
- else
- SkipUntil(tok::r_square);
+ if (!HasFlagsSet(Flags, NoBracketsCount)) {
+ if (HasFlagsSet(Flags, StopAtCodeCompletion))
+ SkipUntil(tok::r_square, StopAtCodeCompletion);
+ else
+ SkipUntil(tok::r_square);
+ }
break;
case tok::l_brace:
// Recursively skip properly-nested braces.
ConsumeBrace();
- if (HasFlagsSet(Flags, StopAtCodeCompletion))
- SkipUntil(tok::r_brace, StopAtCodeCompletion);
- else
- SkipUntil(tok::r_brace);
+ if (!HasFlagsSet(Flags, NoBracketsCount)) {
+ if (HasFlagsSet(Flags, StopAtCodeCompletion))
+ SkipUntil(tok::r_brace, StopAtCodeCompletion);
+ else
+ SkipUntil(tok::r_brace);
+ }
break;

// Okay, we found a ']' or '}' or ')', which we think should be balanced.

@@ -342,17 +353,17 @@ bool Parser::SkipUntil(ArrayRef<tok::Tok

// higher level, we will assume that this matches the unbalanced token // and return it. Otherwise, this is a spurious RHS token, which we skip.

case tok::r_paren:
- if (ParenCount && !isFirstTokenSkipped)

+ if (!HasFlagsSet(Flags, NoBracketsCount) && ParenCount && !isFirstTokenSkipped)

return false; // Matches something.
ConsumeParen();
break;
case tok::r_square:
- if (BracketCount && !isFirstTokenSkipped)

+ if (!HasFlagsSet(Flags, NoBracketsCount) && BracketCount && !isFirstTokenSkipped)

return false; // Matches something.
ConsumeBracket();
break;
case tok::r_brace:
- if (BraceCount && !isFirstTokenSkipped)

+ if (!HasFlagsSet(Flags, NoBracketsCount) && BraceCount && !isFirstTokenSkipped)

return false; // Matches something.
ConsumeBrace();
break;
@@ -2031,7 +2042,7 @@ bool BalancedDelimiterTracker::expectAnd
const char *Msg,
tok::TokenKind SkipToToc ) {
LOpen = P.Tok.getLocation();
- if (P.ExpectAndConsume(Kind, DiagID, Msg, SkipToToc))
+ if (P.ExpectAndConsume(Kind, DiagID, Msg, SkipToToc, NoCount))
return true;
if (getDepth() < MaxDepth)
@@ -2056,16 +2067,21 @@ bool BalancedDelimiterTracker::diagnoseM

// If we're not already at some kind of closing bracket, skip to our closing

// token.

+ Parser::SkipUntilFlags Flags = Parser::StopAtSemi | Parser::StopBeforeMatch;

+ if (NoCount)
+ Flags = Flags | Parser::NoBracketsCount;
if (P.Tok.isNot(tok::r_paren) && P.Tok.isNot(tok::r_brace) &&
P.Tok.isNot(tok::r_square) &&
- P.SkipUntil(Close, FinalToken,
- Parser::StopAtSemi | Parser::StopBeforeMatch) &&
+ P.SkipUntil(Close, FinalToken, Flags) &&
P.Tok.is(Close))
LClose = P.ConsumeAnyToken();
return true;
}
void BalancedDelimiterTracker::skipToEnd() {
- P.SkipUntil(Close, Parser::StopBeforeMatch);
+ Parser::SkipUntilFlags Flags = Parser::StopBeforeMatch;
+ if (NoCount)
+ Flags = Flags | Parser::NoBracketsCount;
+ P.SkipUntil(Close, Flags);
consumeClose();
}

Modified: cfe/trunk/lib/Parse/RAIIObjectsForParser.h

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/RAIIObjectsForParser.h?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/lib/Parse/RAIIObjectsForParser.h (original)

+++ cfe/trunk/lib/Parse/RAIIObjectsForParser.h Wed Dec 18 02:46:25 2013

@@ -361,6 +361,7 @@ namespace clang {
tok::TokenKind Kind, Close, FinalToken;
SourceLocation (Parser::*Consumer)();
SourceLocation LOpen, LClose;
+ bool NoCount;
unsigned short &getDepth() {
switch (Kind) {
@@ -378,9 +379,10 @@ namespace clang {
public:
BalancedDelimiterTracker(Parser& p, tok::TokenKind k,
- tok::TokenKind FinalToken = tok::semi)
+ tok::TokenKind FinalToken = tok::semi,
+ bool NoCount = false)
: GreaterThanIsOperatorScope(p.GreaterThanIsOperator, true),
- P(p), Kind(k), FinalToken(FinalToken)
+ P(p), Kind(k), FinalToken(FinalToken), NoCount(NoCount)
{
switch (Kind) {
default: llvm_unreachable("Unexpected balanced token");

Modified: cfe/trunk/test/OpenMP/parallel_messages.cpp

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/parallel_messages.cpp?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/test/OpenMP/parallel_messages.cpp (original)

+++ cfe/trunk/test/OpenMP/parallel_messages.cpp Wed Dec 18 02:46:25 2013

@@ -6,6 +6,18 @@ void foo() {

#pragma omp parallel // expected-error {{unexpected OpenMP directive '#pragma omp parallel'}}

int main(int argc, char **argv) {

+ #pragma omp parallel { // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();

+ #pragma omp parallel ( // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();

+ #pragma omp parallel [ // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();

+ #pragma omp parallel ] // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();

+ #pragma omp parallel ) // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();

+ #pragma omp parallel } // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

+ foo();
#pragma omp parallel

#pragma omp parallel unknown() // expected-warning {{extra tokens at the end of '#pragma omp parallel' are ignored}}

foo();

Modified: cfe/trunk/test/OpenMP/threadprivate_messages.cpp

URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/threadprivate_messages.cpp?rev=197553&r1=197552&r2=197553&view=diff ==============================================================================

--- cfe/trunk/test/OpenMP/threadprivate_messages.cpp (original)

+++ cfe/trunk/test/OpenMP/threadprivate_messages.cpp Wed Dec 18 02:46:25 2013

@@ -24,6 +24,12 @@ int foo() { // expected-note {{declared
return (a);
}

+#pragma omp threadprivate (a) ( // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} +#pragma omp threadprivate (a) [ // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} +#pragma omp threadprivate (a) { // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} +#pragma omp threadprivate (a) ) // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} +#pragma omp threadprivate (a) ] // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} +#pragma omp threadprivate (a) } // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'a'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}} #pragma omp threadprivate a // expected-error {{expected '(' after 'threadprivate'}} #pragma omp threadprivate(d // expected-error {{expected ')'}} expected-note {{to match this '('}} expected-error {{'#pragma omp threadprivate' must precede all references to variable 'd'}} #pragma omp threadprivate(d)) // expected-error {{'#pragma omp threadprivate' must precede all references to variable 'd'}} expected-warning {{extra tokens at the end of '#pragma omp threadprivate' are ignored}}

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits