| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This CL moves the Chrome code base to C++11 atomics instead of inline
assembly for the toolchains that support C++11's <atomic>. The patch is
constructed to modify as little code as possible, and will be followed-up with
other patches to clean up the rest of the code base.
This change should allow compilers to emit better code when dealing with atomics
(LLVM has recently seen substantial improvements thanks to morisset's work), be
more portable, eventually delete a bunch of code, and in subsequent changes
atomicity will be based on memory location instead of individual accesses (making
the code less error-prone, and easier to analyze).
This patch stems from a fix I recently made to V8 to build under NaCl and
PNaCl. This was gating V8's C++11 migration: they needed to move to the
LLVM-based NaCl toolchain which didn't work with atomicops' inline assembly. V8
currently uses the __sync_* primitives for the NaCl/PNaCl build, which imply
sequential consistency and are therefore suboptimal. Before doing further work I
want to fix Chrome's own headers, and trickle these changes to V8.
Future changes:
* The atomicops headers are copied in 8 locations [0] in the Chrome code base,
not all of them are up to date. Those should all be updated in their
individual repositories (some are in thiry_party).
* The signature of functions should be updated to take an atomic location
instead of the pointer argument, and all call sites should be updated (there
are current 127 such calls [1] in chromium/src in non-implementation and
non-test files).
* Atomic operations should be used directly.
A few things worth noting:
* The atomicops_internals_portable.h file contains the main implementation, and
has extra notes on implementation choices.
* The CL removes x86 CPU features detection from the portable implementation:
- Because the headers are copied in a few places and the x86 CPU feature
detection uses a static global that parses cpuid, the
atomicops_internals_x86_gcc.cc file was only built once, but its struct
interface was declared external and used in protobuf and
tcmalloc. Removing the struct from Chrome's own base makes the linker sad
because of the two uses. This has two implications:
# Don't specialize atomics for SSE2 versus non SSE2. This is a non-issue
since Chrome requires a minimum of SSE2. The code is therefore faster
(skip a branch).
# The code doesn't detect the AMD Opteron Rev E memory barrier bug from
AMD errata 147 [2]. This bug affects Opterons 1xx/2xx/8xx that shipped
in 2003-2004. In general compilers should work around this bug, not
library code. GCC has had this workaround since 2009 [3], but it is
still an open bug in LLVM [4]. See comment #27 on the code review: this
CPU is used by 0.02% of Chrome users, for whom the race would have to
occur and the compiler not have the workaround for the bug to manifest.
This also makes the code faster by skipping a branch.
* The CL moves the declaration of the x86 CPU features struct to atomicops.h,
and matches its fields with the ones duplicated across the code base. This
is transitional: it fixes a link failure because tcmalloc relied on the
x86_gcc.{h,cc} declaration and implementation, and it fixes an ODR violation
where the struct didn't always have 3 members (and the other members were
sometimes accessed, uninitialized).
* tsan already rewrites all atomics to its own primitives, its header is
therefore now unnecessary. The implementation takes care to detect and error
out if that turns out not to be true.
* MemoryBarrier works around a libstdc++ bug in older versions [5]. The
workaround is exactly the non-buggy code for libstdc++ (call out to the
compiler's builtin).
* The assembly generated by GCC 4.8 and LLVM 3.6 for x86-32/x86-64/ARM when
using this portable implementation can be found in [6].
[0]: find . -name "atomicops.h"
./base/atomicops.h
./v8/src/base/atomicops.h
./native_client_sdk/src/libraries/sdk_util/atomicops.h
./third_party/webrtc/base/atomicops.h
./third_party/tcmalloc/chromium/src/base/atomicops.h
./third_party/tcmalloc/vendor/src/base/atomicops.h
./third_party/re2/util/atomicops.h
./third_party/protobuf/src/google/protobuf/stubs/atomicops.h
[1]: git grep Barrier_ | grep -v atomicops | grep -v unittest | wc -l
[2]: http://support.amd.com/us/Processor_TechDocs/25759.pdf
[3]: https://gcc.gnu.org/ml/gcc-patches/2009-10/txt00046.txt
[4]: http://llvm.org/bugs/show_bug.cgi?id=5934
[5]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51038
[6]: https://code.google.com/p/chromium/issues/detail?id=423074
TEST=ninja -C out/Release base_unittests
TEST=trybots
BUG=246514
BUG=234215
BUG=420970
BUG=423074
R= dvyukov@google.com, thakis@chromium.org, glider@chromium.org, hboehm@google.com, morisset@google.com, willchan@chromium.org
Review URL: https://codereview.chromium.org/636783002
Cr-Commit-Position: refs/heads/master@{#299485}
|
