DescriptionFix atomic ops on ARM to compile in NaCl untrusted targets.This makes the arm version of atomicops_internals use gcc intrinsics for the NaClbuild. Other linux targets won't change.BUG=116317TEST=compiles on arm botsCommitted: https://src.chromium.org/viewvc/chrome?view=rev&revision=152791 Patch Set 1 # Patch Set 2 : # Patch Set 3 : # Patch Set 4 : #Total comments: 10 Patch Set 5 : # Patch Set 6 : # Patch Set 7 : #Total comments: 4 Patch Set 8 : # Patch Set 9 : # Download [raw][tar.bz2] Unified diffs | Side-by-side diffs | Delta from patch set | Stats (+22 lines, -164 lines) | Patch |
---|
M | base/atomicops.h | View | 1 chunk | +3 lines, -2 lines | 0 comments | Download | D | base/atomicops_internals_arm_gcc.h | View | 1 chunk | +0 lines, -124 lines | 0 comments | Download | A + | base/atomicops_internals_gcc.h | View | 5 chunks | +18 lines, -38 lines | 0 comments | Download | M | base/base.gypi | View | 1 chunk | +1 line, -0 lines | 0 comments | Download |
Messages Total messages: 29 (0 generated) bbudge | mseaborn for NaCl / atomic ops correctness jar OR willchan for OWNERS in base dmichael . |
| 8 years, 4 months ago (2012-08-16 20:44:17 UTC) #1 |
dmichael (off chromium) | chrome/nacl.gypi and ppapi/native_client/native_client.gypi lgtm |
| 8 years, 4 months ago (2012-08-16 21:04:15 UTC) #2 |
willchan no longer on Chromium | I'm happy to rubberstamp this once mseaborn, or another qualified revier, has reviewed this. |
| 8 years, 4 months ago (2012-08-16 22:11:47 UTC) #3 |
bbudge | Thanks Will. I think I'll add brettw and remove you though as this CL is . |
| 8 years, 4 months ago (2012-08-16 22:15:32 UTC) #4 |
bbudge | Adding willchan back. There may be some questions about merging the NaCl and ARM internal . |
| 8 years, 4 months ago (2012-08-16 22:25:03 UTC) #5 |
willchan no longer on Chromium | In case it wasn't clear, Brett's a base/ OWNER too, so if he has more . |
| 8 years, 4 months ago (2012-08-16 22:26:52 UTC) #6 |
bbudge | 8 years, 4 months ago (2012-08-16 22:28:25 UTC) #7 |
jar (doing other things) | 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.h#newcode10 base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. . |
| 8 years, 4 months ago (2012-08-16 22:47:20 UTC) #8 |
Mark Seaborn | 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.h#newcode10 base/atomicops_internals_nacl.h:10: // For Native Client, use GCC atomic op intrinsics. . |
| 8 years, 4 months ago (2012-08-17 00:08:24 UTC) #9 |
DaleCurtis | 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.h#newcode10 . |
| 8 years, 3 months ago (2012-08-21 00:03:18 UTC) #10 |
bbudge | Since the comments address the atomicops changes, I'm going to split the NaCl build changes . |
| 8 years, 3 months ago (2012-08-21 02:02:07 UTC) #11 |
DaleCurtis | cc: thestig who wrote the original atomicops_internal for ARM and might be able to better . |
| 8 years, 3 months ago (2012-08-21 02:19:24 UTC) #12 |
bbudge | Compile failed because I 'optimized' out the public MemoryBarrier() function. |
| 8 years, 3 months ago (2012-08-21 02:27:04 UTC) #13 |
DaleCurtis | http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h#newcode21 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 . |
| 8 years, 3 months ago (2012-08-21 17:52:01 UTC) #14 |
nfullagar | http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h#newcode18 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 . |
| 8 years, 3 months ago (2012-08-21 18:23:20 UTC) #15 |
Lei Zhang | http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (left): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h#oldcode73 base/atomicops_internals_gcc.h:73: nit: extra new line? http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h#newcode1 . |
| 8 years, 3 months ago (2012-08-21 18:44:46 UTC) #16 |
bbudge | http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/13005/base/atomicops_internals_gcc.h#newcode18 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 . |
| 8 years, 3 months ago (2012-08-21 19:33:57 UTC) #17 |
bbudge | This latest version compiles and runs correctly in the NaCl IPC-based PPAPI proxy. It also . |
| 8 years, 3 months ago (2012-08-21 21:49:02 UTC) #18 |
Roland McGrath | 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_gcc.h#newcode21 base/atomicops_internals_gcc.h:21: return __sync_bool_compare_and_swap(const_cast<Atomic32*>(ptr), What's this cast for? |
| 8 years, 3 months ago (2012-08-21 21:52:59 UTC) #19 |
bbudge | 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_gcc.h#newcode21 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: > . |
| 8 years, 3 months ago (2012-08-21 22:06:12 UTC) #20 |
Roland McGrath | http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_gcc.h#newcode20 base/atomicops_internals_gcc.h:20: if (__sync_bool_compare_and_swap(ptr, old_value, new_value)) { Why not just use . |
| 8 years, 3 months ago (2012-08-21 22:14:14 UTC) #21 |
bbudge | http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_gcc.h File base/atomicops_internals_gcc.h (right): http://codereview.chromium.org/10831358/diff/15008/base/atomicops_internals_gcc.h#newcode20 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 . |
| 8 years, 3 months ago (2012-08-21 23:23:39 UTC) #22 |
nfullagar | atomicops_internals_gcc.h --> base/base.gypi |
| 8 years, 3 months ago (2012-08-21 23:26:51 UTC) #23 |
bbudge | On 2012/08/21 23:26:51, nfullagar wrote: > atomicops_internals_gcc.h --> base/base.gypi Done. |
| 8 years, 3 months ago (2012-08-21 23:33:51 UTC) #24 |
DaleCurtis | | 8 years, 3 months ago (2012-08-22 17:43:22 UTC) #25 |
brettw | | 8 years, 3 months ago (2012-08-22 18:13:40 UTC) #26 |
Philippe | On 2012/08/22 18:13:40, brettw wrote: > lgtm Hi guys, I'm maintaining the atomicops implementation in . |
| 7 years, 11 months ago (2013-01-03 13:56:52 UTC) #27 |
Use chromium.org instead | Those GCC intrinsics are certainly fine across all machines when compiled by a GCC version . |
| 7 years, 11 months ago (2013-01-03 17:36:23 UTC) #28 |
jar (doing other things) | +cc isherman On Thu, Jan 3, 2013 at 9:35 AM, Roland McGrath <mcgrathr@google.com> wrote: > . |
| 7 years, 11 months ago (2013-01-04 20:49:57 UTC) #29 |
|