Bug 106739

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

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.

Created attachment 183178 [details] proposed patch.

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.

Created attachment 183766 [details] proposed patch. Rebased on r140325.

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.

Created attachment 184191 [details] proposed patch with improved init. Init method improved to use less allocation/new. Also rebased on r140530.

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.

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?

(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.

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.

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.

Comment on attachment 184526 [details] proposed WTFized patch. Attachment 184526 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16110174

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

Created attachment 184530 [details] fixed WTFized patch.

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.

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

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?

(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?

(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.

> 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

(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.

Created attachment 186335 [details] fixed WTFized patch. Rebased on r141751.

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.

Created attachment 187614 [details] fixed WTFized patch. Rebased on r142489.

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.

(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.

(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.

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!

(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?

Created attachment 188102 [details] Fixed WTFized patch.

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.

Created attachment 188109 [details] Fixed WTFized patch.

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.

Created attachment 188857 [details] Fixed WTFized patch. Rebased on r142863.

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.

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

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

Created attachment 189059 [details] Fixed WTFized patch. Atomics.cpp added to automake (for GTK) and to cmake.

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.

Created attachment 190283 [details] Fixed WTFized patch. Rebased on r144054.

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.

Comment on attachment 190283 [details] Fixed WTFized patch. Clearing flags on attachment: 190283 Committed r144077: <http://trac.webkit.org/changeset/144077>

All reviewed patches have been landed. Closing bug.