nodejs
build: Don't add `-latomic` for clang
compiler-rt supports __atomic_is_lock_free since LLVM 12[0]. Requiring -latomic on clang builds introduces a dependency on libgcc, which is often undesirable on LLVM-based build environments. This change makes sure it's never added in clang builds, so it can use the compiler-rt's implementation, making it possible to build Node.js on Linux systems with LLVM as the main toolchain, without having to install GCC.
compiler-rt's __atomic_is_lock_free works on all architectures, so there is no need to to link libatomic for mips64, ppc, arm, riscv etc. as well, as long as clang is used.
Similar patch was already adopted or discussed in:
- Chimera Linux
- https://github.com/chimera-linux/cports/blob/master/contrib/nodejs/patches/no-libatomic.patch
- Gentoo (LLVM profiles)
- https://bugs.gentoo.org/869992
- https://bugs.gentoo.org/911340
- https://github.com/gentoo/gentoo/pull/33141
[0] https://github.com/llvm/llvm-project/commit/00530dee5d1295dc20ebafdd9a8a79662f41513e
I see. I can't reproduce this error locally, perhaps it's either:
- too old clang on Github runners
- compiler-rt not being installed
- compiler-rt not being picked up for some reason
I will get back to you once I figure out.
Any progress on this PR? I have been using this patch for months with my llvm+musl gentoo system on both x86_64 and aarch64 and have had no problems. Hope this issue will be resolved soon. Let me know if there is anything I can do to help.
It seems unlikely. We found recently that newer V8 versions require it for __atomic_compare_exchange
I do keep trying to explain this to people on our end. You can't be sure that the compiler won't emit a libcall that compiler-rt doesn't supply (it's not clear to me why Clang does this, but whatever) -- or that, with GCC, needs libatomic, hence what nodejs is doing right now is fine (or fine enough). Working on one person's machine doesn't mean it should be removed.
We removed it a few times and had reports for some, but not all, users of the Clang case, and the GCC case is obvious. So yes, the status quo is correct (or more correct than dropping it).
https://github.com/microsoft/mimalloc/pull/898 I worked on a similar problem with mimalloc a while ago. mimalloc was also unconditionally adding -latomic, but by using add_link_library, it changed to only use it in environments where libatomic is available. I think a similar solution could be applied to nodejs, what do you think?
It seems unlikely. We found recently that newer V8 versions require it for
__atomic_compare_exchange
I could build latest nodejs(98788dace6) with llvm-18.1.8 successfully. Did you mean the upstream V8, not the one included in nodejs?
Anyway, LLVM has implemented __atomic_compare_exchange years ago. I haven't been able to verify that the latest V8 can be built with LLVM (building V8 in a non-glibc environment is a pain!). But as long as the latest nodejs can be built without problems, I think it's good to support natively such an environment.
Does gyp have ability to find optional dependencies like CMake's check_linker_flag? If so, it can be used to eliminate ugly flag addition based on the architecture in tools/v8_gypfiles/v8.gyp If not, at least the behaviour should be configurable as @targos says.
First of all, I'm sorry for the late follow-up.
@targos
It seems unlikely. We found recently that newer V8 versions require it for
__atomic_compare_exchange
Just for clarity __atomic_compare_exchange is provided by compiler-rt here https://github.com/llvm/llvm-project/blob/llvmorg-18.1.8/compiler-rt/lib/builtins/atomic.c#L41
The reason why the CI is red and what I overlooked when submitting the PR is the fact that clang on the most of Linux distros is going to pick up libgcc_s as a runtime library, unless you suppress that with --rtlib=compiler-rt ldflag.
What would have a chance of working would be not requiring -latomic after checking that compiler-rt is used as rtlib, that would be probably way too messy to do in gyp and there is a chance I'm still overlooking something for certain configurations, so...
What could be accepted more easily is a new
configureflag to disable it.
...I'm happy to hear that and I totally agree that would be the best approach! I will do it in this PR.
Would you be also open to having a CI job checking the clang+compiler-rt configuration (which would disable latomic), just to make sure things don't regress? I could try to use the docker.io/gentoo/stage3:musl-llvm image to add a job similar to this one:
https://github.com/nodejs/node/blob/accb23927230b0149321a62d6c7b64fe6ce7d65e/.github/workflows/test-linux.yml#L35-L56
@thesamesam
We removed it a few times and had reports for some, but not all, users of the Clang case, and the GCC case is obvious. So yes, the status quo is correct (or more correct than dropping it).
Are you referring to https://bugs.gentoo.org/870919?
There's https://bugs.gentoo.org/869992#c4 and so on too. All of the reports are with Clang versions later than the one which added support. I think someone who cares about such setups needs to investigate it, but the explanation being offered isn't sufficient at least.
OK, coming back to this, I think it'd work to have such a check if it's possible in gyp.
I assume the problem reported on Gentoo is caused by the user's environment setup, not nodejs. The idea of adding the configure option seems fine. Is there any obstacle?
I don't think that concurs with @vadorovsky's analysis at https://bugs.gentoo.org/911340#c5.
@targos @thesamesam First of all, sorry for very late follow up!
I went ahead with adding a configure option, which also performs a check whether compiler-rt really supports atomics. I also added more context and linked to the Gentoo bug, where we were figuring out how and under what circumstances compiler-rt doesn't need libatomic and came up with the solution proposed her.
I'm still thinking how to test that in CI. I was considering using the docker.io/gentoo/stage3:musl-llvm container image. If you agree with that solution, I can do that.
Closing this PR. Detecting whether compiler-rt can provide all atomics works, but bloats the gyp script. We decided that the way to go is to fix provide an empty stub libatomic.so in Gentoo on installations which have compiler-rt with all atomics enabled.