Misleading parsing error message for OpenMP directives

[skatrak]

Currently, the expected behavior when encountering an invalid OpenMP directive is that the parser emits an "error: expected OpenMP construct". For example:

$ cat test.f90
subroutine foo
  integer :: x = 10
  !$omp invalid_directive
  ! ...
end subroutine foo

$ flang-new -fc1 -fopenmp -fsyntax-only test.f90
error: Could not parse test.f90
./test.f90:3:10: error: expected OpenMP construct
    !$omp invalid_directive
           ^
./test.f90:3:3: in the context: specification construct
...

However, if there is an assignment operation preceding the invalid OpenMP directive, the error message becomes something else without any mention of the possibility that there is an invalid OpenMP directive. For example:

$ cat test.f90
subroutine foo
  integer :: x
  x = 10
  !$omp invalid_directive
  ! ...
end subroutine foo

$ flang-new -fc1 -fopenmp -fsyntax-only test.f90
error: Could not parse test.f90
./test.f90:4:9: error: expected 'END'
    !$omp invalid_directive
          ^
./test.f90:4:3: in the context: execution part construct
...

This makes identifying problems related to typos or to trying to use unsupported OpenMP compound constructs hard to troubleshoot on large applications.

[llvmbot]

@llvm/issue-subscribers-flang-frontend

Author: Sergio Afonso (skatrak)

Currently, the expected behavior when encountering an invalid OpenMP directive is that the parser emits an "error: expected OpenMP construct". For example: ``` $ cat test.f90 subroutine foo integer :: x = 10 !$omp invalid_directive ! ... end subroutine foo

$ flang-new -fc1 -fopenmp -fsyntax-only test.f90
error: Could not parse test.f90
./test.f90:3:10: error: expected OpenMP construct
!$omp invalid_directive
^
./test.f90:3:3: in the context: specification construct
...

However, if there is an assignment operation preceding the invalid OpenMP directive, the error message becomes something else without any mention of the possibility that there is an invalid OpenMP directive. For example:

$ cat test.f90
subroutine foo
integer :: x
x = 10
!$omp invalid_directive
! ...
end subroutine foo

$ flang-new -fc1 -fopenmp -fsyntax-only test.f90
error: Could not parse test.f90
./test.f90:4:9: error: expected 'END'
!$omp invalid_directive
^
./test.f90:4:3: in the context: execution part construct
...

This makes identifying problems related to typos or to trying to use unsupported OpenMP compound constructs hard to troubleshoot on large applications.
</details>

[kiranchandramohan]

If an executable construct (like assignment) precedes the !$omp token then it attempts to pass the OpenMP construct as an executable OpenMP construct. There are two options for this
-> The end loop directive

llvm-project/flang/lib/Parser/executable-parsers.cpp

Line 52 in bc627a4

construct<ExecutableConstruct>(indirect(ompEndLoopDirective)),

-> The OpenMP construct

You are seeing the error message (error: expected 'END') for the end loop directive.

Note that the parser always parses the end loop directive as a separate construct and it corrects this by adding the end loop directive to the previous construct during early Semantics. I think this is due to similar steps in the parsing of some special cases of loops.

This does not happen for the declarative case, since declarative sections cannot contain loops and only contains OpenMP Declarative constructs (

llvm-project/flang/lib/Parser/program-parsers.cpp

Line 144 in b856952

many(openmpDeclarativeConstruct), many(indirect(compilerDirective)),

).

Adding a message to the OpenMP End loop directive results in the following message for your code. If this is better I can make a patch.

error: Could not parse test.f90
./test.f90:5:10: error: expected OpenMP construct
    !$omp invalid_directive
           ^
./test.f90:5:3: in the context: OpenMP construct
    !$omp invalid_directive
    ^
./test.f90:5:3: in the context: execution part construct
    !$omp invalid_directive
    ^
./test.f90:4:3: in the context: execution part
    x = 10
    ^
./test.f90:2:1: in the context: SUBROUTINE subprogram
  subroutine foo
  ^
./test.f90:5:10: error: expected OpenMP End Loop directive
    !$omp invalid_directive
           ^
./test.f90:5:3: in the context: execution part construct
    !$omp invalid_directive
    ^
./test.f90:4:3: in the context: execution part
    x = 10
    ^
./test.f90:2:1: in the context: SUBROUTINE subprogram
  subroutine foo

[skatrak]

Thank you @kiranchandramohan for investigating this. I do believe that the alternative error message you're describing would be a nice improvement over what we have, at least for this case.

I think replacing the "error: expected 'END'" message by a more descriptive "error: expected OpenMP End Loop directive" or similar message helps on all cases when that's the actual issue. But, in this case, having the "error: expected OpenMP construct" message come before it rather than after is what actually makes the difference, in my opinion.

It seems more likely that a misspelt directive appearing in the executable section would refer to some unrelated construct rather than an !$omp end <loop-construct>, especially if the error message is not based on whether we're parsing right after a do-loop. So, it makes sense that it wouldn't be presented as the first option to users.