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

Provide atomic helper functions #384

Closed
mstorsjo opened this issue Dec 7, 2023 · 12 comments
Closed

Provide atomic helper functions #384

mstorsjo opened this issue Dec 7, 2023 · 12 comments

Comments

@mstorsjo
Copy link
Owner

mstorsjo commented Dec 7, 2023

It was noted in llvm/llvm-project#73398 (comment) that we're lacking atomic helper functions.

These functions are used as fallback, when the compiler can't generate inline code for atomic access to variables. In practice, it seems like calls to these functions are generated when using variables larger than 8 bytes, marked as atomic.

When this happens, Clang also prints a warning like this:

warning: misaligned atomic operation may incur significant performance penalty; the expected alignment (80 bytes) exceeds the actual alignment (4 bytes) [-Watomic-alignment]
   30 |     return atomic_load(&value);

In GCC, these helpers are provided in libatomic-1.dll, and if using them, you need to link with -latomic. (Callers that expect to use them might want to try to see if it is possible to link with -latomic, and if possible do that, otherwise leave it out.)

For LLVM based toolchains, compiler-rt builtins don't provide these helpers by default, but they can. But we'd need to settle on how to do that.

It can be done in two different ways; we can set -DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=FALSE (this defaults to TRUE) when building the compiler-rt builtins. Then we get these helper functions as part of the regular compiler-rt builtins library, which we link automatically, so everything works, without needing to add -latomic, and no extra runtime DLL is needed.

There's one drawback with this approach: The atomic helper functions use locks for making the accesses actually atomic. But if we statically link the locks into each separate DLL/EXE, then each module would use their own locks, so accesses to atomic variables larger than 8 bytes across DLLs would end up not properly atomic. Practically speaking, this is a very rare case though (the case in libcxx was the first time I hear someone request these helper functions at all), and I guess it is even more rare to have such atomics in the public DLL interface.

The alternative is to build compiler-rt builtins with -DCOMPILER_RT_BUILD_STANDALONE_LIBATOMIC=TRUE. This produces a new libclang_rt.atomic_dynamic-<arch>.dll and corresponding .dll.a. (Building it requires a recent fix, llvm/llvm-project@76b8975.) It is possible to copy libclang_rt.atomic_dynamic-<arch>.dll.a and name it libatomic.dll.a, so that users that check for -latomic will succeed and find it, even if the actual linked DLL has a different name.

I've tried implementing both of these; see master...atomics and master...atomics-standalone.

CCing @lazka @MehdiChinoune @mati865 @jeremyd2019 @alvinhochun @cjacek for some opinions on the matter before settling which way to go here. Do you think we should go for the complex solution with the separate DLL, or just keep it simple for now? (As user projects tend to need to check for -latomic anyway, we can probably easily switch to that form later, if we feel we need to.)

@lazka
Copy link

lazka commented Dec 7, 2023

Looking at the usual llvm-only Unix suspects I see:

Related finds:

Conclusion: I don't know, both seem fine.

@mstorsjo
Copy link
Owner Author

mstorsjo commented Dec 8, 2023

On modern platforms (at least the last decade), it seems like macOS is shipping the __atomic_* symbols as part of libSystem and/or libcompiler-rt, as a shared library. libcompiler-rt looks like a shared library version of compiler-rt builtins with atomics included in the same library.

Well it sounds like they have something like a libcompiler_rt (which I presume is a shared library)

Conclusion: I don't know, both seem fine.

It's not so much about what others do; larger OS vendors that have these bits integrated as a proper shared system library don't need to configure it via the regular compiler-rt build system.

It mostly boils down to this comment at https://reviews.llvm.org/D71600#1868502:

atomic.c is disabled by default for a good reason: it doesn't work correctly in general. In particular, if an atomic variable is shared across two shared libraries, the two libraries will use different locks, and therefore the operation won't be atomic.

It might make sense to provide a shared library compiler-rt-atomic or something like that for targets that want it, but someone would have to implement it.

This comment is from 2020; the standalone libatomic mode was added in 2021 here: llvm/llvm-project@d56729b

But in any case; for us it only boils down to whether we consider atomic access (to uncommonly large variables) across DLLs something we care about supporting properly - is it worth the extra inconvenience of an extra runtime DLL and a bit more build system mess?

As very few projects use this - presumably none of the ones within MSYS2 clang64 today as this is missing there right now AFAIK - almost nobody would be inconvenienced by the extra runtime DLL anyway. But it does make things a little bit more complicated in the build, e.g. 5087d5d. The only user I'm aware of right now, is libcxx's testsuite.

@lazka
Copy link

lazka commented Dec 8, 2023

It's not so much about what others do; larger OS vendors that have these bits integrated as a proper shared system library don't need to configure it via the regular compiler-rt build system.

ok, I mostly care that build systems/projects with unix in mind just work ideally, which is why I looked there. From what I understand both ways should work fine in that regard.

I don't have a strong opinion otherwise.

@alvinhochun
Copy link
Contributor

I am not familiar with the atomic helper functions, but looking at it briefly it seems std::atomic would behave the same as _Atomic and would be affected similarly. If we don't use the separate DLL, what are the chances of some usage of the atomic helper functions being linked into libc++.dll and some being linked into the user binary and causing issues?

I was curious and looked up what MSVC does will do, but looks like it won't help us:

Our locking atomics will have the lock stored inside the atomic object, meaning locking atomic objects are bigger than their non-atomic counterparts, and you can’t cast a pointer to a non-atomic object to a pointer to its atomic equivalent. On the other hand it means you can share locking atomics between processes (using shared memory segments) and get correct synchronization.

@mstorsjo
Copy link
Owner Author

mstorsjo commented Dec 8, 2023

I am not familiar with the atomic helper functions, but looking at it briefly it seems std::atomic would behave the same as _Atomic and would be affected similarly. If we don't use the separate DLL, what are the chances of some usage of the atomic helper functions being linked into libc++.dll and some being linked into the user binary and causing issues?

I guess there’s such a risk, in theory, but if that would be the case, I think the libc++ build would be failing now already. As we don’t have these symbols at all right now, any currently working build can’t be using them (unless they explicitly test for their availability in a configure test).

@mati865
Copy link
Contributor

mati865 commented Dec 9, 2023

DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=FALSE sounds neat because it doesn't involve additional dependencies.
Oh l on the other hand DCOMPILER_RT_BUILD_STANDALONE_LIBATOMIC=TRUE while involving additional shared dependency solves the issues for real.
Having been bitten by various binutils hacks that backfired I'd opt for 2nd solution, but I'm a bit biased.

@brad0
Copy link

brad0 commented Dec 10, 2023

Looking at the usual llvm-only Unix suspects I see:

OpenBSD uses their own build system and does the equivalent of COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=OFF.

@mstorsjo
Copy link
Owner Author

Looking at the usual llvm-only Unix suspects I see:

OpenBSD uses their own build system and does the equivalent of COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=OFF.

Thanks! I presume it's within a shared library in the end?

@brad0
Copy link

brad0 commented Dec 10, 2023

Thanks! I presume it's within a shared library in the end?

FreeBSD and OpenBSD only ship a PIC static library for libcompiler_rt.

https://github.com/freebsd/freebsd-src/blob/main/lib/libcompiler_rt/Makefile
https://github.com/openbsd/src/blob/master/gnu/lib/libcompiler_rt/Makefile

@mstorsjo
Copy link
Owner Author

Thanks! I presume it's within a shared library in the end?

FreeBSD and OpenBSD only ship a PIC static library for libcompiler_rt.

https://github.com/freebsd/freebsd-src/blob/main/lib/libcompiler_rt/Makefile https://github.com/openbsd/src/blob/master/gnu/lib/libcompiler_rt/Makefile

Oh, interesting. I guess that they don't consider cross-library atomic access (for unusually sized atomic variables that need the helpers) an issue then. On the other hand, as long as one doesn't link shared libraries with -Bsymbolic (or flag the atomics helper function symbols as hidden), I guess all libraries in a process would end up using one single copy of the helpers (and their associated locks), so it's probably even less of an issue.

Anyway, thanks for the data point, I guess this counts as a vague +1 for the case of "COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=OFF is good enough".

@mstorsjo
Copy link
Owner Author

With the LLVM 18.x release candidates coming up soon, I think I'll go ahead and merge the approach of using COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=OFF, for now.

@mstorsjo
Copy link
Owner Author

The latest (pre)release, at https://github.com/mstorsjo/llvm-mingw/releases/tag/20240130, should have atomics helpers now.

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

No branches or pull requests

5 participants