Bug 155371 – build fails with memory model cannot be stronger than success memory model for ‘__atomic_compare_exchange’

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

When compiling webkit-gtk with the -Os flag, gcc fails with:

FAILED: /usr/bin/x86_64-pc-linux-gnu-g++  -DBUILDING_GTK__=1 -DBUILDING_WITH_CMAKE=1 -DBUILDING_WTF -DDATA_DIR=\"share\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DUSER_AGENT_GTK_MAJOR_VERSION=602 -DUSER_AGENT_GTK_MINOR_VERSION=1 -DWEBKITGTK_API_VERSION_STRING=\"4.0\"   -DENABLE_JIT=0 -DENABLE_YARR_JIT=0 -DENABLE_ASSEMBLER=0 -DNDEBUG -march=native -O2 -pipe -fno-strict-aliasing  -std=c++11 -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/bmalloc -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/dtoa -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/text -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/threads -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/unicode -I/var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/ThirdParty -I. -IDerivedSources -isystem /usr/include/glib-2.0 -isystem /usr/lib64/glib-2.0/include    -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/ParkingLot.cpp.o -MF Source/WTF/wtf/CMakeFiles/WTF.dir/ParkingLot.cpp.o.d -o Source/WTF/wtf/CMakeFiles/WTF.dir/ParkingLot.cpp.o -c /var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/ParkingLot.cpp
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/atomic:41:0,
                 from /var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/Atomics.h:29,
                 from /var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/ParkingLot.h:31,
                 from /var/tmp/portage/net-libs/webkit-gtk-2.10.7/work/webkitgtk-2.10.7/Source/WTF/wtf/ParkingLot.cpp:27:
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/bits/atomic_base.h: In member function ‘void WTF::WordLock::lock()’:
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/bits/atomic_base.h:538:70: error: failure memory model cannot be stronger than success memory model for ‘__atomic_compare_exchange’
  return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 1, __m1, __m2);
                                                                      ^
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/bits/atomic_base.h: In member function ‘void WTF::WordLock::unlock()’:
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/bits/atomic_base.h:538:70: error: failure memory model cannot be stronger than success memory model for ‘__atomic_compare_exchange’
  return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 1, __m1, __m2);
                                                                      ^
ninja: build stopped: subcommand failed.

I can reproduce the bug for webkit-gtk-2.10.4-r1. 

The build with -Os succeeded for webkit-gtk-2.8.5.

This bug has been reported in Gentoo at https://bugs.gentoo.org/show_bug.cgi?id=575388

[Michael Catanzaro]

From the downstream bug, it seems this is affected by the GCC optimization level as it works fine with -O2. All our distributors use -O2 but there's probably value in supporting -OS due to our insane installed size.

[Guillaume Emont]

Created attachment 282039 [details]
Safe workaround, but probably suboptimal.

After a quick inquiry, it seems to me that there is an issue with gcc and/or libstdc++. The relevan code in libstdc++ doesn't look suspicious though, so I'd lean towards an issue with gcc's -Os under some specific conditions that we meet, though I could be wrong. I have tried to make a simple test case that reproduces that to understand better what's up but have not succeeded so far. In the meantime, this seems to work as a workaround (it compiles and looks safe, though locks might get slower than they could because of this).

[Guillaume Emont]

Created attachment 282542 [details]
Test case

I've worked from ParkingLot.cpp, simplifying things as much as possible while still reproducing the bug.
The resulting code probably makes little sense, but I think it should compile. It shows the same error message when compiling with -Os.
The test is only 37 lines of code without dependency on any webkit code. I guess it's starting to look like a gcc or libstdc++ bug?

I think the error is trying to tell you that you are doing something dangerous that you are not supposed to do. 

But do check with the latest gcc, because people reported webkit-gtk builds with gcc-5.3.0

[Guillaume Emont]

the error message is not consistent with what the code is doing though (it does not specify a failure memory model, that one is selected internally by libstdc++). I will try with gcc 5.3 when I find the time, likely in over a week.

[Guillaume Emont]

I just tried with gcc 5.4, and it has no problem compiling the test file in -Os.

[Guillaume Emont]

Created attachment 283545 [details]
Patch

Proposed patch to work around the issue on gcc <5. Not sure if I should leave the #warning or not.

[Guillaume Emont]

Created attachment 283630 [details]
Patch

New version that removes the warning and tries to make clang happy.

[Guillaume Emont]

Created attachment 283631 [details]
Patch

Ooops, should have used defined.

[Alex Christensen]

Comment on attachment 283631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283631&action=review

> Source/WTF/wtf/Atomics.h:75
> +#if defined(ATOMICS_ALWAYS_USE_SEQ_CST)

This should use the USE(SOMETHING) macro.

[Michael Catanzaro]

Comment on attachment 283631 [details]
Patch

Can you respond to Alex's comment?

[JF Bastien]

Created attachment 288772 [details]
patch which inlines all atomics instead

I don't think this is the right fix. I've attached a different one. TL;DR the quick fix is to make these wrapper function ALWASY_INLINE.

Details:

I tested with a few GCC versions, and the problem is specific to pre-5 revisions of GCC at Os only. The reason is that libstdc++ tries to be clever about enforcing the C++ standard's clause [atomics.types.operations.req] ¶21 which states:

    Requires: The failure argument shall not be `memory_order_release` nor
    `memory_order_acq_rel`. The failure argument shall be no stronger than the
    success argument.

It fails at doing this because its inlining heuristics are modified by Os, and they're not quite as dumb as O0 but not smart enough to get to the good code at O1. Adding ALWAYS_INLINE fixes the silliness at Os, leaves O1 great, and makes O0 slightly less bad but still pretty bad.

Try it out here: https://godbolt.org/g/8FPxWt


The other good news is that I'm going to get this particular problem fixed in the version of C++ which will come after C++17:
https://github.com/jfbastien/papers/blob/master/source/P0418r1.bs

[Geoffrey Garen]

Comment on attachment 288772 [details]
patch which inlines all atomics instead

r=me

[WebKit Commit Bot]

Comment on attachment 288772 [details]
patch which inlines all atomics instead

Clearing flags on attachment: 288772

Committed r205914: <http://trac.webkit.org/changeset/205914>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jay Freeman (saurik)]

Created attachment 290451 [details]
test case for gcc 4.9 -Os of Atomic<>::compareExchange{Weak,Strong}

[Jay Freeman (saurik)]

Did you actually test compiling WebKit using this patch, or did you just use that website to verify against the limited test case? I ask, because I was just compiling WebKit using -Os on gcc 4.9, this patch is already included in the code I'm using, and it still doesn't compile with the same error message. I have whittled down a slightly longer test case that demonstrates that using ALWAYS_INLINE on the WTF atomics is not sufficient.

Essentially, with more callers with different arguments (including multiple instantiations with different template arguments), and using both the strong and weak variants in the same translation unit, gcc decides the code for __cmpexch_failure_order (which is constexpr but not even marked inline much less _GLIBCXX_ALWAYS_INLINE) has been duplicated too many times and it decides to stop inlining it, leading to the usage error.

https://godbolt.org/g/X3mWxp

(Sorry, btw: this is the first time I have attached a file to this bug tracker and I didn't realize quite how to get the comment and the test case to be included together, so the attachment was already sent separately.)