WIP/RFC: Add abi tags for ucrt/cxxlib #59029
Conversation
giordano
left a comment
giordano
left a comment
There was a problem hiding this comment.
Looks overall reasonable to me, but there are some build and tests failures to be addressed.
base/binaryplatforms.jl
Outdated
| ) | ||
| const libc_mapping = Dict( | ||
| "libc_nothing" => "", | ||
| "libc_nothing" => "-mingw32", # This is non-specific, our default above uses `msvcrt` |
There was a problem hiding this comment.
I think this is being applied to macOS and FreeBSD as well?
There was a problem hiding this comment.
and this dict has two entries with the same key
There was a problem hiding this comment.
Yeah, yeah . There's a reason this is WIP/RFC. The question currently is just on the abstract strategy ;)
|
|
||
| # For compatibility, libstdcxx_version counts as both cxxlib=libstdcxx and | ||
| # cxxlib_version. | ||
| if tag == "libstdcxx_version" |
There was a problem hiding this comment.
@giordano do we have any packages that require building with g++ on macOS? I know we used to, which is why we ship libstdc++ on macOS and open it as part of CSL, but I just loaded WGLMakie and it didn't pick up any libraries that actually use it. If we can actually convince ourselves that we'll never have the situation where most things use libc++ but a few things use libstdc++, then I think we're fine to do this Keno's way. Otherwise, I think we need to have libstdcxx_version and libcxx_version so Julia can express that it is simultaneously loading two different C++ stdlibs.
There was a problem hiding this comment.
Just to state it explicitly, mixing C++ implementations is a BAD idea, but one way it can come about and not be fatal is if a library uses C++ internally (and does not take in/give out C++ objects through its API). If you have two such libraries loaded at once, you need to ensure that you're selecting versions of those libraries that agree on the minimum version of libstdc++, despite the fact that 99% of your program is using libc++.
I was under the impression that we did have sub-trees of dependencies running using libstdc++ even on macOS/FreeBSD, but perhaps that is no longer the case and we don't have to fear this anymore.r
There was a problem hiding this comment.
do we have any packages that require building with g++ on macOS?
Yes, there are a bunch of packages which do use g++ on macOS.
There was a problem hiding this comment.
Can we assemble a list. Why are they using g++ do they specifically care about libstdc++ or is there an issue with clang? If the latter, can we try rebuilding them with g++ but linking libc++? gcc should support it.
There was a problem hiding this comment.
This is what I found with a quick search (CC=gcc and CMAKE_TARGET_TOOLCHAIN.*gcc):
Unclear why gcc is needed (sometimes it may be due to OpenMP runtime, but unclear just by reading the code):
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/Chafa/build_tarballs.jl#L14-L17
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/cilkrts/build_tarballs.jl#L17
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/Coin-OR/CSDP/build_tarballs.jl#L25-L27
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/CvxCompress/build_tarballs.jl#L29-L32
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/D/difmap/build_tarballs.jl#L21-L24
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/G/GTPSA/build_tarballs.jl#L21
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/K/KMC/build_tarballs.jl#L41-L45 (according to comments this may use GNU extensions)
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/L/LAMMPS/build_tarballs.jl#L84 was changed in [LAMMPS] Update LAMMPS to stable_29Aug2024 JuliaPackaging/Yggdrasil#9360 because of errors like
[19:04:33] In file included from /workspace/srcdir/lammps/src/MEAM/pair_meam.cpp:18: [19:04:33] In file included from /workspace/srcdir/lammps/src/MEAM/pair_meam.h:23: [19:04:33] In file included from /workspace/srcdir/lammps/src/pair.h:17: [19:04:33] In file included from /workspace/srcdir/lammps/src/pointers.h:30: [19:04:33] In file included from /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/string:505: [19:04:33] In file included from /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/string_view:176: [19:04:33] In file included from /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/__string:57: [19:04:33] In file included from /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/algorithm:644: [19:04:33] /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/memory:3658:7: error: exception specification of overriding function is more lax than base version [19:04:33] 3658 | class __shared_ptr_emplace [19:04:33] | ^ [19:04:33] /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/memory:4331:26: note: in instantiation of template class 'std::__shared_ptr_emplace<LAMMPS_NS::PotentialFileReader, std::allocator<LAMMPS_NS::PotentialFileReader>>' requested here [19:04:33] 4331 | ::new(__hold2.get()) _CntrlBlk(__a2, _VSTD::forward<_Args>(__args)...); [19:04:33] | ^ [19:04:33] /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/memory:4710:29: note: in instantiation of function template specialization 'std::shared_ptr<LAMMPS_NS::PotentialFileReader>::make_shared<LAMMPS_NS::LAMMPS *&, const std::basic_string<char> &, const char (&)[5]>' requested here [19:04:33] 4710 | return shared_ptr<_Tp>::make_shared(_VSTD::forward<_Args>(__args)...); [19:04:33] | ^ [19:04:33] /workspace/srcdir/lammps/src/MEAM/pair_meam.cpp:552:36: note: in instantiation of function template specialization 'std::make_shared<LAMMPS_NS::PotentialFileReader, LAMMPS_NS::LAMMPS *&, const std::basic_string<char> &, const char (&)[5]>' requested here [19:04:33] 552 | if (comm->me == 0) reader = std::make_shared<PotentialFileReader>(lmp, userfile, "MEAM"); [19:04:33] | ^ [19:04:33] /opt/aarch64-apple-darwin20/aarch64-apple-darwin20/sys-root/usr/include/c++/v1/memory:3567:13: note: overridden virtual function is here [19:04:33] 3567 | virtual ~__shared_weak_count(); [19:04:33] | ^ [19:04:33] 1 error generated. - https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/L/lrslib/build_tarballs.jl#L26-L31
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/S/Sundials/Sundials32%405/build_tarballs.jl#L37-L41
Can probably be fixed by using LLVMOpenMP for OpenMP runtime:
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/CombBLAS/build_tarballs.jl#L27 a comment mentions "OpenMP concerns", there was a message about the code seemingly being written with GCC in mind
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/C/CovidSim/build_tarballs.jl#L23-L25
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/H/HHsuite/build_tarballs.jl#L24-L28
- https://github.com/JuliaPackaging/Yggdrasil/blob/c291e8f5925e2b21b5b8d6a0649f46c0dbac5bd7/S/SuperLU_MT/build_tarballs.jl#L39-L42
Because of JuliaPackaging/Yggdrasil#7139 (they claim a newer SDK didn't help, without providing more information):
This is a prepratory commit for reflecting windows/mingw32 libc information in our binary platforms, as well as adding the basic detection for ucrt in our Makefiles (so it doesn't accidentally try to use binaries with the wrong ABI). Of course there are policy choices to be made about what we want to ship and test. This does not make any of those policy choices, it simply aims to provide the capability to represent these ABIs in case we decide to add them later. To reflect the possibility of a libc++ difference (mingw comes in both libc++ and libstdc++ flavors these days and the same is true for linux of course, although less prominently so), this adds a new `cxxlib` abi tag that can be either `libstdcxx` or `libcxx` (perhaps `msvc` in the future, since clang does support that). There is also `cxxlib_version`. For compatibility the existing `libstdcxx_version` implies both `cxxlib=libstdcxx` as well as the corresponding versions. I'm not weeded to this representation, I just figured it was a reasonable first attempt. Fixes #59009
Keno
force-pushed
the
kf/platformupgrades
branch
from
f6635bd to
b09e1d9
Compare
This is a prepratory commit for reflecting windows/mingw32 libc information in our binary platforms, as well as adding the basic detection for ucrt in our Makefiles (so it doesn't accidentally try to use binaries with the wrong ABI). Of course there are policy choices to be made about what we want to ship and test. This does not make any of those policy choices, it simply aims to provide the capability to represent these ABIs in case we decide to add them later.
To reflect the possibility of a libc++ difference (mingw comes in both libc++ and libstdc++ flavors these days and the same is true for linux of course, although less prominently so), this adds a new
cxxlibabi tag that can be eitherlibstdcxxorlibcxx(perhapsmsvcin the future, since clang does support that). There is alsocxxlib_version. For compatibility the existinglibstdcxx_versionimplies bothcxxlib=libstdcxxas well as the corresponding versions. I'm not weeded to this representation, I just figured it was a reasonable first attempt.Fixes #59009