Bug 106729

Summary: Use __sync_add_and_fetch instead of __gnu_cxx::__exchange_and_add
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Web Template FrameworkAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Blocker CC: andersca, ap, bdakin, beidson, benjamin, darin, d-r, enrica, hausmann, hyatt, kling, mitz, mjs, mrowe, ojan.autocc, ossy, sam, simon.fraser, thorton, tmpsantos, webkit.review.bot, zan
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 106739, 106740    
Bug Blocks: 106708    
Attachments:
Description Flags
Patch
none
Patch none

Csaba Osztrogonác
Reported 2013-01-12 03:02:41 PST
https://trac.webkit.org/changeset/139514 broke the GCC WK2 builds (EFL,Qt,GTK), because atomicIncrement implementation in WTF use __gnu_cxx::__exchange_and_add which supports only 32 bit int values. We should use __sync_add_and_fetch instead of __gnu_cxx::__exchange_and_add which supports all types. Fix is coming.
Attachments
Patch (1.76 KB, patch)
2013-01-12 03:04 PST, Csaba Osztrogonác 		no flags
DetailsFormatted DiffDiff
Patch (2.00 KB, patch)
2013-01-12 11:34 PST, Csaba Osztrogonác 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2013-01-12 03:04:43 PST
Created attachment 182464 [details] Patch
Csaba Osztrogonác
Comment 2 2013-01-12 03:06:32 PST
It is the only one blocker now to fix the Qt WK2 build on Linux.
Csaba Osztrogonác
Comment 3 2013-01-12 03:15:58 PST
Comment on attachment 182464 [details] Patch and add cq?, because I won't be online all the weekend to land it manually.
Zan Dobersek
Comment 4 2013-01-12 05:56:51 PST
(In reply to comment #2) > It is the only one blocker now to fix the Qt WK2 build on Linux. Same for the GTK port.
Benjamin Poulain
Comment 5 2013-01-12 11:09:07 PST
Comment on attachment 182464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182464&action=review > Source/WTF/ChangeLog:7 > + Use __sync_add_and_fetch instead of __gnu_cxx::__exchange_and_add > + https://bugs.webkit.org/show_bug.cgi?id=106729 > + > + Reviewed by NOBODY (OOPS!). > + You should have a bug description in your change log. > Source/WTF/ChangeLog:11 > + (WTF): You can remove this. > Source/WTF/wtf/Atomics.h:125 > +inline int atomicIncrement(int volatile* addend) { return __sync_add_and_fetch(addend, 1); } > +inline int atomicDecrement(int volatile* addend) { return __sync_sub_and_fetch(addend, 1); } > + > +inline int64_t atomicIncrement(int64_t volatile* addend) { return __sync_add_and_fetch(addend, 1); } > +inline int64_t atomicDecrement(int64_t volatile* addend) { return __sync_sub_and_fetch(addend, 1); } Is there any reason to keep this separated from Darwin with the change?
Csaba Osztrogonác
Comment 6 2013-01-12 11:34:12 PST
Created attachment 182474 [details] Patch
Csaba Osztrogonác
Comment 7 2013-01-12 11:36:37 PST
(In reply to comment #5) > (From update of attachment 182464 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182464&action=review > > > Source/WTF/ChangeLog:7 > > + Use __sync_add_and_fetch instead of __gnu_cxx::__exchange_and_add > > + https://bugs.webkit.org/show_bug.cgi?id=106729 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > You should have a bug description in your change log. Done. > > Source/WTF/ChangeLog:11 > > + (WTF): > > You can remove this. Done. > > Source/WTF/wtf/Atomics.h:125 > > +inline int atomicIncrement(int volatile* addend) { return __sync_add_and_fetch(addend, 1); } > > +inline int atomicDecrement(int volatile* addend) { return __sync_sub_and_fetch(addend, 1); } > > + > > +inline int64_t atomicIncrement(int64_t volatile* addend) { return __sync_add_and_fetch(addend, 1); } > > +inline int64_t atomicDecrement(int64_t volatile* addend) { return __sync_sub_and_fetch(addend, 1); } > > Is there any reason to keep this separated from Darwin with the change? I have no idea why Darwin implementation is different, it uses OSAtomicIncrement32Barrier and OSAtomicIncrement64Barrier. I don't know if Darwin has __sync_sub_and_fetch ...
Benjamin Poulain
Comment 8 2013-01-12 11:40:43 PST
> > Is there any reason to keep this separated from Darwin with the change? > I have no idea why Darwin implementation is different, it uses OSAtomicIncrement32Barrier and OSAtomicIncrement64Barrier. I don't > know if Darwin has __sync_sub_and_fetch ... It is just a compiler builtin. I don't see why it would not be on Darwin. That's not a big concern, let's land this so Qt/GTK/EFL build again.
WebKit Review Bot
Comment 9 2013-01-12 12:24:45 PST
Comment on attachment 182474 [details] Patch Clearing flags on attachment: 182474 Committed r139553: <http://trac.webkit.org/changeset/139553>
WebKit Review Bot
Comment 10 2013-01-12 12:24:50 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.