Bug 106739 – Fix the atomicIncrement implementation for MIPS GCC

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Csaba Osztrogonác]

After https://trac.webkit.org/changeset/139514 we need 32 and 64 bit implementation 
for atomicIncrement(). I fixed it in http://trac.webkit.org/changeset/139553, but
it seems __sync_add_and_fetch isn't supported by the MIPS GCC version. As far as
I know it should be supported from GCC 4.1. Is it possible if the MIPS bot has
an older GCC or __sync_add_and_fetch isn't supported on MIPS platform?

[Gergely Kis]

I will take a look into this. The MIPS bot is using GCC 4.4 cross compiler from emdebian.

[Gergely Kis]

There are no atomic 64bit builtin implementations in MIPS GCC. We will provide a custom implementation for the correct variant of __sync_add_and_fetch.

[Balazs Kilvady]

Created attachment 183178 [details]
proposed patch.

[WebKit Review Bot]

Attachment 183178 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pr..." exit_code: 1
Source/WTF/wtf/Atomics.cpp:115:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:120:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 183766 [details]
proposed patch.

Rebased on r140325.

[WebKit Review Bot]

Attachment 183766 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pr..." exit_code: 1
Source/WTF/wtf/Atomics.cpp:116:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:121:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 184191 [details]
proposed patch with improved init.

Init method improved to use less allocation/new. Also rebased on r140530.

[WebKit Review Bot]

Attachment 184191 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:107:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:112:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 184191 [details]
proposed patch with improved init.

This looks amazingly inefficient for an atomic increment/decrement. Is there seriously no better solution for MIPS?

[Balazs Kilvady]

(In reply to comment #9)
> (From update of attachment 184191 [details])
> This looks amazingly inefficient for an atomic increment/decrement. Is there seriously no better solution for MIPS?
Thank you for reviewing.

32 bit MIPS cores have a ll/sc instruction pair to implement atomic read-modify-write. But only for 32 bit values in memory. Only on 64 bit cores there is a lld/scd pair for 64 memory-values. We haven't found a way to ensure atomicity on 32 bit cores without locking.

This patch is only for 64-bit atomics on 32 bit MIPS cores, and that 32-bit's atomics are inlined efficiently
by the toolchain. When webkit compiled for mips64, then we do have 64-bit atomic-ops as well by gcc, but those instructions are not available on the 32-bit cores.

[Balazs Kilvady]

Created attachment 184526 [details]
proposed WTFized patch.

The code of this patch is based on the Android atomicInc implementation for MIPS32 so you would get the same implementation if you compile webkit for android platform and run webkit on MIPS32.

[WebKit Review Bot]

Attachment 184526 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:90:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:95:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 184526 [details]
proposed WTFized patch.

Attachment 184526 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16110174

[Early Warning System Bot]

Comment on attachment 184526 [details]
proposed WTFized patch.

Attachment 184526 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16083771

[Balazs Kilvady]

Created attachment 184530 [details]
fixed WTFized patch.

[WebKit Review Bot]

Attachment 184530 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:90:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:95:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 184530 [details]
fixed WTFized patch.

Attachment 184530 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16097313

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Simon Hausmann]

Comment on attachment 184530 [details]
fixed WTFized patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=184530&action=review

>> Source/WTF/wtf/Atomics.cpp:90
>> +int64_t __sync_add_and_fetch_8(int64_t volatile* addend, int64_t value)
> 
> __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

So this is a workaround for a bug in the toolchain that is missing this symbol? Maybe that should be explained in a comment and the ChangeLog? What if the toolchain is newer and suddenly provides the symbol? Are version guards missing?

[Gergely Kis]

(In reply to comment #18)
> (From update of attachment 184530 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184530&action=review
> 
> >> Source/WTF/wtf/Atomics.cpp:90
> >> +int64_t __sync_add_and_fetch_8(int64_t volatile* addend, int64_t value)
> > 
> > __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> 
> So this is a workaround for a bug in the toolchain that is missing this symbol? Maybe that should be explained in a comment and the ChangeLog? What if the toolchain is newer and suddenly provides the symbol? Are version guards missing?


Dear Simon, 

Thank you for reviewing.

Currently none of the MIPS32 toolchains defines this symbol, that is why we did not provide version guards. Another possibility would be to add this MIPS implementation under a different name, that conforms to the webkit coding style, and select this new symbol using #if CPU(MIPS) in Atomics.h,

This would have the advantage, that the code will still work, if in the future a MIPS toolchain adds this symbol and we can add support for that version of the toolchain with the proper version guards.

Do you like this plan?

[Andras Becsi]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 184530 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184530&action=review
> > 
> > >> Source/WTF/wtf/Atomics.cpp:90
> > >> +int64_t __sync_add_and_fetch_8(int64_t volatile* addend, int64_t value)
> > > 
> > > __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> > 
> > So this is a workaround for a bug in the toolchain that is missing this symbol? Maybe that should be explained in a comment and the ChangeLog? What if the toolchain is newer and suddenly provides the symbol? Are version guards missing?
> 
> 
> Dear Simon, 
> 
> Thank you for reviewing.
> 
> Currently none of the MIPS32 toolchains defines this symbol, that is why we did not provide version guards. Another possibility would be to add this MIPS implementation under a different name, that conforms to the webkit coding style, and select this new symbol using #if CPU(MIPS) in Atomics.h,
> 
> This would have the advantage, that the code will still work, if in the future a MIPS toolchain adds this symbol and we can add support for that version of the toolchain with the proper version guards.
> 
> Do you like this plan?
The newest Android NDK (Revision 8d released in December 2012) already support these 64 bit atomic operations with the shipped 4.6 an 4.7 gcc compilers, as far as I know also for MIPS.

[Gergely Kis]

> The newest Android NDK (Revision 8d released in December 2012) already support these 64 bit atomic operations with the shipped 4.6 an 4.7 gcc compilers, as far as I know also for MIPS.

No, on MIPS it does not: 

kisg@KISGPC:/data/usr/android-ndk/toolchains/mipsel-linux-android-4.7/prebuilt/linux-x86/bin$ ./mipsel-linux-android-gcc -I/data/usr/android-ndk-r8d/platforms/android-14/arch-mips/usr/include --sysroot=/data/usr/android-ndk-r8d/platforms/android-14/arch-mips /data/synctest.c 
/tmp/ccaK9ive.o: In function `main':
synctest.c:(.text+0x40): undefined reference to `__sync_add_and_fetch_8'
collect2: error: ld returned 1 exit status

[Andras Becsi]

(In reply to comment #21)
> > The newest Android NDK (Revision 8d released in December 2012) already support these 64 bit atomic operations with the shipped 4.6 an 4.7 gcc compilers, as far as I know also for MIPS.
> 
> No, on MIPS it does not: 
> 
> kisg@KISGPC:/data/usr/android-ndk/toolchains/mipsel-linux-android-4.7/prebuilt/linux-x86/bin$ ./mipsel-linux-android-gcc -I/data/usr/android-ndk-r8d/platforms/android-14/arch-mips/usr/include --sysroot=/data/usr/android-ndk-r8d/platforms/android-14/arch-mips /data/synctest.c 
> /tmp/ccaK9ive.o: In function `main':
> synctest.c:(.text+0x40): undefined reference to `__sync_add_and_fetch_8'
> collect2: error: ld returned 1 exit status

Indeed, it looks like it was only introduced in the ARM toolchain.
Sorry for the noise.

[Balazs Kilvady]

Created attachment 186335 [details]
fixed WTFized patch.

Rebased on r141751.

[WebKit Review Bot]

Attachment 186335 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:90:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:95:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 187614 [details]
fixed WTFized patch.

Rebased on r142489.

[WebKit Review Bot]

Attachment 187614 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:90:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:95:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 184530 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184530&action=review
> > 
> > >> Source/WTF/wtf/Atomics.cpp:90
> > >> +int64_t __sync_add_and_fetch_8(int64_t volatile* addend, int64_t value)
> > > 
> > > __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> > 
> > So this is a workaround for a bug in the toolchain that is missing this symbol? Maybe that should be explained in a comment and the ChangeLog? What if the toolchain is newer and suddenly provides the symbol? Are version guards missing?
> 
> 
> Dear Simon, 
> 
> Thank you for reviewing.
> 
> Currently none of the MIPS32 toolchains defines this symbol, that is why we did not provide version guards. Another possibility would be to add this MIPS implementation under a different name, that conforms to the webkit coding style, and select this new symbol using #if CPU(MIPS) in Atomics.h,
> 
> This would have the advantage, that the code will still work, if in the future a MIPS toolchain adds this symbol and we can add support for that version of the toolchain with the proper version guards.
> 
> Do you like this plan?

Well, what is the chance of plan C, i.e. fixing this where it should be fixed, upstream in (lib)gcc?

If we land a workaround in WebKit for this, then IMHO it should include a link to the upstream bug report and it should be temporary.

_If_ it becomes clear that this does not belong into libgcc (if upstream comes up with a good reason of say rejecting a bug report), then adding the missing functionality under a different name (not using the gcc symbol names) sounds like a good idea. It can then be explained properly in the ChangeLog.

[Chao-ying Fu]

(In reply to comment #27)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 184530 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=184530&action=review
> > > 
> > > >> Source/WTF/wtf/Atomics.cpp:90
> > > >> +int64_t __sync_add_and_fetch_8(int64_t volatile* addend, int64_t value)
> > > > 
> > > > __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> > > 
> > > So this is a workaround for a bug in the toolchain that is missing this symbol? Maybe that should be explained in a comment and the ChangeLog? What if the toolchain is newer and suddenly provides the symbol? Are version guards missing?
> > 
> > 
> > Dear Simon, 
> > 
> > Thank you for reviewing.
> > 
> > Currently none of the MIPS32 toolchains defines this symbol, that is why we did not provide version guards. Another possibility would be to add this MIPS implementation under a different name, that conforms to the webkit coding style, and select this new symbol using #if CPU(MIPS) in Atomics.h,
> > 
> > This would have the advantage, that the code will still work, if in the future a MIPS toolchain adds this symbol and we can add support for that version of the toolchain with the proper version guards.
> > 
> > Do you like this plan?
> 
> Well, what is the chance of plan C, i.e. fixing this where it should be fixed, upstream in (lib)gcc?
> 
> If we land a workaround in WebKit for this, then IMHO it should include a link to the upstream bug report and it should be temporary.

FYI.  I just opened a gcc bug as follows.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56300
Bug 56300 - Add __sync_fetch_and_add_8 and other 8-byte atomic functions to 32-bit MIPS targets

Thanks!

> 
> _If_ it becomes clear that this does not belong into libgcc (if upstream comes up with a good reason of say rejecting a bug report), then adding the missing functionality under a different name (not using the gcc symbol names) sounds like a good idea. It can then be explained properly in the ChangeLog.

[Chao-ying Fu]

From Andrew Pinkski, GCC 4.8 has a new libatomic to support 8-byte atomic functions.  For GCC before version 4.8, we can provide this patch with either same or different function names.

When GCC 4.8 toolchain is ready to compile webkit, we can submit another patch to enable the usage of libatomic for 32-bit MIPS targets.

Is this approach ok?  Thanks!

[Simon Hausmann]

(In reply to comment #29)
> From Andrew Pinkski, GCC 4.8 has a new libatomic to support 8-byte atomic functions.  For GCC before version 4.8, we can provide this patch with either same or different function names.
> 
> When GCC 4.8 toolchain is ready to compile webkit, we can submit another patch to enable the usage of libatomic for 32-bit MIPS targets.

Excellent. I understand that gcc 4.8 is still in development, so it seems reasonable to me to place a workaround into WebKit.

I can't review the quality of the implementation itself (I'm not an expert on that topic), but it would seem to me that the surrounding #ifdefs could be limited to MIPS and GCC < 4.8, as well as the file name perhaps?

[Balazs Kilvady]

Created attachment 188102 [details]
Fixed WTFized patch.

[WebKit Review Bot]

Attachment 188102 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:96:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:101:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 188109 [details]
Fixed WTFized patch.

[WebKit Review Bot]

Attachment 188109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:96:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:101:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 188857 [details]
Fixed WTFized patch.

Rebased on r142863.

[WebKit Review Bot]

Attachment 188857 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:96:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:101:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tomas Popela]

Please edit /Source/WTF/GNUmakefile.list.am too as GTK is also affected.

[Balazs Kilvady]

(In reply to comment #37)
> Please edit /Source/WTF/GNUmakefile.list.am too as GTK is also affected.

In progress…

[Balazs Kilvady]

Created attachment 189059 [details]
Fixed WTFized patch.

Atomics.cpp added to automake (for GTK) and to cmake.

[WebKit Review Bot]

Attachment 189059 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp', u'Source/WTF/wtf/CMakeLists.txt']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:97:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:102:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kilvady]

Created attachment 190283 [details]
Fixed WTFized patch.

Rebased on r144054.

[WebKit Review Bot]

Attachment 190283 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.pro', u'Source/WTF/wtf/Atomics.cpp', u'Source/WTF/wtf/CMakeLists.txt']" exit_code: 1
Source/WTF/wtf/Atomics.cpp:97:  __sync_add_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WTF/wtf/Atomics.cpp:102:  __sync_sub_and_fetch_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 190283 [details]
Fixed WTFized patch.

Clearing flags on attachment: 190283

Committed r144077: <http://trac.webkit.org/changeset/144077>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.