Bug 106729 – Use __sync_add_and_fetch instead of __gnu_cxx::__exchange_and_add

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 182464 [details]
Patch

[Csaba Osztrogonác]

It is the only one blocker now to fix the Qt WK2 build on Linux.

[Csaba Osztrogonác]

Comment on attachment 182464 [details]
Patch

and add cq?, because I won't be online all the weekend to land it manually.

[Zan Dobersek]

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

Created attachment 182474 [details]
Patch

[Csaba Osztrogonác]

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

> > 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 on attachment 182474 [details]
Patch

Clearing flags on attachment: 182474

Committed r139553: <http://trac.webkit.org/changeset/139553>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.