[OpenMP] crash on invalid with metadirective

[k-arrows]

Reproducer:
https://godbolt.org/z/aGxv1PG6d

void f(){
#pragma omp metadirective(
}

Essentially the same as #139360, but not yet fixed

[llvmbot]

@llvm/issue-subscribers-bug

Author: None (k-arrows)

Reproducer: https://godbolt.org/z/aGxv1PG6d ```cpp void f(){ #pragma omp metadirective( } ```

Essentially the same as #139360, but not yet fixed

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (k-arrows)

Reproducer: https://godbolt.org/z/aGxv1PG6d ```cpp void f(){ #pragma omp metadirective( } ```

Essentially the same as #139360, but not yet fixed

[AaronBallman]

I'm not certain of the best way to fix this because the existing parsing code looks suspect in a few ways:

llvm-project/clang/lib/Parse/ParseOpenMP.cpp

Line 2772 in 9b63bdd

case OMPD_metadirective: {

We have a tentative parsing action involved in this parsing loop.

Sometimes, we issue an error and continue parsing. This seems like the root cause of our problem in this issue:

llvm-project/clang/lib/Parse/ParseOpenMP.cpp

Line 2790 in 9b63bdd

Diag(Tok, diag::err_omp_expected_clause) << "metadirective";

Sometimes, we issue an error and return the directive without touching the tentative parsing action:

llvm-project/clang/lib/Parse/ParseOpenMP.cpp

Line 2800 in 9b63bdd

if (T.expectAndConsume(diag::err_expected_lparen_after,

This will cause a different assertion that should also be fixed: https://godbolt.org/z/YY3f59cjv

Sometimes, we issue and error and commit to the parse instead of reverting the failed parse:

llvm-project/clang/lib/Parse/ParseOpenMP.cpp

Line 2808 in 9b63bdd

if (TI.Sets.size() == 0) {

When I try to fix this up by no longer continuing to parse after issuing the error, we get very different diagnostic output in test/OpenMP/metadirective_messages.cpp and perhaps that means my changes are incorrect. It could also mean follow-on parsing also needs to be fixed, though.

I think an OpenMP expert may need to come by to repair this one fully. CC @alexey-bataev @jhuber6 @shiltian

[alexey-bataev]

Assume, the correct would be to emit the error in line 2790, skip to the end of the line and exit

[AaronBallman]

So something like:

      if (CKind == OMPC_unknown) {
        Diag(Tok, diag::err_omp_expected_clause) << "metadirective";
        TPA.Revert();
        SkipUntil(tok::annot_pragma_openmp_end);
        return StmtError();
      }

?

And should we do that for the other places which emit an error? If so, I get:

# | error: 'expected-warning' diagnostics expected but not seen:
# |   File F:\source\llvm-project\clang\test\OpenMP\metadirective_messages.cpp Line 46: isa trait 'some-unsupported-feature' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further

from metadirective_messages.cpp which seems like a regression?

[alexey-bataev]

So something like:

      if (CKind == OMPC_unknown) {
        Diag(Tok, diag::err_omp_expected_clause) << "metadirective";
        TPA.Revert();
        SkipUntil(tok::annot_pragma_openmp_end);
        return StmtError();
      }

?

And should we do that for the other places which emit an error? If so, I get:

# | error: 'expected-warning' diagnostics expected but not seen:
# |   File F:\source\llvm-project\clang\test\OpenMP\metadirective_messages.cpp Line 46: isa trait 'some-unsupported-feature' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further

from metadirective_messages.cpp which seems like a regression?

Try to return Directive instead of StmtError