dependencies: add custom atomic dependency #11445
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11445 +/- ##
==========================================
+ Coverage 65.25% 68.70% +3.44%
==========================================
Files 420 209 -211
Lines 91410 45429 -45981
Branches 20305 9402 -10903
==========================================
- Hits 59648 31211 -28437
+ Misses 27115 11817 -15298
+ Partials 4647 2401 -2246 see 273 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I'm not qualified to review this, but it's been a long time since atomics were merged into the C standard, and since POSIX hasn't yet addressed the issue, I very much welcome Meson's support for an atomic dependency |
|
The discussion in #10621 is relevant. I need to look at this more later though, we've run into a series of issues on odd platforms with atomic tests. |
|
The intention is indeed to first check if atomics is already provided and if not try to link the atomic library if possible. I confess that I do not know if atomics are more complicated than that. |
Dudemanguy
force-pushed
the
stdatomic-dep
branch
from
484f5bb to
4a56692
Compare
|
I bumped it to |
Dudemanguy
force-pushed
the
stdatomic-dep
branch
from
4a56692 to
416c717
Compare
|
Updated to 1.3.0 now. |
Dudemanguy
force-pushed
the
stdatomic-dep
branch
2 times, most recently
from
2007324 to
fc07c2b
Compare
mesonbuild/dependencies/misc.py
Outdated
| 'atomic', | ||
| [DependencyMethods.BUILTIN, DependencyMethods.SYSTEM], | ||
| builtin_class=AtomicBuiltinDependency, | ||
| system_class=AtomicSystemDependency, |
There was a problem hiding this comment.
This has come up in Gentoo before, which also had a test to add -latomic to LDFLAGS after running some test code to see if it's necessary for a successful link.
https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=ec90f4c3a4e0ef9841dede7b18df90e2aceafd45
We (I, actually) disabled that and replaced it with some code that says "is -latomic a valid library? Good, then use it unconditionally with -Wl,--push-state,--as-needed,-latomic,--pop-state". The reason was that the test often misfired because the test code could be satisfied via builtins, but more advanced use cases in the software being built required explicit -latomic.
So I wonder if maybe we really need to first check DependencyMethods.SYSTEM, on the same assumption: if -latomic is a valid external library, adding it to be cautious is assumed to never be harmful. It makes the compiler command line potentially longer and the linker has to add another library to search through for available symbols, but that's not a huge problem given the alternative.
There was a problem hiding this comment.
Sure, that sounds reasonable to me. I swapped the order so the system method is checked first.
Dudemanguy
force-pushed
the
stdatomic-dep
branch
from
fc07c2b to
7c407ce
Compare
docs/markdown/Dependencies.md
Outdated
|
|
||
| Provides access to the atomic operations library. This first attempts | ||
| to look for a valid atomic external library before trying to fallback | ||
| to what is provided by the libc. |
There was a problem hiding this comment.
| to what is provided by the libc. | |
| to what is provided by the C runtime libraries. |
but let's fix that on merge to avoid delaying this more.
There was a problem hiding this comment.
Well, since we forgot to re-review this a while back we do need to bump the new-since version number yet again... :)
Other than that though I do think this looks quite reasonable.
mesonbuild/dependencies/misc.py
Outdated
| @@ -50,6 +50,27 @@ def netcdf_factory(env: 'Environment', | |||
| packages['netcdf'] = netcdf_factory | |||
|
|
|||
|
|
|||
| class AtomicBuiltinDependency(BuiltinDependency): | |||
| def __init__(self, name: str, env: 'Environment', kwargs: T.Dict[str, T.Any]): | |||
There was a problem hiding this comment.
Nit: the codebase now depends on python 3.7 and as such, uses from __future__ import annotations everywhere. That means we don't need to quote annotations to defer them anymore. We should use the new style, i.e. avoid the quotes, when adding brand-new code
mesonbuild/dependencies/misc.py
Outdated
|
|
||
|
|
||
| class AtomicSystemDependency(SystemDependency): | ||
| def __init__(self, name: str, env: 'Environment', kwargs: T.Dict[str, T.Any]): |
Almost exactly the same as how the dl dependency works. On certain systems (like BSDs that use clang), stdatomic is provided by compiler-rt and doesn't need a separate library explictly linked. On a typical GNU/LINUX system, atomic is a separate library that must be explictly found and linked against. So just add a builtin and system method for these two use cases.
Dudemanguy
force-pushed
the
stdatomic-dep
branch
from
7c407ce to
9fd0695
Compare
|
Totally forgot about this PR as well. Rebased and updated. |
|
Thanks for the rebase, will review on Sunday as I'm out of time today |
Almost exactly the same as how the dl dependency works. On certain systems (like BSDs that use clang), stdatomic is provided by compiler-rt and doesn't need a separate library explictly linked. On a typical GNU/LINUX system, atomic is a separate library that must be explictly found and linked against. So just add a builtin and system method for these two use cases.