Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831358:
Fix atomic ops on ARM to compile in NaCl untrusted targets. (Closed)
|
Created:
8 years, 4 months ago by bbudge Modified:
7 years, 11 months ago Reviewers:
nfullagar, jar (doing other things), Lei Zhang, Use chromium.org instead, DaleCurtis, Roland McGrath, Mark Seaborn, brettw CC:
chromium-reviews, erikwright+watch_chromium.org, brettw-cc_chromium.org, Roland McGrath, Lei Zhang Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix atomic ops on ARM to compile in NaCl untrusted targets.
This makes the arm version of atomicops_internals use gcc intrinsics for the NaCl
build. Other linux targets won't change.
BUG=116317
TEST=compiles on arm bots
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152791
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : #
Total comments: 3
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 29 (0 generated)
mseaborn for NaCl / atomic ops correctness jar OR willchan for OWNERS in base dmichael for PPAPI build files http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h File base/atomicops_internals_nacl.h (right): http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. 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.
chrome/nacl.gypi and ppapi/native_client/native_client.gypi lgtm
I'm happy to rubberstamp this once mseaborn, or another qualified revier, has reviewed this.
Thanks Will. I think I'll add brettw and remove you though as this CL is going to grow a little and most stuff is related to our untrusted NaCl build of the PPAPI proxy.
Adding willchan back. There may be some questions about merging the NaCl and ARM internal defs.
In case it wasn't clear, Brett's a base/ OWNER too, so if he has more context here, it's fine to have him review this instead of me. On Thu, Aug 16, 2012 at 3:25 PM, <bbudge@chromium.org> wrote: > Adding willchan back. There may be some questions about merging the NaCl > and ARM > internal defs. > > http://codereview.chromium.**org/10831358/<http://codereview.chromium.org/108... >
http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h File base/atomicops_internals_nacl.h (right): http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. 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: > 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. http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:63: return NoBarrier_CompareAndSwap(ptr, old_value, new_value); 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.
http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h File base/atomicops_internals_nacl.h (right): http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. On 2012/08/16 20:44:17, bbudge1 wrote: > 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. 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.]
On 2012/08/17 00:08:24, Mark Seaborn wrote: > http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h > File base/atomicops_internals_nacl.h (right): > > http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... > base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op > intrinsics. > On 2012/08/16 20:44:17, bbudge1 wrote: > > 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. > > 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.] This same issue is now blocking my CL too, http://codereview.chromium.org/10826296/ Let me know if any help is needed.
Since the comments address the atomicops changes, I'm going to split the NaCl build changes off into another issue so this will be easier to review. This CL now only changes base/atomicops*. http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h File base/atomicops_internals_nacl.h (right): http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. On 2012/08/16 22:47:21, jar wrote: > 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: > > 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. > 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. http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. On 2012/08/17 00:08:24, Mark Seaborn wrote: > On 2012/08/16 20:44:17, bbudge1 wrote: > > 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. > > 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.] Since I have no idea what the strange Linux code that is used for ChromeOS on ARM, I am reluctant to change it.
cc: thestig who wrote the original atomicops_internal for ARM and might be able to better elaborate on the "strange Linux code that is used for ChromeOS" :)
Compile failed because I "optimized" out the public MemoryBarrier() function.
http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:21: #else // !defined(OS_NACL) Per http://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libgcc/config/arm/linux-atomic.c the GCC intrinsics are implemented using the same linux primitives, so I think you can safely have everyone use the gcc intrinsics. Looks like it's always been implemented that way, so whatever version we're using should be fine too: http://gcc.gnu.org/ml/gcc-patches/2008-07/msg00025.html
http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:18: __sync_val_compare_and_swap(const_cast<Atomic32*>(ptr), \ The “val” version of __sync_val_compare_and_swap returns the contents of *ptr before the operation. But code below looks like it is expecting a bool result?
http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (left): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:73: nit: extra new line? http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Apparently we don't do this anymore. See relevant chromium-dev thread. http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:5: // This file is an internal atomic implementation, use base/atomicops.h instead. Can you add a comment here to say what platforms this is for? Previously, it was obvious it was for ARM from the file name.
http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:18: __sync_val_compare_and_swap(const_cast<Atomic32*>(ptr), \ On 2012/08/21 18:23:20, nfullagar wrote: > The “val” version of __sync_val_compare_and_swap returns the contents of *ptr > before the operation. But code below looks like it is expecting a bool result? It does appear that way, but the old function signature returns Atomic32, which led me to the 'val' version. Neither one works in the NaCl IPC proxy though. http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:21: #else // !defined(OS_NACL) On 2012/08/21 17:52:01, DaleCurtis wrote: > Per http://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libgcc/config/arm/linux-atomic.c > the GCC intrinsics are implemented using the same linux primitives, so I think > you can safely have everyone use the gcc intrinsics. > > Looks like it's always been implemented that way, so whatever version we're > using should be fine too: > > http://gcc.gnu.org/ml/gcc-patches/2008-07/msg00025.html I'm going to run a trial on the normal Chrome ARM build to see if this is true, and that all tests pass.
This latest version compiles and runs correctly in the NaCl IPC-based PPAPI proxy. It also switches the Linux ARM build to use GCC intrinsics rather than Linux kernel-provided functions. http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.h File base/atomicops_internals_nacl.h (right): http://codereview.chromium.org/10831358/diff/1/base/atomicops_internals_nacl.... base/atomicops_internals_nacl.h:63: return NoBarrier_CompareAndSwap(ptr, old_value, new_value); On 2012/08/16 22:47:21, jar wrote: > 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. 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. http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (left): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:73: On 2012/08/21 18:44:47, Lei Zhang wrote: > nit: extra new line? Done. http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/21 18:44:47, Lei Zhang wrote: > Apparently we don't do this anymore. See relevant chromium-dev thread. Done. http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_g... base/atomicops_internals_gcc.h:5: // This file is an internal atomic implementation, use base/atomicops.h instead. On 2012/08/21 18:44:47, Lei Zhang wrote: > Can you add a comment here to say what platforms this is for? Previously, it was > obvious it was for ARM from the file name. Done. http://codereview.chromium.org/10831358/diff/2014/base/atomicops.h File base/atomicops.h (right): http://codereview.chromium.org/10831358/diff/2014/base/atomicops.h#newcode137 base/atomicops.h:137: #include "base/atomicops_internals_gcc.h" Moving this ahead of GCC x86 and adding OS_NACL so NaCl will use intrinsics by default. This is more robust than using platform specific internals since we would expect it to always produce valid NaCl code.
http://codereview.chromium.org/10831358/diff/2014/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/2014/base/atomicops_internals_gc... base/atomicops_internals_gcc.h:21: return __sync_bool_compare_and_swap(const_cast<Atomic32*>(ptr), What's this cast for?
http://codereview.chromium.org/10831358/diff/2014/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/2014/base/atomicops_internals_gc... base/atomicops_internals_gcc.h:21: return __sync_bool_compare_and_swap(const_cast<Atomic32*>(ptr), On 2012/08/21 21:52:59, Roland McGrath wrote: > What's this cast for? It's a vestige of the old file. As you point out, it's not needed. Since that was the only reason to wrap this up in its own function, I removed it just call the intrinsic directly at the 3 call sites. I did add a comment in the Acquire_ function explaining what's going on.
http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... base/atomicops_internals_gcc.h:20: if (__sync_bool_compare_and_swap(ptr, old_value, new_value)) { Why not just use __sync_val_compare_and_swap here? Then you don't need any separate fetch through PTR. http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... base/atomicops_internals_gcc.h:48: if (!__sync_bool_compare_and_swap(ptr, old_value, new_value) == 0) { (!... == 0) should be just (...).
http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... base/atomicops_internals_gcc.h:20: if (__sync_bool_compare_and_swap(ptr, old_value, new_value)) { On 2012/08/21 22:14:15, Roland McGrath wrote: > Why not just use __sync_val_compare_and_swap here? > Then you don't need any separate fetch through PTR. Done. http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_g... base/atomicops_internals_gcc.h:48: if (!__sync_bool_compare_and_swap(ptr, old_value, new_value) == 0) { On 2012/08/21 22:14:15, Roland McGrath wrote: > (!... == 0) should be just (...). Done.
atomicops_internals_gcc.h --> base/base.gypi
On 2012/08/21 23:26:51, nfullagar wrote: > atomicops_internals_gcc.h --> base/base.gypi Done.
lgtm
lgtm
Message was sent while issue was closed.
On 2012/08/22 18:13:40, brettw wrote: > lgtm Hi guys, I'm maintaining the atomicops implementation in Protobuf which essentially means keeping it in sync with the Chromium's one. I'm wondering if I could replace in Protobuf the subtle GCC platform-specific files (for ARM, MIPS, x86) with a single one relying on GCC intrinsics (like this CL does for ARM). This would decrease the maintenance cost in Protobuf and would let the toolchain deal with these subtleties. Do you guys (e.g. Mark Seaborn) see anything (e.g. incorrectness/performance impact) that would prevent me from doing this for platforms other than ARM? Thanks, Philippe.
Those GCC intrinsics are certainly fine across all machines when compiled by a GCC version that supports them (recent versions do for all machines I'm fairly sure). But if you are concerned about wide portability, then it may be necessary to do some research to figure out what is the earliest GCC version that supported the necessary intrinsics on every machine you care about and decide whether you care about being able to compile your code with a compiler older than that.
+cc isherman On Thu, Jan 3, 2013 at 9:35 AM, Roland McGrath <mcgrathr@google.com> wrote: > Those GCC intrinsics are certainly fine across all machines when compiled > by a GCC version that supports them (recent versions do for all machines > I'm fairly sure). But if you are concerned about wide portability, then it > may be necessary to do some research to figure out what is the earliest GCC > version that supported the necessary intrinsics on every machine you care > about and decide whether you care about being able to compile your code > with a compiler older than that. > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
