openmp pragmas break clangd behavior (e.g. go-to-def, rename) on symbols inside parallel loop

[aerosayan]

When using OpenMP #pragmas, clangd's jump to definition, rename, doesn't work for variables or functions.

I found this error while using clangd as a C++ LSP in neovim editor.

Here's a proof of concept C++ code showing the error ...

    int j = 88, k = 99;
    #pragma omp parallel for
    for(int i = 0; i < 10; i++)
    {
        printf("%d\n", k); // ERROR: Does not jump to k or rename symbol k
    }
    printf("%d\n", j); // CORRECT: Does jump to j and rename symbol j

Clangd breaks down inside the for loop. I can't jump to k, or rename it. I can't also jump to the definition of function printf inside the loop.

But outside the loop, I can jump to definition of j and also rename it. Also, I can jump to definition of function printf.

The error is clearly due to the pragma, as if I comment the pragma line out, then everything works correctly again for both k, j, and printf.

Logs

I don't know how to get the clangd logs in neovim right now.

System information

I installed clangd in neovim using Mason, and the clangd version is: 15.0.6

Editor/LSP plugin: Neovim v0.9.0-dev

NVIM v0.9.0-dev
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3
Compilation: /usr/bin/cc -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wdouble-promotion -Wmissing-noreturn -Wmissing-format-attribute -Wmissing-prototypes -Wno-unused-result -Wimplicit-fallthrough -Wvla -fno-common -fdiagnostics-color=auto -fstack-protector-strong -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DMIN_LOG_LEVEL=3 -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_TS_HAS_SET_MATCH_LIMIT -DNVIM_TS_HAS_SET_ALLOCATOR -DNVIM_UNIBI_HAS_VAR_FROM -I/usr/include/luajit-2.1 -I/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/build/src/nvim/auto -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/build/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/build/cmake.config -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/src -I/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include -I/build/neovim-kqgxhc/neovim-0.9.0~ubuntu1+git202303031604-6d4f48182-333b5866f/.deps/usr/include

Operating system: Linux. Ubuntu 20.04.1

[HighCommander4]

I cannot reproduce this in vscode.

I tried the following

• Created a file named main.cpp, with the content:

  #include <stdio.h>
  
  int main() {
     int j = 88, k = 99;
     #pragma omp parallel for
     for(int i = 0; i < 10; i++)
     {
         printf("%d\n", k); // ERROR: Does not jump to k or rename symbol k
     }
     printf("%d\n", j); // CORRECT: Does jump to j and rename symbol j  
  }

• Go-to-def on the use of k insisde the loop succeeded
• Renaming k from the use inside the loop succeeded

I tried with both clandg 15 and a recent trunk, with the same results.

I also tried in a C file (main.c) since your command included -std=gnu99, same results there as well.

[aerosayan]

@HighCommander4 thanks for your quick feedback.

I guess it could possibly be a bug in neovim.

I will try to ask some other neovim users to test it, and see if they also face the same bug.

[aerosayan]

@HighCommander4 I found why you couldn't find the bug: OpenMP pragmas are only activated conditionally if we use the correct compiler flags and include correct header files.

And I guess they weren't activated because you don't have a cmake file or inlcude the omp.h header.

Sorry, I missed that earlier, or else I would've provided you the full cmake file.

Here, this produces the bug for me:

test.cc

#include <stdio.h>
#include <omp.h>

int main()
{
    int j = 88, k = 99;
    #pragma omp parallel for
    for(int i = 0; i < 10; i++)
    {
        printf("%d\n", k); // ERROR: Does not jump to k or rename symbol k
    }
    printf("%d\n", j); // CORRECT: Does jump to j and rename symbol j
    return 0;
}

CMakeLists.txt

cmake_minimum_required(VERSION 3.15)
project(ocx LANGUAGES C CXX)

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED True)
set(CMAKE_CXX_EXTENSIONS False)

# Find the necessary packages
find_package(OpenMP REQUIRED)

# Compile options
add_compile_options(-g3)
add_compile_options(-O0)
add_compile_options(-march=native)
add_compile_options(-pedantic-errors)

# Link options
add_link_options(-g3)
add_compile_options(-O0)

# Compile and link executables
add_executable(omp test.cc)

# Link to libraries
target_link_libraries(omp OpenMP::OpenMP_CXX)

Next we need to use -DCMAKE_EXPORT_COMPILE_COMMANDS=1 option in cmake to build compile_commands.json file.

$ mkdir build
$ cd build
$ cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ..

This creates the compile_commands.json file.

Now, if I reopen neovim, the bug is observable as described in my original post.

But if I delete compile_commands.json file, exit neovim, and reopen the test.cc file again in neovim, the bug disappears, and everything works correctly.

Conclusion: It's the cmake generated compile_commands.json file that allows the bug to be visible. But I can not say that it's causing the bug. Most likely the bug is in neovim or clangd, but I can not say exactly where.

[HighCommander4]

Would you mind showing the entry for test.cc from compile_commands.json itself? (This just makes it a bit easier for me to reproduce, as then I can use the flags from it directly instead of having to get cmake to run successfully.)

[aerosayan]

Sure! Here's the file content ...

[
{
  "directory": "/home/ares/Desktop/omp/build",
  "command": "/usr/bin/c++      -g3 -O0 -march=native -pedantic-errors -fopenmp -std=c++14 -o CMakeFiles/omp.dir/test.cc.o -c /home/ares/Desktop/omp/test.cc",
  "file": "/home/ares/Desktop/omp/test.cc"
}
]

[HighCommander4]

Thanks. -fopenmp seems to be the important flag that affects clangd's behaviour here.

[HighCommander4]

Taking a simplified example:

int main() {
    int k = 0;
    #pragma omp parallel for
    for (int i = 0; i < 10; i++) {
      ++k;
    }
}

Here is the AST without -fopenmp:

TranslationUnitDecl 0x55a4c3a12b18 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x55a4c3a59ee0 <main.cpp:1:1, line:7:1> line:1:5 main 'int ()'
  `-CompoundStmt 0x55a4c3a5a2a8 <col:12, line:7:1>
    |-DeclStmt 0x55a4c3a5a0a0 <line:2:5, col:14>
    | `-VarDecl 0x55a4c3a5a018 <col:5, col:13> col:9 used k 'int' cinit
    |   `-IntegerLiteral 0x55a4c3a5a080 <col:13> 'int' 0
    `-ForStmt 0x55a4c3a5a270 <line:4:5, line:6:5>
      |-DeclStmt 0x55a4c3a5a158 <line:4:10, col:19>
      | `-VarDecl 0x55a4c3a5a0d0 <col:10, col:18> col:14 used i 'int' cinit
      |   `-IntegerLiteral 0x55a4c3a5a138 <col:18> 'int' 0
      |-<<<NULL>>>
      |-BinaryOperator 0x55a4c3a5a1c8 <col:21, col:25> 'bool' '<'
      | |-ImplicitCastExpr 0x55a4c3a5a1b0 <col:21> 'int' <LValueToRValue>
      | | `-DeclRefExpr 0x55a4c3a5a170 <col:21> 'int' lvalue Var 0x55a4c3a5a0d0 'i' 'int'
      | `-IntegerLiteral 0x55a4c3a5a190 <col:25> 'int' 10
      |-UnaryOperator 0x55a4c3a5a208 <col:29, col:30> 'int' postfix '++'
      | `-DeclRefExpr 0x55a4c3a5a1e8 <col:29> 'int' lvalue Var 0x55a4c3a5a0d0 'i' 'int'
      `-CompoundStmt 0x55a4c3a5a258 <col:34, line:6:5>
        `-UnaryOperator 0x55a4c3a5a240 <line:5:7, col:9> 'int' lvalue prefix '++'
          `-DeclRefExpr 0x55a4c3a5a220 <col:9> 'int' lvalue Var 0x55a4c3a5a018 'k' 'int'

Here is the AST with -fopenmp:

TranslationUnitDecl 0x5650960abba8 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x5650960f2820 <main.cpp:1:1, line:7:1> line:1:5 main 'int ()'
  `-CompoundStmt 0x56509610d350 <col:12, line:7:1>
    |-DeclStmt 0x5650960f29e0 <line:2:5, col:14>
    | `-VarDecl 0x5650960f2958 <col:5, col:13> col:9 used k 'int' cinit
    |   `-IntegerLiteral 0x5650960f29c0 <col:13> 'int' 0
    `-OMPParallelForDirective 0x56509610d248 <line:3:5, col:29>
      `-CapturedStmt 0x5650960f3008 <line:4:5, line:6:5>
        |-CapturedDecl 0x5650960f2b70 <<invalid sloc>> <invalid sloc> nothrow
        | |-ForStmt 0x5650960f2f48 <line:4:5, line:6:5>
        | | |-DeclStmt 0x5650960f2de0 <line:4:10, col:19>
        | | | `-VarDecl 0x5650960f2d58 <col:10, col:18> col:14 used i 'int' cinit
        | | |   `-IntegerLiteral 0x5650960f2dc0 <col:18> 'int' 0
        | | |-<<<NULL>>>
        | | |-BinaryOperator 0x5650960f2e50 <col:21, col:25> 'bool' '<'
        | | | |-ImplicitCastExpr 0x5650960f2e38 <col:21> 'int' <LValueToRValue>
        | | | | `-DeclRefExpr 0x5650960f2df8 <col:21> 'int' lvalue Var 0x5650960f2d58 'i' 'int'
        | | | `-IntegerLiteral 0x5650960f2e18 <col:25> 'int' 10
        | | |-UnaryOperator 0x5650960f2e90 <col:29, col:30> 'int' postfix '++'
        | | | `-DeclRefExpr 0x5650960f2e70 <col:29> 'int' lvalue Var 0x5650960f2d58 'i' 'int'
        | | `-CompoundStmt 0x5650960f2f30 <col:34, line:6:5>
        | |   `-UnaryOperator 0x5650960f2f18 <line:5:7, col:9> 'int' lvalue prefix '++'
        | |     `-DeclRefExpr 0x5650960f2ef8 <col:9> 'int' lvalue Var 0x5650960f2958 'k' 'int'
        | |-ImplicitParamDecl 0x5650960f2be0 <line:3:5> col:5 implicit .global_tid. 'const int *const __restrict'
        | |-ImplicitParamDecl 0x5650960f2c48 <col:5> col:5 implicit .bound_tid. 'const int *const __restrict'
        | |-ImplicitParamDecl 0x5650960f2cd8 <col:5> col:5 implicit __context '(unnamed struct at main.cpp:3:5) *const __restrict'
        | `-VarDecl 0x5650960f2d58 <line:4:10, col:18> col:14 used i 'int' cinit
        |   `-IntegerLiteral 0x5650960f2dc0 <col:18> 'int' 0
        `-DeclRefExpr 0x5650960f2f80 <line:5:9> 'int' lvalue Var 0x5650960f2958 'k' 'int'

[HighCommander4]

I agree that there is a clangd bug here, it doesn't seem to handle the constructs in the second AST (e.g. OMPParallelForDirective properly).

That said, I'm curious if simply getting clangd to parse the code without -fopenmp is a viable workaround until the bug is fixed. For example, you could create a .clangd file in the project root containing:

CompileFlags:
  Remove: [-fopenmp]

I would expect this to fix the reported problems, though I'm not sure if it would cause anything else to break. Perhaps you can try it and let us know what you find.

[sam-mccall]

I'm not sure this is a bug, just because I don't think there was ever an attempt to support OpenMP, just c++/c/ObjC.

The syntax doesn't follow the principles of C-family languages, and the way the pragmas create AST nodes really stretches the idea of an "AST" to breaking point.

Concretely here, I'm guessing the problem is that OMPParallelForDirective doesn't know its own bounds (doesn't contain its children).

I'm not sure this is worth fixing, it seems like trying to support badly-behaved AST nodes for a niche dialect may end up incurring a lot of hacks. OTOH if SelectionTree can be cheaply repaired to handle this node maybe that gets us 90% there and we can accept glitches elsewhere.

[aerosayan]

I have a suggestion that might help make supporting OpenMP code easier.

In Summary: Disable the -fopenmp compiler flag while generating the AST in clangd, and use the generated AST for single threaded serial C/C++ code to support the required LSP features

It's arguably a hack that may or may not work, but I think it's feasible.

Essentially, OpenMP code is standard C/C++ code, but the difference in AST is produced due to OpenMP adding new constructs to run threads, create thread local variables, create mutexs, etc.

Understandably, supporting such a complicated AST will be difficult for clangd team.

So, what if we don't even generate the complicated AST with -fopenmp. Just disable the compiler flag, and the AST that will be generated, will be of a single threaded C/C++ code.

And single threaded C/C++ code is already supported by clangd. So, the go0to and symbol-rename features of the LSP will keep working without issue.

This might be the correct approach because almost all OpenMP code also compiles and runs as a single threaded code if we disable the -fopenmp flag during compilation. C/C++ uses #pragmas to define OpenMP directives, but other languages like fortran uses comments such as ! OMP PARALLEL FOR. What I mean to say is, OpenMP directives are treated as non-standard even by the compilers, and which can be optionally compiled when necessary, but the code should run correctly even if OpenMP is not used. Think of it as: A weather simulation code written using OpenMP should also run as a single threaded code when we disable OpenMP.

So, the AST generated by disabling -fopenmp is obviously valid for single threaded code, and you can take advantage of that to support C/C++ code that uses OpenMP.

[HighCommander4]

I have a suggestion that might help make supporting OpenMP code easier.

In Summary: Disable the -fopenmp compiler flag while generating the AST in clangd

That's exactly what my suggestion of adding the following to the clangd config file accomplishes:

CompileFlags:
  Remove: [-fopenmp]

The only remaining question is whether perhaps clangd should do this automatically. I don't have a strong opinion on that.

[sam-mccall]

@aerosayan it would be great if you could try this out and give feedback on what does/doesn't work.

If it feels significantly better in practice, I'd vote for baking this in until someone finds the time/energy/ideas to add real OpenMP support. OTOH if it does confusing things like generate spurious diagnostics then we're probably better off just documenting this as a workaround for people to apply themselves.

[aerosayan]

Reporting preliminary test results.

Tested App: GMSH: https://gitlab.onelab.info/gmsh/gmsh Tested File: src/mesh/meshQuadQuasiStructured.cpp

1. Primary Result: clangd go-to-def, rename, works very well.
2. We get some warning for unused variable nthreads on line 580, but that's okay. Here in the code shown below, the nthreads is used to set the number of threads using openmp num_threads( ) command, so it's not a problem if the warning is shown.

   int nthreads = getNumThreads();
#pragma omp parallel for schedule(dynamic) num_threads(nthreads)
    for(size_t f = 0; f < faces.size(); ++f) {
      GFace *gf = faces[f];

      if(CTX::instance()->mesh.meshOnlyVisible && !gf->getVisibility())
        continue;
      if(CTX::instance()->debugSurface > 0 &&
         gf->tag() != CTX::instance()->debugSurface)
        continue;

I did a recursive grep for all openmp pragmas in the project, and I'm showing only some of the interesting results. I will test these files later, but if there's any error, or diagnostic warnings, I would guess these complex cases below might cause them, but if they don't cause bugs or warnings, then most other openmp codes in the wild will also be okay, because no one writes too much complex code with openmp.

$ grep -ri "pragma omp" . 
./src/geo/GModel.h:#pragma omp atomic write
./src/geo/GModel.h:#pragma omp atomic capture
./src/geo/GModel.h:#pragma omp atomic update
./src/geo/GModelIO_MSH4.cpp:#pragma omp parallel for num_threads(nthreads)
./src/geo/GModelVertexArrays.cpp:#pragma omp parallel for schedule(dynamic) num_threads(nthreads)
./src/mesh/Generator.cpp:#pragma omp parallel for schedule(dynamic) num_threads(nthreads)
./src/mesh/meshQuadQuasiStructured.cpp:#pragma omp parallel for schedule(dynamic) num_threads(nthreads)
./src/mesh/meshGRegionHxt.cpp:    #pragma omp parallel num_threads(nthreads)
./src/common/gmsh.cpp:#pragma omp parallel for num_threads(nthreads)
./src/common/gmsh.cpp:#pragma omp critical
./src/common/GmshMessage.cpp:#pragma omp critical(MsgError)
./src/common/GmshMessage.cpp:#pragma omp critical(MsgError)
./src/common/GmshMessage.cpp:#pragma omp critical(MsgWarning)
./src/numeric/BasisFactory.cpp:#pragma omp critical
./contrib/eigen/Eigen/src/SparseCore/SparseDenseProduct.h:        #pragma omp parallel for schedule(dynamic,(n+threads*4-1)/(threads*4)) num_threads(threads)
./contrib/eigen/Eigen/src/Core/products/GeneralMatrixMatrix.h:        #pragma omp atomic
./contrib/eigen/Eigen/src/Core/products/Parallelizer.h:  #pragma omp parallel num_threads(threads)
./contrib/MeshOptimizer/MeshOptimizer.cpp:#pragma omp parallel for schedule(dynamic) num_threads(nthreads)
./contrib/hxt/core/src/hxt_bbox.c:        #pragma omp parallel for reduction(min:minx,miny,minz) reduction(max:maxx, maxy, maxz)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:  #pragma omp simd aligned(verticesID,bnd:SIMD_ALIGN)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:  #pragma omp simd aligned(bnd:SIMD_ALIGN)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:  #pragma omp parallel reduction(+:mooreSkipped)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:    #pragma omp parallel for simd aligned(nodeInfo:SIMD_ALIGN)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:      #pragma omp parallel for simd aligned(vertCopy,sizeCopy,nodeInfo: SIMD_ALIGN)
./contrib/hxt/tetMesh/src/hxt_tetDelaunay.c:          #pragma omp parallel num_threads(reproducibleSearchThreads)
./contrib/hxt/tetMesh/src/hxt_tetOpti.c:    #pragma omp parallel for simd aligned(badTets:SIMD_ALIGN)
./contrib/hxt/tetMesh/src/hxt_tetOpti.c:    #pragma omp parallel num_threads(shared->numThreads)
./contrib/hxt/tetMesh/src/hxt_tetOpti.c:  #pragma omp parallel for reduction(+: good, bad, sum) reduction(min:min)
./contrib/hxt/tetMesh/src/hxt_tetOpti.c:    #pragma omp parallel num_threads(shared.numThreads)
./contrib/hxt/tetMesh/src/hxt_tetRefine.c:  #pragma omp parallel for simd aligned(nodeInfo:SIMD_ALIGN)
./contrib/metis/GKlib/csr.c:      #pragma omp parallel private(i, j, ncand, rsum, tsum, cand)
./contrib/metis/GKlib/csr.c:      #pragma omp parallel private(i, j, ncand, rsum, tsum, cand)
./contrib/metis/GKlib/csr.c:        #pragma omp for schedule(static)
./contrib/metis/GKlib/csr.c:  #pragma omp parallel if (n > 100)
./contrib/metis/GKlib/csr.c:      #pragma omp parallel if (ncols > OMPMINOPS) 
./contrib/metis/GKlib/csr.c:        #pragma omp for schedule(static) reduction(+:nnzcols)
./contrib/metis/GKlib/csr.c:        #pragma omp for schedule(static)
./contrib/metis/GKlib/csr.c:      #pragma omp parallel if (rowptr[nrows] > OMPMINOPS) 
./contrib/metis/GKlib/csr.c:        #pragma omp for private(j) schedule(static)
./contrib/metis/GKlib/csr.c:  #pragma omp parallel for if (ptr[n] > OMPMINOPS) schedule(static)

[aerosayan]

Everything seems to work okay when we disable -fopenmp. We only get few warnings for unused variables.

[nextsilicon-itay-bookstein]

For posterity, sometimes the flag you need to remove (e.g. via .clangd) is spelled -fopenmp=libomp and not just plain -fopenmp.