Bug 155371

Summary: build fails with memory model cannot be stronger than success memory model for ‘__atomic_compare_exchange’
Product: WebKit Reporter: buchner.johannes
Component: Web Template FrameworkAssignee: JF Bastien <jfbastien>
Severity: Normal CC: achristensen, andersca, benjamin, bugs-noreply, cdumez, cmarcelo, commit-queue, darin, dbates, fpizlo, gnome, guijemont, mcatanzaro, saurik, tonikitoo, zan
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Safe workaround, but probably suboptimal.
Test case
mcatanzaro: review-, mcatanzaro: commit-queue-
patch which inlines all atomics instead
test case for gcc 4.9 -Os of Atomic<>::compareExchange{Weak,Strong} none

Description buchner.johannes 2016-03-11 10:41:34 PST
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
Comment 1 Michael Catanzaro 2016-03-11 12:15:11 PST
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.
Comment 2 Guillaume Emont 2016-06-24 17:57:05 PDT
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).
Comment 3 Guillaume Emont 2016-07-01 05:46:39 PDT
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?
Comment 4 buchner.johannes 2016-07-01 05:56:56 PDT
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
Comment 5 Guillaume Emont 2016-07-01 07:15:36 PDT
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.
Comment 6 Guillaume Emont 2016-07-12 10:09:35 PDT
I just tried with gcc 5.4, and it has no problem compiling the test file in -Os.
Comment 7 Guillaume Emont 2016-07-13 10:46:56 PDT
Created attachment 283545 [details]

Proposed patch to work around the issue on gcc <5. Not sure if I should leave the #warning or not.
Comment 8 Guillaume Emont 2016-07-14 02:58:05 PDT
Created attachment 283630 [details]

New version that removes the warning and tries to make clang happy.
Comment 9 Guillaume Emont 2016-07-14 03:19:45 PDT
Created attachment 283631 [details]

Ooops, should have used defined.
Comment 10 Alex Christensen 2016-07-25 09:22:58 PDT
Comment on attachment 283631 [details]

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

> Source/WTF/wtf/Atomics.h:75

This should use the USE(SOMETHING) macro.
Comment 11 Michael Catanzaro 2016-09-08 20:49:59 PDT
Comment on attachment 283631 [details]

Can you respond to Alex's comment?
Comment 12 JF Bastien 2016-09-13 22:13:59 PDT
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.


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:
Comment 13 Geoffrey Garen 2016-09-14 08:59:10 PDT
Comment on attachment 288772 [details]
patch which inlines all atomics instead

Comment 14 WebKit Commit Bot 2016-09-14 10:14:04 PDT
Comment on attachment 288772 [details]
patch which inlines all atomics instead

Clearing flags on attachment: 288772

Committed r205914: <http://trac.webkit.org/changeset/205914>
Comment 15 WebKit Commit Bot 2016-09-14 10:14:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Jay Freeman (saurik) 2016-10-01 17:18:58 PDT
Created attachment 290451 [details]
test case for gcc 4.9 -Os of Atomic<>::compareExchange{Weak,Strong}
Comment 17 Jay Freeman (saurik) 2016-10-01 17:19:52 PDT
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.


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