Fix atomic ops on ARM to compile in NaCl untrusted targets.

base/atomicops_internals_nacl.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This file is an internal atomic implementation, use base/atomicops.h instead.
+
+#ifndef BASE_ATOMICOPS_INTERNALS_NACL_H_
+#define BASE_ATOMICOPS_INTERNALS_NACL_H_
+
+// For Native Client, use GCC atomic op intrinsics.

[bbudge, 2012/08/16 20:44:17]

This file is a copy of base/atomicops_internals_arm_gcc.h, with the weird Linux
compare-and-exchange and memory-barrier primitives replaced by GCC intrinsics
which should be equivalent.

[jar (doing other things), 2012/08/16 22:47:21]

You should at a minimum put this comment in the code.

IMO, it would be better to re-use the existing header, with only an ifdef based
change in the pair of functions you really changed.

Have you considered that?

It would vastly reduce the amount of replicated code... and this is delicate
code, that IMO should not be replicated.

On 2012/08/16 20:44:17, bbudge1 wrote:

[Mark Seaborn, 2012/08/17 00:08:24]

Is there any reason you can't modify the existing
base/atomicops_internals_arm_gcc.h to use portable GCC intrinsics instead of ARM
Linux's special stuff, and then use the same file for NaCl too?

GCC's intrinsics should know how to use ARM Linux specific stuff on older CPUs
(via libgcc) or use ARM instructions directly on newer CPUs.  (And Chrome OS
probably only targets the newer ARM CPUs anyway.)  Chromium code shouldn't need
to know about these details, as far as I can tell -- certainly there's no reason
explained in the code.

[CC'ing Roland, who I asked about this earlier.]

[bbudge, 2012/08/21 02:02:07]

Done. I added conditional expressions to switch between GCC intrinsics for NaCl
and the existing code for others. I renamed the file
'base/atomicops_internals_gcc.h' since it really isn't overtly ARM specific. I
am switching the NaCl untrusted build to always use GCC intrinsics.

[bbudge, 2012/08/21 02:02:07]

Since I have no idea what the strange Linux code that is used for ChromeOS on
ARM, I am reluctant to change it.

+
+namespace base {
+namespace subtle {
+
+inline Atomic32 NoBarrier_CompareAndSwap(volatile Atomic32* ptr,
+                                         Atomic32 old_value,
+                                         Atomic32 new_value) {
+  Atomic32 prev_value = *ptr;
+  do {
+    if (!__sync_val_compare_and_swap(const_cast<Atomic32*>(ptr),
+                                     old_value, new_value)) {
+      return old_value;
+    }
+    prev_value = *ptr;
+  } while (prev_value == old_value);
+  return prev_value;
+}
+
+inline Atomic32 NoBarrier_AtomicExchange(volatile Atomic32* ptr,
+                                         Atomic32 new_value) {
+  Atomic32 old_value;
+  do {
+    old_value = *ptr;
+  } while (__sync_val_compare_and_swap(const_cast<Atomic32*>(ptr),
+                                       old_value, new_value));
+  return old_value;
+}
+
+inline Atomic32 NoBarrier_AtomicIncrement(volatile Atomic32* ptr,
+                                          Atomic32 increment) {
+  return Barrier_AtomicIncrement(ptr, increment);
+}
+
+inline Atomic32 Barrier_AtomicIncrement(volatile Atomic32* ptr,
+                                        Atomic32 increment) {
+  for (;;) {
+    // Atomic exchange the old value with an incremented one.
+    Atomic32 old_value = *ptr;
+    Atomic32 new_value = old_value + increment;
+    if (__sync_val_compare_and_swap(const_cast<Atomic32*>(ptr),
+                                    old_value, new_value) == 0) {
+      // The exchange took place as expected.
+      return new_value;
+    }
+    // Otherwise, *ptr changed mid-loop and we need to retry.
+  }
+
+}
+
+inline Atomic32 Acquire_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  return NoBarrier_CompareAndSwap(ptr, old_value, new_value);

[jar (doing other things), 2012/08/16 22:47:21]

This was confusing, since Acquire semantics require a barrier.  It turns out
that your NoBarrier* function here calls __sync_*, which I'm guessing provides a
Barrier.

It would be much clearer to call:
Barrier_CompareAndSwap(...) here, and then
NoBarrier_CompareAndSwap() call Barrier_CompareAndSwap()


If you can't see a nice way of merging this file with the one you copied, you
should then make a corresponding readability change in that file as well.

[bbudge, 2012/08/21 21:49:02]

This code is indeed confusing. I succeeded in merging the two files into a
single one which uses GCC intrinsics. The function signature for
compare-and-swap in the old code is misleading, as the return type should be
bool. I had to use __sync_bool_compare_and_swap to make things work. I tried
calling my new Barrier_CompareAndSwap function directly from the Acquire and
Release_CompareAndSwap functions but that doesn't work correctly.

+}
+
+inline Atomic32 Release_CompareAndSwap(volatile Atomic32* ptr,
+                                       Atomic32 old_value,
+                                       Atomic32 new_value) {
+  return NoBarrier_CompareAndSwap(ptr, old_value, new_value);
+}
+
+inline void NoBarrier_Store(volatile Atomic32* ptr, Atomic32 value) {
+  *ptr = value;
+}
+
+inline void MemoryBarrier() {
+  __sync_synchronize();
+}
+
+inline void Acquire_Store(volatile Atomic32* ptr, Atomic32 value) {
+  *ptr = value;
+  MemoryBarrier();
+}
+
+inline void Release_Store(volatile Atomic32* ptr, Atomic32 value) {
+  MemoryBarrier();
+  *ptr = value;
+}
+
+inline Atomic32 NoBarrier_Load(volatile const Atomic32* ptr) {
+  return *ptr;
+}
+
+inline Atomic32 Acquire_Load(volatile const Atomic32* ptr) {
+  Atomic32 value = *ptr;
+  MemoryBarrier();
+  return value;
+}
+
+inline Atomic32 Release_Load(volatile const Atomic32* ptr) {
+  MemoryBarrier();
+  return *ptr;
+}
+
+} // namespace base::subtle
+} // namespace base
+
+#endif  // BASE_ATOMICOPS_INTERNALS_NACL_H_