Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows (Shared): omp_locks failed Symbol #3795

Open
ax3l opened this issue Mar 11, 2024 · 7 comments
Open

Windows (Shared): omp_locks failed Symbol #3795

ax3l opened this issue Mar 11, 2024 · 7 comments
Assignees
Labels

Comments

@ax3l
Copy link
Member

ax3l commented Mar 11, 2024

This is a Windows (shared/DLL) specific build regression to #3696.

Since this PR / the 24.02 release, we cannot build WarpX and ImpactX on Conda-Forge with a shared AMReX dependency anymore, due to this error:

[364/744] C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app_2d.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\app_2d.dir\Source\main.cpp.obj  /out:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe /implib:lib\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib /pdb:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  lib\libwarpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib  lib\ablastr_2d.lib  %PREFIX%\Library\lib\amrex_2d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\fftw3.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  lib\buildInfopyWarpX_2d.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe D:/bld/warpx_1706926672986/work/build/bin/warpx.2d && cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe D:/bld/warpx_1706926672986/work/build/bin/warpx.2d""
FAILED: bin/warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe 
C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app_2d.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\app_2d.dir\Source\main.cpp.obj  /out:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe /implib:lib\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib /pdb:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  lib\libwarpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib  lib\ablastr_2d.lib  %PREFIX%\Library\lib\amrex_2d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\fftw3.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  lib\buildInfopyWarpX_2d.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe D:/bld/warpx_1706926672986/work/build/bin/warpx.2d && cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe D:/bld/warpx_1706926672986/work/build/bin/warpx.2d""
LINK: command "%BUILD_PREFIX%\Library\bin\lld-link.exe CMakeFiles\app_2d.dir\Source\main.cpp.obj /out:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.exe /implib:lib\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib /pdb:bin\warpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console lib\libwarpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib lib\ablastr_2d.lib %PREFIX%\Library\lib\amrex_2d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\fftw3.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib lib\buildInfopyWarpX_2d.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=1" failed (exit code 1) with the following output:
lld-link: error: undefined symbol: __declspec(dllimport) class std::array<struct omp_lock_t, 128> amrex::OpenMP::omp_locks
>>> referenced by libwarpx.2d.NOMPI.OMP.DP.PDP.OPMD.PSATD.QED.lib(WarpXParticleContainer.cpp.obj):(public: class amrex::BaseFab<double> & __cdecl amrex::BaseFab<double>::lockAdd<1>(class amrex::BaseFab<double> const &, class amrex::Box const &, class amrex::Box const &, int, int, int))

It is not clear to me why this happens. I checked:

  • same compiler (clang-cl) & linker (lld-link) used for AMReX and application
  • same OpenMP implementation used for AMReX and application
  • same build more (Release) used for AMReX and application

Maybe we need to try a standard C array instead?

@ax3l ax3l added the bug label Mar 11, 2024
@ax3l
Copy link
Member Author

ax3l commented Mar 11, 2024

I think in practice, omp_lock_t is always used as a pointer omp_lock_t*, with not much details what the type implementation is used for.

ChatGPT has this opinion:

One problem might be that omp_lock_t are not guaranteed to be copyable or movable, which are usually requirements for types to be stored in standard C++ containers like std::array, std::vector, etc. Like a raw C array, std::array requires its elements to be default constructible, copyable, and assignable because it relies on these operations for initialization and element access.
[...] Since omp_lock_t lacks these operations (as duplicating a lock could lead to undefined behavior), it cannot be directly stored in a std::array.

It suggests to use a std::vector<std::unique_ptr<omp_lock_t>> omp_locks or omp_lock_t omp_locks[nlocks];.

@ax3l ax3l mentioned this issue Mar 11, 2024
5 tasks
WeiqunZhang pushed a commit that referenced this issue Mar 11, 2024
## Summary

Use a plain C array over a `std::array` for `omp_locks`. Primarily
because this causes linker issues on MSVC/Clang-Cl, secondarily because
`omp_locks` might violate in some implementations the type requirements
of `std::array` (MoveConstructible and MoveAssignable type `T`).
https://en.cppreference.com/w/cpp/container/array

## Additional background

Potentially a fix for #3795

Testing in conda-forge/impactx-feedstock#28

## Checklist

The proposed changes:
- [x] fix a bug or incorrect behavior in AMReX
- [ ] add new capabilities to AMReX
- [ ] changes answers in the test suite to more than roundoff level
- [ ] are likely to significantly affect the results of downstream AMReX
users
- [ ] include documentation in the code and/or rst files, if appropriate
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this issue Mar 11, 2024
Make `omp_locks` a variable in an unnamed namespace instead of an extern
global variable.

This might potentially fix the Windows symbol issue (AMReX-Codes#3795).
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this issue Mar 11, 2024
Make `omp_locks` a variable in an unnamed namespace instead of an extern
global variable.

This might potentially fix the Windows symbol issue (AMReX-Codes#3795).
WeiqunZhang added a commit to WeiqunZhang/amrex that referenced this issue Mar 11, 2024
Make `omp_locks` a variable in an unnamed namespace instead of an extern
global variable.

This might potentially fix the Windows symbol issue (AMReX-Codes#3795).
WeiqunZhang added a commit that referenced this issue Mar 14, 2024
Make `omp_locks` a variable in an unnamed namespace instead of an extern
global variable.

This might potentially fix the Windows symbol issue (#3795).
@ax3l
Copy link
Member Author

ax3l commented Apr 7, 2024

WarpX does not add template lockAdd<runOn...> in its usage of lockAdd...

mf[mfi].template lockAdd<RunOn::Host>(tmp, tbx, tbx, 0, 0, 1);

https://github.com/search?q=repo%3AECP-WarpX%2FWarpX%20lockAdd&type=code

Even though it has a default and should not need it, maybe that confuses MSVC...

We could try that as a patch to
conda-forge/warpx-feedstock#82 (review)

@ax3l
Copy link
Member Author

ax3l commented Apr 22, 2024

With the new test added in AMReX via #3805, I suspect a toolchain issue in conda-forge causes this....

@ax3l
Copy link
Member Author

ax3l commented Apr 29, 2024

@isuruf noticed:
On Conda-Forge's builds, the AMReX DLL has void* * __cdecl amrex::OpenMP::get_lock(int), but not struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int).

Might be technically an alias, but is weird:
https://github.com/llvm/llvm-project/blob/llvmorg-18.1.4/openmp/runtime/src/include/omp.h.var#L82-L85

I can reproduce this: if I inspect the latest .dll with radare2 I also see:

$ rabin2 -s amrex_3d.dll
...
3352 0x00054390 0x180054f90 GLOBAL FUNC 0    amrex_3d.dll
    ?get_lock@OpenMP@amrex@@YAPEAPEAXH@Z
    void * __ptr64 * __ptr64 __cdecl amrex::OpenMP::get_lock(int)
...
19   0x003e97c0 0x1803ea3c0 NONE   FUNC 0    libomp.dll                            imp.omp_destroy_lock
20   0x003e97c8 0x1803ea3c8 NONE   FUNC 0    libomp.dll                            imp.omp_get_max_threads
21   0x003e97d0 0x1803ea3d0 NONE   FUNC 0    libomp.dll                            imp.omp_get_num_threads
22   0x003e97d8 0x1803ea3d8 NONE   FUNC 0    libomp.dll                            imp.omp_get_thread_num
23   0x003e97e0 0x1803ea3e0 NONE   FUNC 0    libomp.dll                            imp.omp_in_parallel
24   0x003e97e8 0x1803ea3e8 NONE   FUNC 0    libomp.dll                            imp.omp_init_lock
25   0x003e97f0 0x1803ea3f0 NONE   FUNC 0    libomp.dll                            imp.omp_set_num_threads
...

@ax3l

This comment was marked as resolved.

@ax3l
Copy link
Member Author

ax3l commented Apr 29, 2024

We can also try to switch from Clang-CL back to MVSC and the new LLVM support in it:
https://learn.microsoft.com/en-us/cpp/build/reference/openmp-enable-openmp-2-0-support?view=msvc-170
https://gitlab.kitware.com/cmake/cmake/-/issues/25570

@WeiqunZhang
Copy link
Member

@ax3l Could we try this? #3910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants