| Summary: | [CMake] Properly detect compiler flags, needed libs, and fallbacks for usage of 64-bit atomic operations | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Adrian Perez <aperez> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | aperez, berto, clopez, ews-watchlist, guijemont, mcatanzaro, olivier.blin, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | Other | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=182610 https://bugs.webkit.org/show_bug.cgi?id=161900 |
||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Adrian Perez
2018-02-08 13:29:33 PST
In a private chat with Alberto Garcia he mentioned that for the Debian packaging of WebKitGTK+ the following patch is used: https://sources.debian.org/patches/webkit2gtk/2.18.6-1/fix-ftbfs-armel.patch/ In short, what it does is using a check similar to the one present in “Source/WebKit/CMakeLists.txt” (lines 780 and below), and only if linking fails then it adds “Source/WTF/wtf/Atomics.cpp” to the build, which has implementations for “__sync_add_and_fetch_8” and “__sync_sub_and_fetch_8”. These functions are also provided by “libatomic“. I think we should do better detection of what's needed to use 64-bit atomic operations and packagers should not need using downstream patches like the one linked above. Also, I have been checking the libstdc++ manual at http://gcc.gnu.org/onlinedocs/libstdc++/manual/using_concurrency.html has some remarks about usage of atomic operations. The relevant ones are: - “[…] multithreaded C++ application are only supported when libstdc++ and all user code was built with compilers which report (via gcc/g++ -v) the same thread model and that model is not ‘single’” - “[…] you will probably need to add a library or flag to g++. […] Some ports support a special flag (the spelling isn't even standardized yet) to add all required macros to a compilation (if any such flags are required then you must provide the flag for all compilations not just linking) and link-library additions and/or replacements at link time. The documentation is weak. On several targets (including GNU/Linux, Solaris and various BSDs) -pthread is honored. Some other ports use other switches. This is not well documented anywhere other than in "gcc -dumpspecs" (look at the 'lib' and 'cpp' entries).” - “Some uses of std::atomic also require linking to libatomic.” With all the above in mind, the following seems like a sensible approach to me: * We may want to check the output of “${CXX} -v” and ensure that it does not contain “Thread model: single”. If that line is found, CMake should bail out with a message which prompts the user to switch to a different compiler. * For compilers which support “-pthread”, always pass it *both* for compilation and linking. Note that this is not exclusive to GCC: a quick check shows that Clang supports the flag at least on x86_64! (Doubt: How about MacOS? I guess it won't hurt.) * Detect how to enable the support for 64-bit atomic operations, in this order of preference: 1. Compiler has direct support: it emits code which does not need additional support routines being linked in. Action: None. No additional compiler or linker flags are needed, nor building extra sources. 2. Compiler needs support functions, libatomic is present and linking it in a test program which uses 64-bit atomic operations succeeds. Action: Flag “-latomic” is added to the linker flags for all WebKit components. 3. Compiler needs support functions, libatomic is not present, or not needed when linking in a test program which uses 64-bit atomic operations *and* “Source/WTF/wtf/Atomics.cpp” is also built. Action: Add “Source/WTF/wtf/Atomics.cpp” to the list of sources to build into WTF. This is enough because all other WebKit components link to WTF. 4. Compiler needs support functions, libatomic is present and linking it in a test program which uses 64-bit atomic operations succeeds *if* “Souce/WTF/Atomics.cpp” is also built. Action: Add “Source/WTF/wtf/Atomics.cpp” to the list of sources to build into WTF *and* add flag “-latomic” to the linker flags for all WebKit components. 5. Cannot link a test program which uses 64-bit atomic operations using any of the above approaches. Action: Let CMake bail out and inform the user. I think the above would cover all possible cases and would avoid needing downstream patches for some architectures, or stopgap patches like the one from bug #182610 (which fixed the MIPS build). According to https://nibblestew.blogspot.com/2017/12/these-three-things-could-improve-linux.html point two, these stupid libraries (-lm, -lpthread, I assume also -latomic) are going away in glibc. (In reply to Michael Catanzaro from comment #2) > According to > https://nibblestew.blogspot.com/2017/12/these-three-things-could-improve- > linux.html point two, these stupid libraries (-lm, -lpthread, I assume also > -latomic) are going away in glibc. That post has *suggestions* to solve this mess, but does not state anywhere that the upstreams for GCC, pkg-config, nor glibc are committed to solve it. Even if this would be fixed in the next GCC, pkg-config, and glibc releases, we would still need to support LTS distros until 2020, so we *need* to figure out how to pass the needed options when building WebKitGTK+ — there's no way around it. "The good thing is that Glibc maintainers are already in the process of doing this transition. Soon all of this pointless flag juggling will go away." (In reply to Adrian Perez from comment #3) > Even if this would be fixed in the next GCC, pkg-config, and glibc > releases, we would still need to support LTS distros until 2020, > so we *need* to figure out how to pass the needed options when > building WebKitGTK+ — there's no way around it. True. I'm not sure if this is the proper place to report this, but in the latest releases from the 2.19.x series the armel and sh4 builds are failing when linking JavaScriptCore: cd /<<PKGBUILDDIR>>/obj-arm-linux-gnueabi/Source/JavaScriptCore && /usr/bin/cmake -E cmake_link_script CMakeFiles/JavaScriptCore.dir/link.txt --verbose=1 /usr/bin/c++ -fPIC -Wno-expansion-to-defined -Wno-attributes -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wextra -Wall -g1 -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -DNDEBUG -DG_DISABLE_CAST_CHECKS -fno-strict-aliasing -fno-exceptions -std=c++14 -fno-rtti -Wl,--no-undefined -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -Wl,--no-keep-memory -shared -Wl,-soname,libjavascriptcoregtk-4.0.so.18 -o ../../lib/libjavascriptcoregtk-4.0.so.18.7.7 CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSourceXXX.cpp.o ../../lib/libWTFGTK.a /usr/lib/arm-linux-gnueabi/libicui18n.so /usr/lib/arm-linux-gnueabi/libglib-2.0.so ../../lib/libbmalloc.a -ldl /usr/lib/arm-linux-gnueabi/libicudata.so /usr/lib/arm-linux-gnueabi/libicui18n.so /usr/lib/arm-linux-gnueabi/libicuuc.so -lpthread /usr/lib/arm-linux-gnueabi/libgio-2.0.so /usr/lib/arm-linux-gnueabi/libgobject-2.0.so /usr/lib/arm-linux-gnueabi/libglib-2.0.so /usr/lib/arm-linux-gnueabi/libz.so CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource69.cpp.o: In function `std::__atomic_base<unsigned long long>::fetch_add(unsigned long long, std::memory_order)': /usr/include/c++/7/bits/atomic_base.h:514: undefined reference to `__atomic_fetch_add_8' (In reply to Alberto Garcia from comment #5) > I'm not sure if this is the proper place to report this, but in the > latest releases from the 2.19.x series the armel and sh4 builds are > failing when linking JavaScriptCore: > > [...] Indeed, the sh4 and armel build issues you are reporting are also a manifestiation of this bug. I'll give today a try to make a generic fix as outlined above for this bug, so we can get CMake to detect whether libatomic is needed independently of the architecture. (In reply to Adrian Perez from comment #1) > * For compilers which support “-pthread”, always pass it *both* for > compilation and linking. Note that this is not exclusive to GCC: > a quick check shows that Clang supports the flag at least on x86_64! > (Doubt: How about MacOS? I guess it won't hurt.) This seems to be the way to do it in CMake: --- webkit2gtk-2.20.1.orig/Source/cmake/OptionsGTK.cmake +++ webkit2gtk-2.20.1/Source/cmake/OptionsGTK.cmake @@ -32,6 +32,7 @@ find_package(LibSoup 2.42.0 REQUIRED) find_package(LibXml2 2.8.0 REQUIRED) find_package(PNG REQUIRED) find_package(Sqlite REQUIRED) +set(THREADS_PREFER_PTHREAD_FLAG ON) find_package(Threads REQUIRED) find_package(ZLIB REQUIRED) find_package(ATK REQUIRED) More info here: https://bugs.debian.org/895969 (In reply to Adrian Perez from comment #0) > After the patch for bug #182610 on MIPS libatomic is unconditionally > added to the libraries linked into JSC. Instead, it would be desirable > to detect whether it's needed and add it only in that case. How about this? list(APPEND JavaScriptCore_LIBRARIES -Wl,--as-needed -Wl,-latomic -Wl,--no-as-needed ) It's not a ${FOO_LIBRARIES} variable so doesn't belong in JavaScriptCore_LIBRARIES. Same problem with the commit in bug #228370, that'll go away next time somebody does a cleanup of that CMake file, it should never have been pushed.
(In reply to Michael Catanzaro from comment #9) > It's not a ${FOO_LIBRARIES} variable so doesn't belong in > JavaScriptCore_LIBRARIES. Same problem with the commit in bug #228370, > that'll go away next time somebody does a cleanup of that CMake file, it > should never have been pushed. Ok, so there's two questions: 1) Is -Wl,--as-needed -Wl,-latomic -Wl,--no-as-needed a fix for our problem? 2) Where should we put it? (In reply to Alberto Garcia from comment #10) > Ok, so there's two questions: > > 1) Is -Wl,--as-needed -Wl,-latomic -Wl,--no-as-needed a fix for our problem? > 2) Where should we put it? I don't know about (1). For (2) you could do something like: WEBKIT_ADD_TARGET_PROPERTIES(JavaScriptCore LINK_FLAGS "...") if there's really no better way.... (In reply to Alberto Garcia from comment #10) > (In reply to Michael Catanzaro from comment #9) > > It's not a ${FOO_LIBRARIES} variable so doesn't belong in > > JavaScriptCore_LIBRARIES. Same problem with the commit in bug #228370, > > that'll go away next time somebody does a cleanup of that CMake file, it > > should never have been pushed. > > Ok, so there's two questions: > > 1) Is -Wl,--as-needed -Wl,-latomic -Wl,--no-as-needed a fix for our problem? Bad idea. If “libatomic.{a,so}” is not available, linking would fail without reason in systems which do not need/ship libatomic (e.g. in those cases where the compiler directly emits code to implement atomic operations). When using “-Wl,--as-needed”, libraries listed in the linker command line from which no symbols are used do not generate DT_NEEDED entries in the resulting binary. That's okay, and in general we should always build with “--as-needed” (many GNU/Linux distros add it to LDFLAGS) so the dynamic linker does not load at runtime libraries which are not really used — but that's not what this bug is about. > 2) Where should we put it? Nowhere, we don't want the build failing randomly due to the linker always searching for libatomic.{a,so} even in systems that do not need it. (In reply to Alberto Garcia from comment #7) > (In reply to Adrian Perez from comment #1) > > * For compilers which support “-pthread”, always pass it *both* for > > compilation and linking. Note that this is not exclusive to GCC: > > a quick check shows that Clang supports the flag at least on x86_64! > > (Doubt: How about MacOS? I guess it won't hurt.) > > This seems to be the way to do it in CMake: > > --- webkit2gtk-2.20.1.orig/Source/cmake/OptionsGTK.cmake > +++ webkit2gtk-2.20.1/Source/cmake/OptionsGTK.cmake > @@ -32,6 +32,7 @@ find_package(LibSoup 2.42.0 REQUIRED) > find_package(LibXml2 2.8.0 REQUIRED) > find_package(PNG REQUIRED) > find_package(Sqlite REQUIRED) > +set(THREADS_PREFER_PTHREAD_FLAG ON) > find_package(Threads REQUIRED) > find_package(ZLIB REQUIRED) > find_package(ATK REQUIRED) > > More info here: https://bugs.debian.org/895969 This is something we may want to do, I think. I see this in WebKit/CMakeLists.txt:
if (COMPILER_IS_GCC_OR_CLANG)
set(ATOMIC_TEST_SOURCE
"
#include <atomic>
int main() { std::atomic<int64_t> i(0); i++; return 0; }
"
)
check_cxx_source_compiles("${ATOMIC_TEST_SOURCE}" ATOMIC_INT64_IS_BUILTIN)
if (NOT ATOMIC_INT64_IS_BUILTIN)
set(CMAKE_REQUIRED_LIBRARIES atomic)
check_cxx_source_compiles("${ATOMIC_TEST_SOURCE}" ATOMIC_INT64_REQUIRES_LIBATOMIC)
if (ATOMIC_INT64_REQUIRES_LIBATOMIC)
list(APPEND WebKit_LIBRARIES PRIVATE atomic)
endif ()
unset(CMAKE_REQUIRED_LIBRARIES)
endif ()
endif ()
Maybe it needs to move down to JavaScriptCore?
(In reply to Michael Catanzaro from comment #14) > I see this in WebKit/CMakeLists.txt: > > if (COMPILER_IS_GCC_OR_CLANG) > set(ATOMIC_TEST_SOURCE > " > #include <atomic> > int main() { std::atomic<int64_t> i(0); i++; return 0; } > " > ) > check_cxx_source_compiles("${ATOMIC_TEST_SOURCE}" I don't think this test alone is enough to determine if libatomic is needed in all cases. (In reply to Adrian Perez from comment #12) > > 1) Is -Wl,--as-needed -Wl,-latomic -Wl,--no-as-needed a fix for our problem? > > Bad idea. If “libatomic.{a,so}” is not available, linking would fail > without reason in systems which do not need/ship libatomic (e.g. in > those cases where the compiler directly emits code to implement > atomic operations). Perhaps we could ship a cmake file to detect atomics support, llvm already ships one: https://sources.debian.org/src/llvm-toolchain-5.0/1:5.0.1-4/cmake/modules/CheckAtomic.cmake/ Created attachment 340248 [details]
Patch for WebKitGTK+ 2.20.2
I tried this patch in the Debian build of 2.20.2 and I can confirm that it fixes the build on all major architectures.
From what I've seen these are the architectures that need to link against libatomic, and checking for __atomic_fetch_add_8() seems to be enough to determine that:
- armel
- m68k
- mips
- mipsel
- powerpc
- powerpcspe
- sh4
In addition that check, enabling THREADS_PREFER_PTHREAD_FLAG fixes the build in RISC-V 64.
Plus, the Atomics.cpp file can be removed from the repository (it has already been removed in the master branch). Comment on attachment 340248 [details] Patch for WebKitGTK+ 2.20.2 View in context: https://bugs.webkit.org/attachment.cgi?id=340248&action=review (informal review: r+ with nits) This patch is already better than what we have now. I would go for landing this (with the couple of things I mention above fixed) instead of waiting further. We can always refine later on if needed. > Source/JavaScriptCore/CMakeLists.txt:125 > + list(APPEND JavaScriptCore_LIBRARIES -latomic) Probably it would be good to also try to compile the test program again here, passing -latomic to the linker, and if that also fails then write a message and stop CMake — better to fail at configuration time than at build time. > Source/cmake/OptionsGTK.cmake:35 > +set(THREADS_PREFER_PTHREAD_FLAG ON) I would also add this to OptionsWPE.cmake Comment on attachment 340248 [details] Patch for WebKitGTK+ 2.20.2 View in context: https://bugs.webkit.org/attachment.cgi?id=340248&action=review > Source/cmake/OptionsGTK.cmake:36 > +set(THREADS_PREFER_PTHREAD_FLAG ON) > find_package(Threads REQUIRED) And also split it out from this list, I suppose, for style: move these to either the top or bottom, separated by a blank line. Created attachment 340404 [details] Patch I realized that we already have the test that we need in Source/WebKit/CMakeLists.txt: https://github.com/WebKit/webkit/blob/525b5862131cf05ea05bace559173e65da5da827/Source/WebKit/CMakeLists.txt#L813 (originally from bug 173921) So we can simply use it in JavaScriptCore. I just tested it in armel and it seems to work fine. Here's the patch for master. Comment on attachment 340404 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340404&action=review There are a couple of small things I would change, but looks good overall. > Source/JavaScriptCore/CMakeLists.txt:127 > +if (COMPILER_IS_GCC_OR_CLANG) I would probably move the checks to “Source/cmake/WebKitCompilerFlags.cmake” and only have here something like: if (ATOMIC_INT64_REQUIRES_LIBATOMIC) list(APPEND JavaScriptCore_LIBRARIES atomic) endif () Mainly because the check is not specific to JavaCriptCore, and as a matter of fact gets used both for JSC and WebKit with this patch :-) > Source/cmake/OptionsGTK.cmake:19 > +set(THREADS_PREFER_PTHREAD_FLAG ON) Could you please add this line also in “OptionsWPE.cmake” and “OptionsJSCOnly.cmake”? Comment on attachment 340404 [details] Patch Attachment 340404 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7691199 New failing tests: http/tests/preload/onload_event.html Created attachment 340430 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 340473 [details]
Patch
Ok, here's the updated patch with Adrian's comments
Comment on attachment 340473 [details]
Patch
Alberto, this looks great now, thanks a lot for updating
the patch. As my review was informal (I'm not a reviewer
yet), I'll try to point a reviewer to this bug to try and
get it landed ASAP.
Comment on attachment 340473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340473&action=review Clearly better than what we had before. I agree WebKitComplierFlags.cmake is a good place for this test. Thanks, both of you! > Source/cmake/OptionsGTK.cmake:19 > +set(THREADS_PREFER_PTHREAD_FLAG ON) Add another blank line to separate it a bit from the install dirs. > Source/cmake/OptionsWPE.cmake:14 > +set(THREADS_PREFER_PTHREAD_FLAG ON) Ditto. Committed r231843: <https://trac.webkit.org/changeset/231843> Created attachment 340490 [details]
Patch for the 2.20 branch
The patch for the 2.20 branch is slightly different so I'm including it here for convenience.
(In reply to Alberto Garcia from comment #30) > Created attachment 340490 [details] > Patch for the 2.20 branch > > The patch for the 2.20 branch is slightly different so I'm including it here > for convenience. I see you have already added it to the list of patches for the 2.20.3 release in the wiki at https://trac.webkit.org/wiki/WebKitGTK/2.20.x :-) Thanks again for working on this! (In reply to Adrian Perez from comment #31) > (In reply to Alberto Garcia from comment #30) > > Created attachment 340490 [details] > > Patch for the 2.20 branch > > > > The patch for the 2.20 branch is slightly different so I'm including it here > > for convenience. > > I see you have already added it to the list of patches for the 2.20.3 > release in the wiki at https://trac.webkit.org/wiki/WebKitGTK/2.20.x :-) > Thanks again for working on this! This broke cross-compilation of WPE for ARMv7 with CMake (using CMake 3.8.2) The error is: CMake Error: TRY_RUN() invoked in cross-compiling mode, please set the following cache variables appropriately: THREADS_PTHREAD_ARG (advanced) Full log availables: - configure log: http://termbin.com/3vm7 - CMakeOutput.log: http://termbin.com/lo5l - CMakeError.log: http://termbin.com/dkerm And I have seen this also recently on the JSCOnly bots that cross-compile for MIPS/ARMv7. A quick search suggests that https://gitlab.kitware.com/cmake/cmake/issues/16920 may be related. But I'm unsure if its the cause. (In reply to Carlos Alberto Lopez Perez from comment #32) > This broke cross-compilation of WPE for ARMv7 with CMake (using > CMake 3.8.2) Perhaps we could disable that flag when doing cross-compiling, but we'd need to see how to do that and whether it continues building. This is not the only cross-compilation problem that we have by the way: https://bugs.webkit.org/show_bug.cgi?id=172799 Reverted r231843 for reason: Broke cross build Committed r232062: <https://trac.webkit.org/changeset/232062> Was it really needed to revert the libatomic change? It looks like only the pthread change was causing an issue. Berto can decide how he wants to re-land, I'm just the rollout monkey. ;) Created attachment 340984 [details]
Patch without THREADS_PREFER_PTHREAD_FLAG
I think Olivier is right, the pthread part is only needed for RISC-V as far as I'm aware, the rest of the changes are safe.
Committed r232067: <https://trac.webkit.org/changeset/232067> Berto, this is supposedly FIXED, but you are still carrying a different patch in the Debian package! Only for riscv64, see comment #32 (In reply to Alberto Garcia from comment #40) > Only for riscv64, see comment #32 Shouldn't we try and incorporate the changes needed for RISC-V upstream? I suspect the architecture will only get more popular as time goes by. If it's not too much code and JSC JIT is disabled, then yes, in a different bug. (In reply to Adrian Perez from comment #41) > (In reply to Alberto Garcia from comment #40) > > Only for riscv64, see comment #32 > > Shouldn't we try and incorporate the changes needed for RISC-V > upstream? I suspect the architecture will only get more popular > as time goes by. As explained in the comment that I mentioned above, this breaks other builds under certain circumstances. So first we need to find out what's going on and how to solve it. Having a riscv64 machine or VM would help. At the moment I don't have neither. Here are some tips: https://wiki.debian.org/RISC-V#Setting_up_a_riscv64_virtual_machine |