Bug 182622 – [CMake] Properly detect compiler flags, needed libs, and fallbacks for usage of 64-bit atomic operations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adrian Perez]

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.

There is already a check for libatomic at “Source/WebKit/CMakeLists.txt”
(lines starting at 780) which could be reused. Probably the best course
of action is to:

  - Move the check into a new file e.g. “Source/cmake/CheckLibatomic.cmake”
  - Add “include(CheckLibatomic)” into “Source/cmake/WebKitCommon.cmake”
  - Use the result from the checks in all places where it's needed (for
    now that would be “Source/{JavaScriptCore,WebKit}/CMakeLists.txt”)

[Adrian Perez]

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).

[Michael Catanzaro]

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.

[Adrian Perez]

(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.

[Michael Catanzaro]

"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.

[Alberto Garcia]

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'

[Adrian Perez]

(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.

[Alberto Garcia]

(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

[Alberto Garcia]

(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
)

[Michael Catanzaro]

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.

[Alberto Garcia]

(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?

[Michael Catanzaro]

(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....

[Adrian Perez]

(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.

[Adrian Perez]

(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.

[Michael Catanzaro]

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?

[Alberto Garcia]

(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.

[Alberto Garcia]

(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/

[Alberto Garcia]

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.

[Alberto Garcia]

Plus, the Atomics.cpp file can be removed from the repository (it has already been removed in the master branch).

[Adrian Perez]

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

[Michael Catanzaro]

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.

[Alberto Garcia]

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.

[Adrian Perez]

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”?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alberto Garcia]

Created attachment 340473 [details]
Patch

Ok, here's the updated patch with Adrian's comments

[Adrian Perez]

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.

[Michael Catanzaro]

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.

[Alberto Garcia]

Committed r231843: <https://trac.webkit.org/changeset/231843>

[Radar WebKit Bug Importer]

<rdar://problem/40292317>

[Alberto Garcia]

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.

[Adrian Perez]

(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!

[Carlos Alberto Lopez Perez]

(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.

[Alberto Garcia]

(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

[Michael Catanzaro]

Reverted r231843 for reason:

Broke cross build

Committed r232062: <https://trac.webkit.org/changeset/232062>

[Olivier Blin]

Was it really needed to revert the libatomic change?

It looks like only the pthread change was causing an issue.

[Michael Catanzaro]

Berto can decide how he wants to re-land, I'm just the rollout monkey. ;)

[Alberto Garcia]

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.

[Alberto Garcia]

Committed r232067: <https://trac.webkit.org/changeset/232067>

[Michael Catanzaro]

Berto, this is supposedly FIXED, but you are still carrying a different patch in the Debian package!

[Alberto Garcia]

Only for riscv64, see comment #32

[Adrian Perez]

(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.

[Michael Catanzaro]

If it's not too much code and JSC JIT is disabled, then yes, in a different bug.

[Alberto Garcia]

(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