Add atomic operations for ARM.

Add atomic operations for ARM.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=15070

[Lei Zhang, 2009-04-03 19:10:09]

Dean, want to take a look at this and/or recommend some other reviewers?

[Dean McNamee, 2009-04-14 15:42:35]

Sorry, I miss this review...

- I had talked (and started) some tests, but never really got anything great up.
 Seems like we should really have some good tests to hammer this and make sure
it's right.

- I didn't review the subtleties, I am hoping this code was derived from
someplace that figured that all out?

http://codereview.chromium.org/57031/diff/1/3
File base/atomicops_internals_arm_gcc.h (right):

http://codereview.chromium.org/57031/diff/1/3#newcode14
Line 14: Atomic32 new_value,
indention is off

http://codereview.chromium.org/57031/diff/1/3#newcode19
Line 19: Atomic32 new_value,
indention is off

http://codereview.chromium.org/57031/diff/1/3#newcode27
Line 27: return reinterpret_cast<LinuxKernelCmpxchgFunc>(0xffff0fc0)(old_value,
Why do you need this wrapper, and instead can't just declare a function pointer
with the value?

http://codereview.chromium.org/57031/diff/1/3#newcode51
Line 51: const_cast<Atomic32*>(ptr)));
indention is off

http://codereview.chromium.org/57031/diff/1/3#newcode62
Line 62: // From Google Gears
If a bunch of this code is from Gears (which it sounds like it is), I would just
put that in the top comment, instead of announcing it in random places.

http://codereview.chromium.org/57031/diff/1/3#newcode93
Line 93: reinterpret_cast<LinuxKernelMemoryBarrierFunc>(0xffff0fa0)();
This again should just be some function pointer.

http://codereview.chromium.org/57031/diff/1/4
File build/build_config.h (right):

http://codereview.chromium.org/57031/diff/1/4#newcode57
Line 57: #define ARCH_CPU_ARMEL 1
Is there 64-bit ARM, should we worry about it?

http://codereview.chromium.org/57031/diff/1/4#newcode68
Line 68: (__WCHAR_MAX__ == 0x7fffffff || __WCHAR_MAX__ == 0xffffffff)
This doesn't seem right, aren't you saying UTF16 when wchar is 4-byte?  Or what
did I miss.

I also think this is sufficiently complicated to need some comments now.

[Dean McNamee, 2009-04-24 15:54:29]

Ping?

On 2009/04/14 15:42:35, Dean McNamee wrote:
> Sorry, I miss this review...
> 
> - I had talked (and started) some tests, but never really got anything great
up.
>  Seems like we should really have some good tests to hammer this and make sure
> it's right.
> 
> - I didn't review the subtleties, I am hoping this code was derived from
> someplace that figured that all out?
> 
> http://codereview.chromium.org/57031/diff/1/3
> File base/atomicops_internals_arm_gcc.h (right):
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode14
> Line 14: Atomic32 new_value,
> indention is off
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode19
> Line 19: Atomic32 new_value,
> indention is off
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode27
> Line 27: return
reinterpret_cast<LinuxKernelCmpxchgFunc>(0xffff0fc0)(old_value,
> Why do you need this wrapper, and instead can't just declare a function
pointer
> with the value?
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode51
> Line 51: const_cast<Atomic32*>(ptr)));
> indention is off
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode62
> Line 62: // From Google Gears
> If a bunch of this code is from Gears (which it sounds like it is), I would
just
> put that in the top comment, instead of announcing it in random places.
> 
> http://codereview.chromium.org/57031/diff/1/3#newcode93
> Line 93: reinterpret_cast<LinuxKernelMemoryBarrierFunc>(0xffff0fa0)();
> This again should just be some function pointer.
> 
> http://codereview.chromium.org/57031/diff/1/4
> File build/build_config.h (right):
> 
> http://codereview.chromium.org/57031/diff/1/4#newcode57
> Line 57: #define ARCH_CPU_ARMEL 1
> Is there 64-bit ARM, should we worry about it?
> 
> http://codereview.chromium.org/57031/diff/1/4#newcode68
> Line 68: (__WCHAR_MAX__ == 0x7fffffff || __WCHAR_MAX__ == 0xffffffff)
> This doesn't seem right, aren't you saying UTF16 when wchar is 4-byte?  Or
what
> did I miss.
> 
> I also think this is sufficiently complicated to need some comments now.

[Lei Zhang, 2009-04-29 22:52:47]

On 2009/04/14 15:42:35, Dean McNamee wrote:
> - I had talked (and started) some tests, but never really got anything great
up.
>  Seems like we should really have some good tests to hammer this and make sure
> it's right.
> 
> - I didn't review the subtleties, I am hoping this code was derived from
> someplace that figured that all out?

I derived the code mostly based on the logic from the other atomicops
implementations. It passes the unit tests, but I guess that's not exactly the
strongest vote of confidence.

[Lei Zhang, 2009-04-29 22:52:57]

http://codereview.chromium.org/57031/diff/1/3
File base/atomicops_internals_arm_gcc.h (right):

http://codereview.chromium.org/57031/diff/1/3#newcode14
Line 14: Atomic32 new_value,
On 2009/04/14 15:42:35, Dean McNamee wrote:
> indention is off

I fixed all the indentations.

http://codereview.chromium.org/57031/diff/1/3#newcode27
Line 27: return reinterpret_cast<LinuxKernelCmpxchgFunc>(0xffff0fc0)(old_value,
On 2009/04/14 15:42:35, Dean McNamee wrote:
> Why do you need this wrapper, and instead can't just declare a function
pointer
> with the value?

That's what Gears had, and I can never get func pointer syntax right. Changed.

http://codereview.chromium.org/57031/diff/1/3#newcode62
Line 62: // From Google Gears
On 2009/04/14 15:42:35, Dean McNamee wrote:
> If a bunch of this code is from Gears (which it sounds like it is), I would
just
> put that in the top comment, instead of announcing it in random places.

Ok, I made a note at the top instead.

http://codereview.chromium.org/57031/diff/1/4
File build/build_config.h (right):

http://codereview.chromium.org/57031/diff/1/4#newcode57
Line 57: #define ARCH_CPU_ARMEL 1
On 2009/04/14 15:42:35, Dean McNamee wrote:
> Is there 64-bit ARM, should we worry about it?

Not that I know of.

http://codereview.chromium.org/57031/diff/1/4#newcode68
Line 68: (__WCHAR_MAX__ == 0x7fffffff || __WCHAR_MAX__ == 0xffffffff)
On 2009/04/14 15:42:35, Dean McNamee wrote:
> This doesn't seem right, aren't you saying UTF16 when wchar is 4-byte?  Or
what
> did I miss.
> 
> I also think this is sufficiently complicated to need some comments now.

To quote awalker: "'wchar_t == UTF16' is a Windowsism (gcc defaults to 4 bytes."

We make sure it's 4 bytes by checking _WCHAR_MAX_, which on ia32/amd64 is
0x7fffffff. However, on ARM it's 0xffffffff, so I changed the check to accept
that.

[stoyan, 2009-04-29 23:15:18]

http://codereview.chromium.org/57031/diff/5001/6002
File base/atomicops_internals_arm_gcc.h (right):

http://codereview.chromium.org/57031/diff/5001/6002#newcode28
Line 28: (LinuxKernelMemoryBarrierFunc) 0xffff0fa0;
perhaps __attribute__(weak)? 
Or "const" hoping gcc will be smart enough eliminate the variable storage
entirely.

[Dean McNamee, 2009-04-30 09:08:36]

LG

http://codereview.chromium.org/57031/diff/5001/6002
File base/atomicops_internals_arm_gcc.h (right):

http://codereview.chromium.org/57031/diff/5001/6002#newcode7
Line 7: // LinuxKernelCmpxchg and Barrier_AtomicIncrement are from Google Gears
period at the end :\

[Mark Mentovai, 2009-04-30 16:14:22]

http://codereview.chromium.org/57031/diff/5001/6003
File build/build_config.h (right):

http://codereview.chromium.org/57031/diff/5001/6003#newcode68
Line 68: (__WCHAR_MAX__ == 0x7fffffff || __WCHAR_MAX__ == 0xffffffff)
Weird that it's set to 0xffffffff on ARM.  Maybe this line should be:

(__WCHAR_MAX__ >= 0x7fffffff && __WCHAR_MAX__ <= 0xffffffff)

[Lei Zhang, 2009-04-30 18:20:02]

On 2009/04/30 16:14:22, Mark Mentovai wrote:
> http://codereview.chromium.org/57031/diff/5001/6003
> File build/build_config.h (right):
> 
> http://codereview.chromium.org/57031/diff/5001/6003#newcode68
> Line 68: (__WCHAR_MAX__ == 0x7fffffff || __WCHAR_MAX__ == 0xffffffff)
> Weird that it's set to 0xffffffff on ARM.  Maybe this line should be:
> 
> (__WCHAR_MAX__ >= 0x7fffffff && __WCHAR_MAX__ <= 0xffffffff)

To quote an old gcc manual: "the macro WCHAR_MAX evaluates to the maximum value
representable by an object of type wint_t." On x86 it's an signed int, and on
arm it's an unsigned int. I don't think __WCHAR_MAX__ will get a value in
between 0x7fffffff and 0xffffffff.

[Lei Zhang, 2009-04-30 19:37:17]

On 2009/04/29 23:15:18, stoyan wrote:
> http://codereview.chromium.org/57031/diff/5001/6002
> File base/atomicops_internals_arm_gcc.h (right):
> 
> http://codereview.chromium.org/57031/diff/5001/6002#newcode28
> Line 28: (LinuxKernelMemoryBarrierFunc) 0xffff0fa0;
> perhaps __attribute__(weak)? 
> Or "const" hoping gcc will be smart enough eliminate the variable storage
> entirely.

I added __attribute__((weak)), thanks.