New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 592903 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 430105
issue 593874

Blocking:
issue 600271
issue 693244



Sign in to add a comment

Using sysroot generates poor code for atomicops.

Project Member Reported by amistry@chromium.org, Mar 8 2016

Issue description

Forked from  crbug.com/423074 

OS: Ubuntu 14.04
Arch: x86-64

GN config:
is_debug = false

I built base/atomics_unittest.cc without sysroot:
third_party/llvm-build/Release+Asserts/bin/clang++ -c base/atomicops_unittest.cc -o foo.o -I. -std=gnu++11 -Itesting/gtest/include -Itesting/gmock/include -O2

which gives me:
0000000000013340 <AtomicOpsTest_Load_Test::TestBody()>:
   13340:       41 56                   push   %r14
   13342:       53                      push   %rbx
   13343:       48 81 ec f8 01 00 00    sub    $0x1f8,%rsp
   1334a:       c7 44 24 10 a5 a5 a5    movl   $0xa5a5a5a5,0x10(%rsp)
   13351:       a5 
   13352:       c7 44 24 14 ff ff ff    movl   $0xffffffff,0x14(%rsp)
   13359:       ff 
   1335a:       c7 44 24 0c a5 a5 a5    movl   $0xa5a5a5a5,0xc(%rsp)
   13361:       a5 
   13362:       8b 44 24 0c             mov    0xc(%rsp),%eax
   13366:       89 84 24 c4 01 00 00    mov    %eax,0x1c4(%rsp)
   1336d:       48 8d bc 24 e0 00 00    lea    0xe0(%rsp),%rdi
   13374:       00 
   13375:       3d a5 a5 a5 a5          cmp    $0xa5a5a5a5,%eax

And with sysroot:
third_party/llvm-build/Release+Asserts/bin/clang++ -c base/atomicops_unittest.cc -o foo.o -I. -std=gnu++11 -Itesting/gtest/include -Itesting/gmock/include -O2 --sysroot=build/linux/debian_wheezy_amd64-sysroot

I get:
00000000000132c0 <AtomicOpsTest_Load_Test::TestBody()>:
   132c0:       41 56                   push   %r14
   132c2:       53                      push   %rbx
   132c3:       48 81 ec f8 01 00 00    sub    $0x1f8,%rsp
   132ca:       c7 44 24 10 a5 a5 a5    movl   $0xa5a5a5a5,0x10(%rsp)
   132d1:       a5 
   132d2:       c7 44 24 14 ff ff ff    movl   $0xffffffff,0x14(%rsp)
   132d9:       ff 
   132da:       c7 44 24 0c a5 a5 a5    movl   $0xa5a5a5a5,0xc(%rsp)
   132e1:       a5 
   132e2:       0f ae f0                mfence 
   132e5:       8b 44 24 0c             mov    0xc(%rsp),%eax
   132e9:       0f ae f0                mfence 
   132ec:       89 84 24 c4 01 00 00    mov    %eax,0x1c4(%rsp)
   132f3:       39 44 24 10             cmp    %eax,0x10(%rsp)

Same behaviour when I change the use_sysroot variable in GN. Performance is terrible (one perf test was consuming ~22% of CPU time in LazyInstance because of the fences).
 

Comment 1 by jfb@chromium.org, Mar 8 2016

Cc: jfb@chromium.org
Which compiler version and C++ standard library version are in the sysroot?
I'm building with clang, but it looks like there's gcc 4.6 (and presumably the corresponding libstdc++) in the sysroot.
~/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot$ ./usr/bin/gcc-4.6 -v
Using built-in specs.
COLLECT_GCC=./usr/bin/gcc-4.6
COLLECT_LTO_WRAPPER=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/bin/../lib/gcc/x86_64-linux-gnu/4.6/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.6.3-14' --with-bugurl=file:///usr/share/doc/gcc-4.6/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.6 --enable-shared --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.6 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --enable-plugin --enable-objc-gc --with-arch-32=i586 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.6.3 (Debian 4.6.3-14)


I found this nugget of code in the c++ headers:
      __int_type
      load(memory_order __m = memory_order_seq_cst) const
      {
        __glibcxx_assert(__m != memory_order_release);
        __glibcxx_assert(__m != memory_order_acq_rel);

        __sync_synchronize();
        __int_type __ret = _M_i;
        __sync_synchronize();
        return __ret;
      }

I guess the sysroot needs a newer version of gcc.
Cc: h...@chromium.org thakis@chromium.org
 Issue 593344  has been merged into this issue.
Fun fact, I stumbled upon the same extact issue today.
See  Issue 593344 , TL;DR I narrowed down and came to the same conclusions of this bug (it's the sysroot)

This has some pretty visible effect, in fact I ended up doing all this starting from a benchmark. Essentially makes base::subtle::NoBarrier* a lie :-/


More and more evidence we should to switch to libc++ on linux too...
At the very least, not have an ancient version of GCC/libstc++ in the sysroot. Support for the c++11 memory model was added in 4.7.

Comment 8 by thakis@chromium.org, Mar 10 2016

Blockedon: 593874
Cc: chrishall@chromium.org

Comment 10 by jfb@chromium.org, Apr 5 2016

As a short-term fix before we move to libc++, can we swap a more recent libstdc++ *header* implement of atomics into the sysroot?

The point of the old sysroot being old is being ABI compatible with older distros, that means old libc and old libstdc++.so, but the atomics headers are just headers and only rely on runtime support for non-lock-free. The non-lock-free support is stable, so we could update these headers and stop paying the cost.

WDYT?
Not surprisingly, I dislike manually patching together sysroots.
Blocking: 600271
My $0.02: if we are really willing to make a short-term fix I'd rather make it (some #ifdef) in base/atomicops.h where it is more visible/auditable. Not sure how much use of std::atomic we have outside of base, given that std::atomics are still in the banned features list.

Comment 14 by jfb@chromium.org, Apr 6 2016

#13 you mean __atomic_* intrinsics directly?

I can do that if people agree with the approach, and then when we have libc++ go to true std::atomic at each place atomicops are currently used (one at a time, fixing usage as I go).

That way atomicops would always be a hack, but we'd remove its usage over time.
#14 Not sure I fully understand the question (I guess I am missing some bit of knowledge here).
My understanding of the situation is that due to the sysroot issue, std::atomic::load/store(..., std::memory_order_relaxed), that is we use in base/atomicops_internals_portable.h to implement NoBarrier_, is not really relaxed on Linux.
The proposal was to make base/atomicops_internals_portable.h something like:

#ifdef HAVE_BROKEN_SYSROOT
 really_chill_out_read_bro(...)
#else
 ...->load(..., std::memory_order_relaxed)
#endif

Now, my point in this regard was: I was hoping we wouldn't be using std::atomic::load/store(..., std::memory_order_relaxed) outside of base/atomicops.h as <atomic> (where load and memory_order_relaxed are defined) is a banned feature in https://chromium-cpp.appspot.com/ (maybe I am misunderstanding the ban?).

On the other side, codesearch suggests that:
- Skia has it's own layer wrapping std::atomic in skia/include/private/SkAtomics.h
- base/metrics/persistent_memory_allocator.cc is using std::atomic
- ditto for third_party/WebKit/Source/wtf/SpinLock.cpp
- ditto for src/third_party/leveldatabase/src/port/atomic_pointer.h (didn't check if we are actually using this part of the header)

Is there any update to this? If not, I'd like to propose a temporary hack, which is to restore the gcc-specific atomics for libstdc++. If folks aren't thoroughly disgusted by this idea, I have a CL prepared.

Comment 17 by jfb@chromium.org, May 9 2016

#16: IMO updating libstdc++ (at least its atomic header), or migrating to libc++ are a better solution.
What's the progress of this? It's blocking a bunch of simplification work.
It's blocked on  bug 593874 . Once that is done, this here will be fixed too.  Bug 593874  just needs doing -- we did the same on OS X already.
Components: Build
Labels: Build
Cc: esprehn@chromium.org
Blockedon: 430105
Cc: jochen@chromium.org hpayer@chromium.org
given that there's only very slow progress towards upgrading the sysroot (see the referenced bug for the main blocker there), I'd propose to go back to the pre-C++11 headers.

When we tried to switch V8, we observed massive performance regressions due to the outdated sysroot
There's lots of progress in upgrading the sysroot (see bug 564904 which blocks that)
Blocking: 693244
We've made a lot of progress on the sysroot front. e.g. r462181 safely landed recently. Is it time to revisit this?
Status: Fixed (was: Untriaged)
IIUC, the issue doesn't reproduce when using the Ubuntu Trusty headers, but it did when we were using the (older) wheezy sysroot.

We've since updated to jessie (which has newer gcc headers than Trusty), and now stretch (even newer).  We've also switched away from using libstdc++ entirely and are now using libc++, so we shouldn't even be using libstdc++ headers any more.

For these reasons, I'm closing this issue out.

Sign in to add a comment