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

Description Csaba Osztrogonác 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.
Comment 1 Csaba Osztrogonác 2013-01-12 03:04:43 PST
Created attachment 182464 [details]
Patch
Comment 2 Csaba Osztrogonác 2013-01-12 03:06:32 PST
It is the only one blocker now to fix the Qt WK2 build on Linux.
Comment 3 Csaba Osztrogonác 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.
Comment 4 Zan Dobersek 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.
Comment 5 Benjamin Poulain 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?
Comment 6 Csaba Osztrogonác 2013-01-12 11:34:12 PST
Created attachment 182474 [details]
Patch
Comment 7 Csaba Osztrogonác 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 ...
Comment 8 Benjamin Poulain 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.
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2013-01-12 12:24:50 PST
All reviewed patches have been landed.  Closing bug.