Using sysroot generates poor code for atomicops. |
|||||||||
Issue descriptionForked 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).
,
Mar 8 2016
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)
,
Mar 9 2016
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.
,
Mar 9 2016
,
Mar 9 2016
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 :-/
,
Mar 9 2016
More and more evidence we should to switch to libc++ on linux too...
,
Mar 9 2016
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.
,
Mar 10 2016
,
Mar 11 2016
,
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?
,
Apr 6 2016
Not surprisingly, I dislike manually patching together sysroots.
,
Apr 6 2016
,
Apr 6 2016
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.
,
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.
,
Apr 6 2016
#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)
,
May 9 2016
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.
,
May 9 2016
#16: IMO updating libstdc++ (at least its atomic header), or migrating to libc++ are a better solution.
,
Jun 12 2016
What's the progress of this? It's blocking a bunch of simplification work.
,
Jun 13 2016
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.
,
Jul 14 2016
,
Jul 19 2016
,
Nov 4 2016
,
Nov 4 2016
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
,
Nov 7 2016
There's lots of progress in upgrading the sysroot (see bug 564904 which blocks that)
,
Feb 17 2017
,
Apr 12 2017
We've made a lot of progress on the sysroot front. e.g. r462181 safely landed recently. Is it time to revisit this?
,
Nov 16 2017
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 |
|||||||||
Comment 1 by jfb@chromium.org
, Mar 8 2016