omp_locks: Avoid extern global variable #3798

WeiqunZhang · 2024-03-11T16:24:47Z

Make omp_locks a variable in an unnamed namespace instead of an extern global variable.

This might potentially fix the Windows symbol issue (#3795).

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).

Patch in AMReX-Codes/amrex#3798

ax3l · 2024-03-11T20:15:56Z

Thanks a lot!

Testing in

ax3l · 2024-03-11T20:19:34Z

Generally, I am wondering if Clang-Cl's lld-link lacks an additional -openmp or -Xclang -fopenmp. I see the libs on the linker line, but maybe it forgets an addtional OpenMP flag somewhere.

Patch in AMReX-Codes/amrex#3798

ax3l · 2024-03-12T01:59:46Z

Now it has a link error with:

lld-link: error: undefined symbol: struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int)

ax3l · 2024-03-12T02:06:15Z

New attempt w/ flags: conda-forge/impactx-feedstock#29

... no change

ax3l

Even though this does not solve the Windows symbol issue yet, this is clearly cleaner :)

ax3l · 2024-03-14T02:40:02Z

Does the type omp_lock_t in llvm-openmp maybe lack the symbol export?
omp_lock_t
Wouldn't one usually export struct definitions?

All the other OpenMP functions are defined as extern, which is interesting as well...

ax3l · 2024-03-14T02:43:21Z

Src/Base/AMReX_OpenMP.H

@@ -17,8 +17,7 @@ namespace amrex::OpenMP {
    void Initialize ();
    void Finalize ();

-    static constexpr int nlocks = 128;
-    extern AMREX_EXPORT omp_lock_t omp_locks[nlocks];
+    omp_lock_t* get_lock (int ilock);


Since it complains about this, maybe an export here helps:

Suggested change

omp_lock_t* get_lock (int ilock);

AMREX_EXPORT omp_lock_t* get_lock (int ilock);

Testing:

[Patch] Refresh Patch 3798 conda-forge/amrex-feedstock#20

then ImpactX feedstock Build Against AMReX 24.03 build3 conda-forge/impactx-feedstock#30

Still the same issue on Windows:

[48/49] C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app.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.dir\src\main.cpp.obj /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.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 impactx.NOMPI.OMP.DP.OPMD.exe D:/bld/impactx_1710432645630/work/build/bin/impactx"" FAILED: bin/impactx.NOMPI.OMP.DP.OPMD.exe C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app.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.dir\src\main.cpp.obj /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.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 impactx.NOMPI.OMP.DP.OPMD.exe D:/bld/impactx_1710432645630/work/build/bin/impactx"" LINK: command "%BUILD_PREFIX%\Library\bin\lld-link.exe CMakeFiles\app.dir\src\main.cpp.obj /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.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) struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int) >>> referenced by libimpactx.NOMPI.OMP.DP.OPMD.lib(ChargeDeposition.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)) [49/49] C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\pyImpactX.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\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO -flto lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\libs\python310.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 copy_directory D:/bld/impactx_1710432645630/work/src/python/impactx D:/bld/impactx_1710432645630/work/build/lib/site-packages/impactx"" FAILED: lib/site-packages/impactx/impactx_pybind.cp310-win_amd64.pyd C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\pyImpactX.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\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO -flto lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\libs\python310.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 copy_directory D:/bld/impactx_1710432645630/work/src/python/impactx D:/bld/impactx_1710432645630/work/build/lib/site-packages/impactx"" LINK: command "%BUILD_PREFIX%\Library\bin\lld-link.exe CMakeFiles\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO -flto lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\libs\python310.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=2" failed (exit code 1) with the following output: lld-link: warning: ignoring unknown argument '-flto' lld-link: error: undefined symbol: __declspec(dllimport) struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int) >>> referenced by libimpactx.NOMPI.OMP.DP.OPMD.lib(ChargeDeposition.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)) ninja: build stopped: subcommand failed.

Could this be missing includes? #3802

I don't see any missing includes.

Let's add a test that uses lockAdd to amrex/Tests/ to see if conda-forge Windows build work for amrex.

I don't see any missing includes.

I wondered how <omp.h> and AMReX_OpenMP.H come into downstream files such as ImpactX ChargeDeposition.cpp and AMReX_BaseFab.H for BaseFab::lockAdd. AMReX_MultiFab.H does not seem to include it directly, must be some transitive path. Anyway, #3802 does some cleaning for directly uses classes for future robustness.

## Summary Execute the `install` and `test_install` targets in Windows CI, too. ## Additional background Increase coverage, e.g., for issues such as #3798 #3802 ## Checklist The proposed changes: - [ ] fix a bug or incorrect behavior in AMReX - [x] 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

## Summary Add includes for directly used types. ## Additional background Related to #3798, I was wondering if there might be an missing include to `BaseFab<double>::lockAdd` and `AMReX_OpenMP.H` somewhere... In the end this is just cleaning. ## Checklist The proposed changes: - [ ] 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 requested a review from ax3l March 11, 2024 16:24

WeiqunZhang force-pushed the omp_lock_t branch from 56aa49a to e70a364 Compare March 11, 2024 16:27

omp_locks: Avoid extern global variable

cdf91b2

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 force-pushed the omp_lock_t branch from e70a364 to cdf91b2 Compare March 11, 2024 16:32

ax3l added a commit to ax3l/amrex-feedstock that referenced this pull request Mar 11, 2024

[Patch] omp_locks: Avoid extern global variable

3917d4d

Patch in AMReX-Codes/amrex#3798

ax3l mentioned this pull request Mar 11, 2024

[Patch] omp_locks: Avoid extern global variable conda-forge/amrex-feedstock#19

Merged

5 tasks

ax3l added the bug label Mar 11, 2024

ax3l added a commit to ax3l/amrex-feedstock that referenced this pull request Mar 11, 2024

[Patch] omp_locks: Avoid extern global variable

1a42614

Patch in AMReX-Codes/amrex#3798

ax3l mentioned this pull request Mar 13, 2024

install with conda error: Symbol not found: __ZN5amrex6OpenMP9omp_locksE BLAST-ImpactX/impactx#550

Closed

ax3l approved these changes Mar 14, 2024

View reviewed changes

ax3l reviewed Mar 14, 2024

View reviewed changes

This was referenced Mar 14, 2024

[Patch] Refresh Patch 3798 conda-forge/amrex-feedstock#20

Merged

Explicit Includes: MultiFab -> BaseFab #3802

Merged

CI: Windows Install & Test Install #3803

Merged

WeiqunZhang merged commit 431ad98 into AMReX-Codes:development Mar 14, 2024
69 checks passed

WeiqunZhang deleted the omp_lock_t branch March 14, 2024 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omp_locks: Avoid extern global variable #3798

omp_locks: Avoid extern global variable #3798

WeiqunZhang commented Mar 11, 2024

ax3l commented Mar 11, 2024 •

edited

Loading

ax3l commented Mar 11, 2024 •

edited

Loading

ax3l commented Mar 12, 2024

ax3l commented Mar 12, 2024 •

edited

Loading

ax3l left a comment

ax3l commented Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024

WeiqunZhang Mar 14, 2024

WeiqunZhang Mar 14, 2024

ax3l Mar 15, 2024 •

edited

Loading

	omp_lock_t* get_lock (int ilock);
	AMREX_EXPORT omp_lock_t* get_lock (int ilock);

omp_locks: Avoid extern global variable #3798

omp_locks: Avoid extern global variable #3798

Conversation

WeiqunZhang commented Mar 11, 2024

ax3l commented Mar 11, 2024 • edited Loading

ax3l commented Mar 11, 2024 • edited Loading

ax3l commented Mar 12, 2024

ax3l commented Mar 12, 2024 • edited Loading

ax3l left a comment

Choose a reason for hiding this comment

ax3l commented Mar 14, 2024 • edited Loading

ax3l Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

ax3l Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

ax3l Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

ax3l Mar 14, 2024

Choose a reason for hiding this comment

WeiqunZhang Mar 14, 2024

Choose a reason for hiding this comment

WeiqunZhang Mar 14, 2024

Choose a reason for hiding this comment

ax3l Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

ax3l commented Mar 11, 2024 •

edited

Loading

ax3l commented Mar 11, 2024 •

edited

Loading

ax3l commented Mar 12, 2024 •

edited

Loading

ax3l commented Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 14, 2024 •

edited

Loading

ax3l Mar 15, 2024 •

edited

Loading