Use C++11 atomics

Use C++11 atomics

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

Committed: https://crrev.com/57a4e4a50c673c25e9cdaab53e32f6e53aa0b574
Cr-Commit-Position: refs/heads/master@{#299485}

[JF, 2014-10-07 05:05:54]

jfb@chromium.org changed reviewers:
+ hboehm@google.com, morisset@google.com

[JF, 2014-10-07 05:05:55]

I build a shed for atomic bikes. Please add appropriate reviewers.

A few things need to be discussed:
 * Will the compilers that Chrome supports all accept this code?
 * Is the migration plan I propose acceptable?
 * Which performance measurements are appropriate to conduct to ensure
performance doesn't regress?
 * What should be done about the AMD Opteron rev E bug?
 * There's currently an issue when linking with NaCl enabled (the IRT can't find
atomic_thread_fence). I'll look into it.

[JF, 2014-10-07 05:30:51]

jfb@chromium.org changed reviewers:
+ rnk@chromium.org

[JF, 2014-10-07 05:30:51]

+rnk for Windows/MSVC expertise.

[Alexander Potapenko, 2014-10-07 06:58:32]

General comments:

I think you'd better keep base/atomicops_internals* for now and just create
atomicops_internals_portable.h in addition to other options.
This would make the patch easier to review and won't break compilation on
platforms where __cplusplus < 201103L.

I also suggest you change the atomicops implementation for base/ only.
Changes to third_party should normally go to the corresponding upstream projects
and merged back to Chromium.
At least this could be done separately, because atomicops implementations in
subprojects are independent from base.
(Speaking about tcmalloc, please do not touch third_party/tcmalloc/vendor, which
is a verbatim copy of the upstream repo).

[Alexander Potapenko, 2014-10-07 07:32:46]

I was about to suggest converting the existing base/atomicops.h API users to use
<atomic> instead of reimplementing base/atomicops.h with C++11 atomics.
However this doesn't sound feasible, because according to
http://chromium-cpp.appspot.com/ "All C++11 Standard Library features are
currently banned, because they are not supported on all of our toolchains yet."
So it looks like the transitional implementation suggested by JF is the right
thing to do for now.

[Dmitry Vyukov, 2014-10-07 07:48:46]

please svn copy atomicops_internals_x86_gcc.h atomicops_internals_portable.h
this will substantially simplify diff verification

[Dmitry Vyukov, 2014-10-07 08:07:04]

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:19: //    only valid under certain uses of
compare exchange.
This is not correct.
failure order acquire is illegal when success order is release

see 29.6.5/20:

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.

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:21: //    implemented as a acquire exchange.
This is not correct.
When it comes to these weird acquire_store and release_load, there is no correct
mapping between atomicops <-> C/C++ atomics other than usage of seq_cst for
*all* operations (including NoBarrier ones). However such mapping is too
expensive. The most reasonable mapping that should (hopefully) work in practice
is to implement acquire store as:
store(relaxed)
thread_fence(seq_cst)
and release load as:
thread_fence(seq_cst)
load(relaxed)

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:22: //  * Release load doesn't exist in the
C11 memory model, it is instead
This is not correct.
see comment for acquire store

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:26: //    needs to increment twice (which
the compiler should be able to detect and
how should a compiler optimize it? what code should it generate?
do gcc/clang actually generate optimized code? if not, please file a bug on gcc
and clang.

[Nico, 2014-10-07 08:25:46]

How's this supposed to work? We don't have C++11 library features. This won't
compile on OS X and android, no?

[morisset, 2014-10-07 18:27:44]

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:26: //    needs to increment twice (which
the compiler should be able to detect and
On 2014/10/07 08:07:04, Dmitry Vyukov wrote:
> how should a compiler optimize it? what code should it generate?
> do gcc/clang actually generate optimized code? if not, please file a bug on
gcc
> and clang.

On ARM/Power, it is possible to only do the addition once, it is just a matter
of returning the value that has been stored by the store-conditional instead of
the value that has been loaded. At least clang appears to do it.

[hboehm, 2014-10-07 18:57:02]

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:19: //    only valid under certain uses of
compare exchange.
On 2014/10/07 08:07:04, Dmitry Vyukov wrote:
> This is not correct.
> failure order acquire is illegal when success order is release
> 
> see 29.6.5/20:
> 
> 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.
Dmitry raises an interesting question, which should probably be clarified in the
standard.  I think his interpretation is probably consistent with the intent,
but it's unclear to me whether "memory_order_acquire is no stronger than
memory_order_release".  One could argue that they're clearly incomparable, and
therefore neither is stronger than the other.

https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:21: //    implemented as a acquire exchange.
On 2014/10/07 08:07:04, Dmitry Vyukov wrote:
> This is not correct.
> When it comes to these weird acquire_store and release_load, there is no
correct
> mapping between atomicops <-> C/C++ atomics other than usage of seq_cst for
> *all* operations (including NoBarrier ones). However such mapping is too
> expensive. The most reasonable mapping that should (hopefully) work in
practice
> is to implement acquire store as:
> store(relaxed)
> thread_fence(seq_cst)
> and release load as:
> thread_fence(seq_cst)
> load(relaxed)
> 
I've encountered similar issues in Android.  As far as I can tell, there are
primarily two classes of use cases for these:

1) Seqlocks (reader side for release-load, single writer for acquire store).  I
think in those cases an acquire fence followed by a relaxed load, or a relaxed
store, followed by a release fence, are appropriate.

2) Essentially as a replacement for a seq_cst fence, sometimes in situations in
which the construct is clearly abused.

I think Dmitry's mapping is reasonably safe, and the one I decided on in
Android.  But the seqlock uses should eventually be fixed.

[hboehm, 2014-10-07 19:03:45]

On 2014/10/07 08:25:46, Nico (away until Oct 27) wrote:
> How's this supposed to work? We don't have C++11 library features. This won't
> compile on OS X and android, no?

We're trying to support <stdatomic.h> and <atomic> on Android.  I've been trying
to move over some internal Android code.  There are currently still issues.  So
long as you are using libc++ and not stlport, things should be somewhat usable.

What are you guys doing about mixing <stdatomic.h> and <atomic>?  The android
<stdatomic.h> just includes <atomic> and promotes various identifiers to global
namespace when it detects it can.  We decided to do that after some false starts
with the two stepping on each other.

[Nico, 2014-10-07 22:04:32]

Android's currently using stlport, not libc++. That's what I meant above.

[chromium-reviews, 2014-10-07 23:49:44]

FWIW, the intent is to at least make bionics stdatomic.h usable within
Android in that context.  And we are using it in some places, but there
still seem to be issues in others.

On Tue, Oct 7, 2014 at 3:04 PM, <thakis@chromium.org> wrote:

> Android's currently using stlport, not libc++. That's what I meant above.
>
> https://codereview.chromium.org/636783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[JF, 2014-10-08 03:28:49]

Thanks for the comments. I answered everyone's comments below,
please take a look at the updated code.


On 2014/10/07 06:58:32, Alexander Potapenko wrote:
> I think you'd better keep base/atomicops_internals* for now and just create
> atomicops_internals_portable.h in addition to other options.
> This would make the patch easier to review and won't break compilation on
> platforms where __cplusplus < 201103L.

This is addressed by patch set 2, which otherwise has no
functional changes from patch set 1.

It resolves the linking issue with AtomicOps_x86CPUFeatureStruct
by moving its declaration and definition around. Note that the
code was, and still is, unsound because it violates ODR:
different parts of the code base have different definitions of
this struct. Some have a single member has_amd_lock_mb_bug,
others have has_sse2, and others have has_cmpxchg16b. This means
that the code sometimes reads out of bounds of the struct. I'll
fix this issue in a subsequent patch set, in this patch, by
adding the other 2 members to the declaration and definition in
base.


> I also suggest you change the atomicops implementation for base/ only.
> Changes to third_party should normally go to the corresponding upstream
projects
> and merged back to Chromium.
> At least this could be done separately, because atomicops implementations in
> subprojects are independent from base.
> (Speaking about tcmalloc, please do not touch third_party/tcmalloc/vendor,
which
> is a verbatim copy of the upstream repo).

Understood, I think patch set 2 does what you want.


On 2014/10/07 07:48:46, Dmitry Vyukov wrote:
> please svn copy atomicops_internals_x86_gcc.h atomicops_internals_portable.h
> this will substantially simplify diff verification

I tried this for patch set 2 onwards. My git repo shows the right
thing, but the code review site doesn't. I'm not sure how to make
this better :-(


On 2014/10/07 08:07:04, Dmitry Vyukov wrote:
>
https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
> base/atomicops_internals_portable.h:19: //    only valid under certain uses of
> compare exchange.
> This is not correct.
> failure order acquire is illegal when success order is release
> 
> see 29.6.5/20:
> 
> 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.

Ha, I hadn't realized this. I thought acquire < relaxed and would
simply lead to a different fence placement. Updated to relaxed in
patch set 3.


>
https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
> base/atomicops_internals_portable.h:21: //    implemented as a acquire
exchange.
> This is not correct.
> When it comes to these weird acquire_store and release_load, there is no
correct
> mapping between atomicops <-> C/C++ atomics other than usage of seq_cst for
> *all* operations (including NoBarrier ones). However such mapping is too
> expensive. The most reasonable mapping that should (hopefully) work in
practice
> is to implement acquire store as:
> store(relaxed)
> thread_fence(seq_cst)
> and release load as:
> thread_fence(seq_cst)
> load(relaxed)

I discussed this with hboehm and agree with your assessment that
what I propose isn't generally correct. I'll have to update
individual call sites later because their assumptions of what
this does may vary. Patch set 3 contains fixes as you suggested.

glider filed a bug about acquire store and release load:
  https://code.google.com/p/chromium/issues/detail?id=420970

hboehm points out that the fence could also be acquire or release
under some use cases.


On 2014/10/07 18:27:44, morisset wrote:
>
https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
> File base/atomicops_internals_portable.h (right):
> 
>
https://codereview.chromium.org/636783002/diff/1/base/atomicops_internals_por...
> base/atomicops_internals_portable.h:26: //    needs to increment twice (which
> the compiler should be able to detect and
> On 2014/10/07 08:07:04, Dmitry Vyukov wrote:
> > how should a compiler optimize it? what code should it generate?
> > do gcc/clang actually generate optimized code? if not, please file a bug on
> gcc
> > and clang.
> 
> On ARM/Power, it is possible to only do the addition once, it is just a matter
> of returning the value that has been stored by the store-conditional instead
of
> the value that has been loaded. At least clang appears to do it.

For his internship in the PNaCl team morisset has implemented
quite a few optimizations on atomics in LLVM when targeting
x86/ARM/Power. Whatever he doesn't have time to finish will have
bugs filed in LLVM's bug tracker, and we'll follow up with GCC
folks to make sure they're aware of the optimizations.

He already implemented the add zero optimization for x86 (hboehm
has a paper about it), but there's currently no proof of its
correctness for ARM and Power so he's held off. He has a todo on
optimizing exchanges that ignore their return value.


On 2014/10/07 08:25:46, Nico (away until Oct 27) wrote:
> How's this supposed to work? We don't have C++11 library features. This won't
> compile on OS X and android, no?

The implementation is tied to specific compilers and
architectures, so it will work properly. I'm trying to cast a
wide net early on, but as hboehm pointed out it'll be tricky to
make everything work. I'm relying on trybots to tell me when I
mess up for certain targets and I'll restrict targets from this.

Initially I want to capture toolchains that declare C++11
support. If it turns out that they lie then I'll rely on feature
detection through __has_include and __has_feature, and I can also
rely on the __atomic_* builtins if need be.

Atomics aren't really a library feature ;-)

[Nico, 2014-10-08 07:17:57]

> Initially I want to capture toolchains that declare C++11
> support.

All of our toolchains declare C++11 language support.

> If it turns out that they lie then I'll rely on feature
> detection through __has_include and __has_feature, and I can also
> rely on the __atomic_* builtins if need be.
> 
> Atomics aren't really a library feature ;-)

In what sense? They are in the <atomics> header, which doesn't exist everywhere.

[Dmitry Vyukov, 2014-10-08 08:05:44]

https://codereview.chromium.org/636783002/diff/60001/base/atomicops.h
File base/atomicops.h (left):

https://codereview.chromium.org/636783002/diff/60001/base/atomicops.h#oldcode142
base/atomicops.h:142: #include "base/atomicops_internals_tsan.h"
__cplusplus is not necessary defined under tsan
It merely depends on target file -- C or C++
So if atomicops.h is included into a C file, tsan will miss atomics there. Not
good.
You need to restore this part as well.

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:9: // written assuming atomicity revolves
around accesses instead of C++11's memory
I don't understand the "because" part.

You still can do:

  69   return ((AtomicLocation32)ptr)->exchange(new_value,
std::memory_order_relaxed);

instead of:

  69   return std::atomic_exchange_explicit(
  70       (AtomicLocation32)ptr, new_value, std::memory_order_relaxed);

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:60: &expected,
remove expected, just do &old_value

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:70: (AtomicLocation32)ptr, new_value,
std::memory_order_relaxed);
I would use member functions, because they are shorter:

return ((AtomicLocation32)ptr)->exchange(new_value, std::memory_order_relaxed);

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:90: &expected,
remove expected, use &old_value

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:102: &expected,
remove expected, use &old_value
here and below

https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
base/atomicops_internals_portable.h:143: // See the 32-bit versions for
comments.
there are no comments on 32-bit versions

[Alexander Potapenko, 2014-10-08 08:56:00]

Please don't forget to update the CL description.

[JF, 2014-10-08 17:07:12]

Updated, please take a look. I still have to:
 * Handle Android and Mac properly (see reply to vyukov below).
 * Work around the linker issue with atomic_thread_fence not
   being present. It looks like a libstdc++ bug:
     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51038
   I can patch NaCl's version, but I'm not sure how to patch the
   version thakis ships with Chrome.


On 2014/10/08 07:17:57, Nico (away until Oct 27) wrote:
> > Atomics aren't really a library feature ;-)
> 
> In what sense? They are in the <atomics> header, which doesn't exist
everywhere.

All of the atomic support is implemented through compiler magic
and cannot be implemented using the language. The library side
makes this compiler magic usable, and makes it so that language
support doesn't need to be added.

The important thing to realize is that atomicops.h can use
atomics when the toolchain supports it, and fall back
otherwise. The incremental plan I propose will eventually make
the code use C++11 atomics when they're available, and look
pretty much as it does today when they're not available.


On 2014/10/08 08:05:44, Dmitry Vyukov wrote:
> __cplusplus is not necessary defined under tsan
> It merely depends on target file -- C or C++
> So if atomicops.h is included into a C file, tsan will miss atomics there. Not
> good.
> You need to restore this part as well.

I don't object to restoring the tsan code if it's required, but I
don't think it is: atomicops.h uses namespaces so it cannot be
included by C code.

The bots tell me I'll either have to change inclusion order for
Android and Mac, or use __atomic_* and feature detection. If I do
the former then I'll need to restore tsan header, but if I do the
latter then I can leave tsan out (since it'll understand
__atomic_*).


>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
> File base/atomicops_internals_portable.h (right):
> 
>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
> base/atomicops_internals_portable.h:9: // written assuming atomicity revolves
> around accesses instead of C++11's memory
> I don't understand the "because" part.
> 
> You still can do:
> 
>   69   return ((AtomicLocation32)ptr)->exchange(new_value,
> std::memory_order_relaxed);
> 
> instead of:
> 
>   69   return std::atomic_exchange_explicit(
>   70       (AtomicLocation32)ptr, new_value, std::memory_order_relaxed);

The memory locations that the users treat atomically are just
int32_t. Having the implementation cast to an atomic type doesn't
change this, and it's technically incorrect though it works in
practice (primarily because tsan prevents the user code from
accessing the memory location through non-atomicops.h
functions). The patch that'll follow this one will change the API
for atomicops.h so that the locations have a different (atomic)
type than the value parameters, which will make this comment
obsolete. If you'd rather have me delete this comment now I can
do so instead of waiting until my second patch, I added a TODO
for now.


>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
> base/atomicops_internals_portable.h:60: &expected,
> remove expected, just do &old_value

Done (here and below).


>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
> base/atomicops_internals_portable.h:70: (AtomicLocation32)ptr, new_value,
> std::memory_order_relaxed);
> I would use member functions, because they are shorter:
> 
> return ((AtomicLocation32)ptr)->exchange(new_value,
std::memory_order_relaxed);

Done.


>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
> base/atomicops_internals_portable.h:143: // See the 32-bit versions for
> comments.
> there are no comments on 32-bit versions

Removed.


On 2014/10/08 08:56:00, Alexander Potapenko wrote:
> Please don't forget to update the CL description.

Done.

I haven't received guidance concerning the ARM Opteron memory
barrier bug. Are folks are OK with the current
implementation (expect the compiler to work around the issue)? If
so I'll update the CL description.

[chromium-reviews, 2014-10-08 17:27:56]

On Wed, Oct 8, 2014 at 9:07 PM,  <jfb@chromium.org> wrote:
> Updated, please take a look. I still have to:
>  * Handle Android and Mac properly (see reply to vyukov below).
>  * Work around the linker issue with atomic_thread_fence not
>    being present. It looks like a libstdc++ bug:
>      https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51038
>    I can patch NaCl's version, but I'm not sure how to patch the
>    version thakis ships with Chrome.
>
>
> On 2014/10/08 07:17:57, Nico (away until Oct 27) wrote:
>>
>> > Atomics aren't really a library feature ;-)
>
>
>> In what sense? They are in the <atomics> header, which doesn't exist
>
> everywhere.
>
> All of the atomic support is implemented through compiler magic
> and cannot be implemented using the language. The library side
> makes this compiler magic usable, and makes it so that language
> support doesn't need to be added.
>
> The important thing to realize is that atomicops.h can use
> atomics when the toolchain supports it, and fall back
> otherwise. The incremental plan I propose will eventually make
> the code use C++11 atomics when they're available, and look
> pretty much as it does today when they're not available.
>
>
> On 2014/10/08 08:05:44, Dmitry Vyukov wrote:
>>
>> __cplusplus is not necessary defined under tsan
>> It merely depends on target file -- C or C++
>> So if atomicops.h is included into a C file, tsan will miss atomics there.
>> Not
>> good.
>> You need to restore this part as well.
>
>
> I don't object to restoring the tsan code if it's required, but I
> don't think it is: atomicops.h uses namespaces so it cannot be
> included by C code.

Then please remove the confusing:
#if defined(__cplusplus)


> The bots tell me I'll either have to change inclusion order for
> Android and Mac, or use __atomic_* and feature detection. If I do
> the former then I'll need to restore tsan header, but if I do the
> latter then I can leave tsan out (since it'll understand
> __atomic_*).
>
>
>
>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
>>
>> File base/atomicops_internals_portable.h (right):
>
>
>
>
https://codereview.chromium.org/636783002/diff/60001/base/atomicops_internals...
>>
>> base/atomicops_internals_portable.h:9: // written assuming atomicity
>> revolves
>> around accesses instead of C++11's memory
>> I don't understand the "because" part.
>
>
>> You still can do:
>
>
>>    69   return ((AtomicLocation32)ptr)->exchange(new_value,
>> std::memory_order_relaxed);
>
>
>> instead of:
>
>
>>    69   return std::atomic_exchange_explicit(
>>    70       (AtomicLocation32)ptr, new_value, std::memory_order_relaxed);
>
>
> The memory locations that the users treat atomically are just
> int32_t. Having the implementation cast to an atomic type doesn't
> change this, and it's technically incorrect though it works in
> practice (primarily because tsan prevents the user code from
> accessing the memory location through non-atomicops.h
> functions). The patch that'll follow this one will change the API
> for atomicops.h so that the locations have a different (atomic)
> type than the value parameters, which will make this comment
> obsolete. If you'd rather have me delete this comment now I can
> do so instead of waiting until my second patch, I added a TODO
> for now.

I understand this, I just don't see relation between the type problem
and usage of standalone versus member functions.
The comment looks good now.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dmitry Vyukov, 2014-10-08 17:28:13]

LGTM FWIW
wait for somebody from chromium as well

[Alexander Potapenko, 2014-10-08 17:39:36]

Nico is away, so you'll need a base/ owner.

https://codereview.chromium.org/636783002/diff/80001/base/atomicops_internals...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/80001/base/atomicops_internals...
base/atomicops_internals_portable.h:61: ->compare_exchange_strong(old_value,
That's a strange way to wrap "->". Did clang-format do this?

[JF, 2014-10-08 17:45:38]

> Then please remove the confusing:
> #if defined(__cplusplus)

Oh that's not to detect whether we're compiling C++ or not: it's to
detect whether we're compiling with a C++ compiler that has the
__cplusplus macro. I type this automatically, but I agree that it's
not required here: we're looking for C++11 and later, non conformant
compilers will have bigger troubles. I'll remove.


> I understand this, I just don't see relation between the type problem
> and usage of standalone versus member functions.
> The comment looks good now.

Agreed it's somewhat moot since all compilers I know currently represent
atomics around accesses and discard most memory location information in
the front-end. I think there are still implications for TBAA.


On 2014/10/08 17:39:36, Alexander Potapenko wrote:
> Nico is away, so you'll need a base/ owner.

Will do once tests pass.


>
https://codereview.chromium.org/636783002/diff/80001/base/atomicops_internals...
> File base/atomicops_internals_portable.h (right):
> 
>
https://codereview.chromium.org/636783002/diff/80001/base/atomicops_internals...
> base/atomicops_internals_portable.h:61: ->compare_exchange_strong(old_value,
> That's a strange way to wrap "->". Did clang-format do this?

Yes.

[Alexander Potapenko, 2014-10-08 17:46:04]

> The bots tell me I'll either have to change inclusion order for
> Android and Mac, or use __atomic_* and feature detection. If I do
> the former then I'll need to restore tsan header, but if I do the
> latter then I can leave tsan out (since it'll understand
> __atomic_*).
Can't you just explicitly list the platforms (or the toolchains, because for
Linux GCC and Clang may behave differently) that support <atomic> in the #ifdef
branch corresponding to atomicops_internals_portable.h?

[JF, 2014-10-08 18:56:11]

On 2014/10/08 17:46:04, Alexander Potapenko wrote:
> > The bots tell me I'll either have to change inclusion order for
> > Android and Mac, or use __atomic_* and feature detection. If I do
> > the former then I'll need to restore tsan header, but if I do the
> > latter then I can leave tsan out (since it'll understand
> > __atomic_*).
> Can't you just explicitly list the platforms (or the toolchains, because for
> Linux GCC and Clang may behave differently) that support <atomic> in the
#ifdef
> branch corresponding to atomicops_internals_portable.h?

I could but I'd rather not: feature detection is less brittle and requires
updates less often (future toolchain updaters may not realize that they now
enable atomics, with feature detection it'll start "just working"). I may
end up having to do this, but I'll try feature detection first.

[JF, 2014-10-09 00:24:33]

I fixed the linker's undefined reference complaint, it was
caused by declaring std::atomic_thread_fence without
defining it:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51038

This was fixed upstream on 2011-11-10, but:
 * The NaCl version of libstdc++ predates it.
 * The libstdc++ version used to build Chrome is the locally
   installed one, and some bots don't have the fixed version.


I also added feature detection for <atomic>, and whitelisted
libc++ and libstdc++. I'll monitor the bots to see if all
tested toolchains are happy with this.

[JF, 2014-10-09 04:20:18]

I did some homework to try to figure out how many people would be impacted by
ignoring the AMD memory barrier bug (note that the CPU has to be buggy, and the
race has to actually occur).

TL;DR: this would affect 0.02% of users. I think it's safe to ignore, and let
the compilers that handle the bug do so.


Details:

The affected processors are Opteron rev E, which map to 1xx/2xx/8xx:
 
http://sites.utexas.edu/jdm4372/2011/04/02/amd-opteron-processor-models-famil...

Those are the first generation Opterons, which shipped 2003-2004:
 
http://en.wikipedia.org/wiki/List_of_AMD_Opteron_microprocessors#First_Genera...

As far as I can tell we don't have good data on which CPUs our users have
(obvious privacy reasons), but I was able to gleam some information from
anonymous crash reports that we receive (the number of crashes is statistically
significant). This obviously skews my sample toward buggier CPUs, but let's
ignore the bias by assuming that 10 year old CPUs are buggier. I looked at data
from Chrome 27 and Chrome 28 separately to cross-validate the results, and
extracted CPU info (gives the vendor, family, model, stepping information from
CPUID).

I translated vendor/family/model/stepping to Opteron using this data:
 
http://www.cpu-world.com/cgi-bin/CPUID.pl?MANUF=AMD&FAMILY=Opteron&MODEL=&SIG...

Specifically:
 
http://www.cpu-world.com/cgi-bin/CompareCPUID.pl?CPUID=37157&CPUID=37156&CPUI...

Which leads me to believe that AuthenticAMD family 15 with the following model
and stepping are affected:
  5 0
  5 1
  5 8
  5 10
  37 0
  37 1
  39 1

[JF, 2014-10-09 04:46:40]

I'm dropping __has_include, and only whitelisting libc++ and libstdc++: the
preprocessor is sad when __has_include isn't supported and it contains angled
braces, as in __has_include(<atomic>), even if a previous condition
short-circuits this one.

There are still issues on the Mac bots: even in these circumstances they can't
find <atomic>.

There are still issues on other bots because of undefined references at link
time for std::atomic_thread_fence. My fix made the NaCl build pass, but there's
something else.

I'll have to investigate the two above issues.

[JF, 2014-10-09 21:12:25]

I have all the tests passing (ignoring what seems to be a flake in
WidgetTest.GetRestoredBounds).

Any opinions on the AMD memory barrier bug? I checked my methodology with cpu@
who said "seems solid. I am guessing that solar flares cause more crashes."

Any other comments, or should I just go for base/OWNERS approval?

[chromium-reviews, 2014-10-09 22:34:09]

I don't think I get a vote, but I would also ignore the AMD bug.  From your
description, it sounds like this is limited to processors that use the
original DDR spec for memory, as opposed to DDR2 or DDR3?  In my mind, that
qualifies as too ancient to care about.

(Fortunately, for my puposes, I know exactly how many Android devices are
using those :-) )

Hans

On Thu, Oct 9, 2014 at 2:12 PM, <jfb@chromium.org> wrote:

> I have all the tests passing (ignoring what seems to be a flake in
> WidgetTest.GetRestoredBounds).
>
> Any opinions on the AMD memory barrier bug? I checked my methodology with
> cpu@
> who said "seems solid. I am guessing that solar flares cause more crashes."
>
> Any other comments, or should I just go for base/OWNERS approval?
>
> https://codereview.chromium.org/636783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[JF, 2014-10-10 04:21:34]

jfb@chromium.org changed reviewers:
+ willchan@chromium.org

[JF, 2014-10-10 04:22:55]

Adding willchan@ for base/OWNERS review. He's on vacation tomorrow, will take a
look Monday.

[Alexander Potapenko, 2014-10-10 08:35:00]

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h
File base/atomicops.h (right):

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h#newcode31
base/atomicops.h:31: #include <cassert> // Small C++ header which defines
implementation specific
Two spaces before the comment, please.

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h#newcod...
base/atomicops.h:201: #endif // Portable / non-portable includes.
two spaces before the comment.

[JF, 2014-10-10 16:06:06]

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h
File base/atomicops.h (right):

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h#newcode31
base/atomicops.h:31: #include <cassert> // Small C++ header which defines
implementation specific
On 2014/10/10 08:35:00, Alexander Potapenko wrote:
> Two spaces before the comment, please.

Done.

https://codereview.chromium.org/636783002/diff/370001/base/atomicops.h#newcod...
base/atomicops.h:201: #endif // Portable / non-portable includes.
On 2014/10/10 08:35:00, Alexander Potapenko wrote:
> two spaces before the comment.

Done.

[JF, 2014-10-10 16:11:21]

jfb@chromium.org changed reviewers:
- jyasskin@chromium.org, rnk@chromium.org

[willchan no longer on Chromium, 2014-10-13 17:42:23]

Am I misunderstanding, or did Nico's point out STLPort on Android never get
addressed? Boehm seems to indicate there's a plan forward, but since it just
sounds like a plan, and hasn't been implemented yet, I'm unclear on the status
of this CL with regards to Android. Can someone clarify?

https://codereview.chromium.org/636783002/diff/450001/base/atomicops_internal...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/450001/base/atomicops_internal...
base/atomicops_internals_portable.h:140: #ifdef ARCH_CPU_64_BITS
Any reason not to use defined() instead?

[JF, 2014-10-13 17:51:59]

On 2014/10/13 17:42:23, willchan OOO until 03-22-15 wrote:
> Am I misunderstanding, or did Nico's point out STLPort on Android never get
> addressed? Boehm seems to indicate there's a plan forward, but since it just
> sounds like a plan, and hasn't been implemented yet, I'm unclear on the status
> of this CL with regards to Android. Can someone clarify?

STLPort won't define __GLIBCXX__ or _LIBCPP_VERSION, so it'll fall back to the
non-portable assembly version.

I can write a version of the portable header that doesn't rely on <atomic>, but
I'd do this as a separate patch. That header would use the language extensions
required to use <atomic>, and would function as expected with STLPort. I would
still keep the currently portable header since it uses the API C++ wants to
expose and is truly portable.

boehm's patch on Android is for platform/system/core/include/cutils/atomic.h,
which has similar problems as Chrome's atomics.

https://codereview.chromium.org/636783002/diff/450001/base/atomicops_internal...
File base/atomicops_internals_portable.h (right):

https://codereview.chromium.org/636783002/diff/450001/base/atomicops_internal...
base/atomicops_internals_portable.h:140: #ifdef ARCH_CPU_64_BITS
On 2014/10/13 17:42:22, willchan OOO until 03-22-15 wrote:
> Any reason not to use defined() instead?

Done, I was following what the other files did.

[willchan no longer on Chromium, 2014-10-13 17:53:39]

Oh, I think I see. Since you've implemented as a whitelist, then if the
necessary library support isn't there, it'll fallback to the old implementation.

[willchan no longer on Chromium, 2014-10-13 17:57:57]

LGTM

Please update the changelist description to indicate early on that it's not
switching to C++11 atomics for all platforms. Like, saying "Use C++11 atomics
when available".

And yeah, linking to a bug with the transition plan would be great. Thanks.

[willchan no longer on Chromium, 2014-10-13 17:58:56]

For the record, I didn't review the C++11 atomics implementation in detail. Just
did a base/OWNERS review. I'm trusting Boehm to have done a proper
implementation review there.

[JF, 2014-10-13 20:38:00]

I filed a bug that I'll use for this and subsequent patches:
  https://code.google.com/p/chromium/issues/detail?id=423074

It contains the assembly generated by GCC 4.8 and LLVM 3.6 for x86-32/x86-64/ARM
for the portable version of atomics that I implemented.

[chromium-reviews, 2014-10-13 22:37:57]

My understanding is that the longer term plan is to move away from
STLPort.  In the meantime, we're relying more on C's <stdatomic.h> and less
on <atomic>, which also has its issues ...

On Mon, Oct 13, 2014 at 1:38 PM, <jfb@chromium.org> wrote:

> I filed a bug that I'll use for this and subsequent patches:
>   https://code.google.com/p/chromium/issues/detail?id=423074
>
> It contains the assembly generated by GCC 4.8 and LLVM 3.6 for
> x86-32/x86-64/ARM
> for the portable version of atomics that I implemented.
>
> https://codereview.chromium.org/636783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[JF, 2014-10-14 00:22:05]

The CQ bit was checked by jfb@chromium.org

[commit-bot: I haz the power, 2014-10-14 00:23:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636783002/510001

[commit-bot: I haz the power, 2014-10-14 01:48:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-14 01:48:34]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[JF, 2014-10-14 03:45:31]

I re-ran the trybot and it passed. I couldn't find any evidence that this
is a known flake, I'll investigate a bit tomorrow.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Potapenko, 2014-10-14 14:16:35]

Just double-checking: you're not deleting
base/atomicops_internals_x86_gcc.{h,cc}, it's just Rietveld showing them as
deleted?

[JF, 2014-10-14 15:01:29]

On 2014/10/14 14:16:35, Alexander Potapenko wrote:
> Just double-checking: you're not deleting
> base/atomicops_internals_x86_gcc.{h,cc}, it's just Rietveld showing them as
> deleted?

Correct.

[JF, 2014-10-14 15:03:14]

On 2014/10/14 03:45:31, JF wrote:
> I re-ran the trybot and it passed. I couldn't find any evidence that this
> is a known flake, I'll investigate a bit tomorrow.

I can't repro this locally, it seems to consistently pass, so I'll assume the
bot failure was a flake.

[JF, 2014-10-14 15:03:17]

On 2014/10/14 03:45:31, JF wrote:
> I re-ran the trybot and it passed. I couldn't find any evidence that this
> is a known flake, I'll investigate a bit tomorrow.

I can't repro this locally, it seems to consistently pass, so I'll assume the
bot failure was a flake.

[JF, 2014-10-14 15:03:59]

The CQ bit was checked by jfb@chromium.org

[commit-bot: I haz the power, 2014-10-14 15:05:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/636783002/510001

[commit-bot: I haz the power, 2014-10-14 15:06:15]

Committed patchset #14 (id:510001)

[commit-bot: I haz the power, 2014-10-14 15:07:07]

Patchset 14 (id:??) landed as
https://crrev.com/57a4e4a50c673c25e9cdaab53e32f6e53aa0b574
Cr-Commit-Position: refs/heads/master@{#299485}