Bug 222959 – [CMake] Build fails on RISC-V with GCC 11

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Khem Raj]

with GCC 11 there is additional use of C11 atomics in WTF code, which is causing the builds to fails like below

/usr/src/debug/webkitgtk/2.30.5-r0/webkitgtk-2.30.5/Source/WTF/wtf/Assertions.cpp:226: undefined reference to `__atomic_compare_exchange_1'

[Khem Raj]

Created attachment 422662 [details]
potential fix

this fixes the problem. But perhaps better would be to let CMake'ry detect if linking with -latomic is needed or not. Since it may not be necessary on all platforms.

[Daniel Kolesa]

there is already build system stuff in place to check for libatomic (since a bunch of 32-bit targets, notably armv6, powerpc and mips, do not have hardware 64-bit operations)

risc-v has them, but __atomic_compare_exchange_1 is a 1-byte CAS, and operations on values less than 4 bytes are apparently not always lock-free on risc-v, so basically you just need to extend the existing checks that are in place

[Khem Raj]

(In reply to Daniel Kolesa from comment #2)
> there is already build system stuff in place to check for libatomic (since a
> bunch of 32-bit targets, notably armv6, powerpc and mips, do not have
> hardware 64-bit operations)
> 
> risc-v has them, but __atomic_compare_exchange_1 is a 1-byte CAS, and
> operations on values less than 4 bytes are apparently not always lock-free
> on risc-v, so basically you just need to extend the existing checks that are
> in place

right, could you help pointing to the test I can enhance and try out ?

[Daniel Kolesa]

sure, here you go https://github.com/WebKit/WebKit/blob/main/Source/cmake/WebKitCompilerFlags.cmake#L275

the test should do a compare-and-swap on int8_t, using the standard C++ atomic API

[Daniel Kolesa]

oh, and you can probably just put it in the existing compile-test (rather than making a separate one with a separate variable; the outcome in either case is "we want libatomic" which means if you just do both operations in the same test, it doesn't matter if just one fails or both fail; i'd also rename ATOMIC_INT64_IS_BUILTIN to something more generic, like say, ATOMIC_NEED_LIBATOMIC

[Khem Raj]

Created attachment 422760 [details]
potential fix

[Adrian Perez]

Comment on attachment 422760 [details]
potential fix

Thanks! The patch looks good to me, but it's lacking a ChangeLog entry.

Could you add one and re-submit the patch? You can use the script
at “Tools/Scripts/prepare-ChangeLog” to generate a new entry and
fill the details there.

There is more information about how to add ChangeLog entries here:

  https://webkit.org/contributing-code/#changelog-files

I hope this helps :)

[Khem Raj]

Created attachment 422903 [details]
potential fix 2

[Adrian Perez]

Comment on attachment 422903 [details]
potential fix 2

Hi again!

The patch is not *almost* ready. There are a couple of small changes
needed—please take a look.

And no worries, it's normal to run into these issues when submitting
patches to WebKit in the beginning, thanks for your patience.  :) 

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

> ChangeLog:4
> +	architevtures (e.g. RISCV) operations on less than 4 bytes are not lock-free 

This line is using tabs for indentation. In the ChangeLog files we use
always 8 spaces. This is the reason why the “style” build bot is red.
You can click the status bubbles to read the logs, in this case the
log is at:

  https://ews-build.webkit.org/#/builders/6/builds/42288

You can use ”Tools/Scripts/check-webkit-style” if you want to verify
patches before submitting them :)

> ChangeLog:8
> +        Reviewed by Adrian Perez.

This should be “Reviewed by Adrian Perez de Castro“, because that is how
my name appears in “Tools/Scripts/webkitpy/common/config/contributors.json”

Even better: It is possible to leave the “Reviewed by OOPS” line without
typing the name of the reviewer, and then when a patch is marked as reviewed
(r+) in Bugzilla the “OOPS” will be replaced automatically when the patch gets
merged in the repository :)

[Adrian Perez]

Ah, and I forgot to mention that we have “Tools/Scripts/webkit-path”
that can upload patches to Bugzilla automatically from the command line.

[Adrian Perez]

(In reply to Adrian Perez from comment #10)
> Ah, and I forgot to mention that we have “Tools/Scripts/webkit-path”
> that can upload patches to Bugzilla automatically from the command line.

I meant ”webkit-patch” O:-)

[Daniel Kolesa]

the test is not correct, the undefined reference was `__atomic_compare_exchange_1` while the one you're testing is `__atomic_fetch_add_1`

[Khem Raj]

(In reply to Daniel Kolesa from comment #12)
> the test is not correct, the undefined reference was
> `__atomic_compare_exchange_1` while the one you're testing is
> `__atomic_fetch_add_1`

yes, although I saw that none of 1byte ops were not available as builtins since the test was simple for __atomic_fetch_add_1, I went with it. but perhaps using __atomic_compare_exchange_1 for test would be more accurate.

[Khem Raj]

(In reply to Adrian Perez from comment #10)
> Ah, and I forgot to mention that we have “Tools/Scripts/webkit-path”
> that can upload patches to Bugzilla automatically from the command line.

it does not work on my host (archlinux ) see http://sprunge.us/XUqwhd

[Khem Raj]

Created attachment 422956 [details]
potential fix 3

[Khem Raj]

(In reply to Daniel Kolesa from comment #12)
> the test is not correct, the undefined reference was
> `__atomic_compare_exchange_1` while the one you're testing is
> `__atomic_fetch_add_1`

I have addressed it in v3 of patch

[Carlos Alberto Lopez Perez]

Comment on attachment 422956 [details]
potential fix 3

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

> Source/WebKit/ChangeLog:4
> +        Use renamed ATOMICS_REQUIRE_LIBATOMIC instead of
> +       ATOMIC_INT64_REQUIRES_LIBATOMIC.

We don't usually wrap long lines on WebKit. The practice is to use just one line for all the subject even if this is many characters long.

> Source/WebKit/ChangeLog:6
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=222959

Also we don't use an extra empty line between the subjet and the bug link

> Source/WebKit/ChangeLog:8
> +        Reviewed by OOPS.

This should be "Reviewed by NOBODY (OOPS!)."
That is important because the tooling we use to land patches looks for those exact words to later replace the "NOBODY (OOPS!)" with the name of the reviewer.

I know this sounds like a lot of bureaucracy at first, but once you get used to it is pretty straightforward.

To generate the changelogs we have a script. We don't create them by hand.
It is used like this:

Tools/Scripts/prepare-ChangeLog -b ${bugzillanumber} -g ${localgitcommitforchangelog}

For example, assuming you are checked out on the patch to upload and this patch is committed locally

Tools/Scripts/prepare-ChangeLog -b 222959 -g HEAD

It will generate the changelog files, then you just edit under the reviewed line it will appear the message of your local commit.. you edit that to tell a bit about what the patch does. Then you add the changelogs to git and commit them by amending the previos commit, then its ready to upload.

You can generate a patch and upload it here manually or you can use another script to upload the patch directly.

Tools/Scripts/webkit-patch upload -g ${localgitcommittobeuploaded} --suggest-reviewers

It will automatically detect the bug number where the patch should be uploaded from the changelog

> Source/cmake/WebKitCompilerFlags.cmake:284
>  if (COMPILER_IS_GCC_OR_CLANG)
>      set(ATOMIC_TEST_SOURCE "
> -        #include <atomic>
> -        int main() { std::atomic<int64_t> i(0); i++; return 0; }
> +	#include <atomic>
> +	int main() {
> +          std::atomic<bool> y (false);
> +          std::atomic<uint64_t> x (0);
> +          bool expected = true;
> +          bool j = y.compare_exchange_weak(expected,false);
> +          x++;
> +          return 0;
> +        }

The Early Warning System (EWS) bot for checking the style is complaining here about mixed spaces/tabs

ERROR: Source/cmake/WebKitCompilerFlags.cmake:276:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/cmake/WebKitCompilerFlags.cmake:277:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 8 files
program finished with exit code 1

^^ https://ews-build.webkit.org/#/builders/6/builds/42334

[Khem Raj]

Created attachment 422979 [details]
potential fix 4

[Khem Raj]

(In reply to Carlos Alberto Lopez Perez from comment #17)
> Comment on attachment 422956 [details]
> potential fix 3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422956&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        Use renamed ATOMICS_REQUIRE_LIBATOMIC instead of
> > +       ATOMIC_INT64_REQUIRES_LIBATOMIC.
> 
> We don't usually wrap long lines on WebKit. The practice is to use just one
> line for all the subject even if this is many characters long.

Done.

> 
> > Source/WebKit/ChangeLog:6
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=222959
> 
> Also we don't use an extra empty line between the subjet and the bug link

Thanks done, I generated them but then edited them heavily :)

> 
> > Source/WebKit/ChangeLog:8
> > +        Reviewed by OOPS.
> 
> This should be "Reviewed by NOBODY (OOPS!)."
> That is important because the tooling we use to land patches looks for those
> exact words to later replace the "NOBODY (OOPS!)" with the name of the
> reviewer.

Good to know. done.

> 
> I know this sounds like a lot of bureaucracy at first, but once you get used
> to it is pretty straightforward.
> 

yeah in the world after git, CHANGELOGs have become out of fashion but I know
few other projects who still use them.


> To generate the changelogs we have a script. We don't create them by hand.
> It is used like this:
> 
> Tools/Scripts/prepare-ChangeLog -b ${bugzillanumber} -g
> ${localgitcommitforchangelog}
> 
> For example, assuming you are checked out on the patch to upload and this
> patch is committed locally
> 
> Tools/Scripts/prepare-ChangeLog -b 222959 -g HEAD
> 
> It will generate the changelog files, then you just edit under the reviewed
> line it will appear the message of your local commit.. you edit that to tell
> a bit about what the patch does. Then you add the changelogs to git and
> commit them by amending the previos commit, then its ready to upload.
> 
> You can generate a patch and upload it here manually or you can use another
> script to upload the patch directly.
> 
> Tools/Scripts/webkit-patch upload -g ${localgitcommittobeuploaded}
> --suggest-reviewers

I think these python tools expect python to point to python2 or something like that, on my machine python points to python3, perhaps that's the issue but it does not work as expected. I manually changed the interpreter in one of these tools to call for python2 and it did lot better. 

> 
> It will automatically detect the bug number where the patch should be
> uploaded from the changelog
> 
> > Source/cmake/WebKitCompilerFlags.cmake:284
> >  if (COMPILER_IS_GCC_OR_CLANG)
> >      set(ATOMIC_TEST_SOURCE "
> > -        #include <atomic>
> > -        int main() { std::atomic<int64_t> i(0); i++; return 0; }
> > +	#include <atomic>
> > +	int main() {
> > +          std::atomic<bool> y (false);
> > +          std::atomic<uint64_t> x (0);
> > +          bool expected = true;
> > +          bool j = y.compare_exchange_weak(expected,false);
> > +          x++;
> > +          return 0;
> > +        }
> 
> The Early Warning System (EWS) bot for checking the style is complaining
> here about mixed spaces/tabs
> 
> ERROR: Source/cmake/WebKitCompilerFlags.cmake:276:  Line contains tab
> character.  [whitespace/tab] [5]
> ERROR: Source/cmake/WebKitCompilerFlags.cmake:277:  Line contains tab
> character.  [whitespace/tab] [5]
> Total errors found: 2 in 8 files
> program finished with exit code 1

Fixes, my editor is as adamant as me :) usually its smart about indenting
and detects the style from sources, but in a CMakeFile embedding C++ code it
falls back to my defaults which causes these issues. 

Is there a way to run all EWS tests locally before submitting ?

> 
> ^^ https://ews-build.webkit.org/#/builders/6/builds/42334

[Daniel Kolesa]

(In reply to Khem Raj from comment #13)
> (In reply to Daniel Kolesa from comment #12)
> > the test is not correct, the undefined reference was
> > `__atomic_compare_exchange_1` while the one you're testing is
> > `__atomic_fetch_add_1`
> 
> yes, although I saw that none of 1byte ops were not available as builtins
> since the test was simple for __atomic_fetch_add_1, I went with it. but
> perhaps using __atomic_compare_exchange_1 for test would be more accurate.

well, the reason i'm saying this is mainly that IMO we should be testing for the functions that are actually missing from the build

gcc claims that the hardware support for less-than-4-byte-atomics is partial in risc-v, idk what it means in practice though (haven't looked into the cross-compiler output more deeply)

[Khem Raj]

(In reply to Daniel Kolesa from comment #20)
> (In reply to Khem Raj from comment #13)
> > (In reply to Daniel Kolesa from comment #12)
> > > the test is not correct, the undefined reference was
> > > `__atomic_compare_exchange_1` while the one you're testing is
> > > `__atomic_fetch_add_1`
> > 
> > yes, although I saw that none of 1byte ops were not available as builtins
> > since the test was simple for __atomic_fetch_add_1, I went with it. but
> > perhaps using __atomic_compare_exchange_1 for test would be more accurate.
> 
> well, the reason i'm saying this is mainly that IMO we should be testing for
> the functions that are actually missing from the build
> 
> gcc claims that the hardware support for less-than-4-byte-atomics is partial
> in risc-v, idk what it means in practice though (haven't looked into the
> cross-compiler output more deeply)

right, I have tweaked the test to check for __atomic_compare_exchange_1 in latest patch.

[Carlos Alberto Lopez Perez]

Comment on attachment 422979 [details]
potential fix 4

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

Some nitpicks with the format used for the Changelogs, commenting in-line.
Other than that I think the patch is fine.
Thanks for your patience :)

> ChangeLog:4
> +        Check for 1 byte atomic operations along with 64bit ones, some
> +        architevtures (e.g. RISCV) operations on less than 4 bytes are not lock-free

The title used on the Changelog files should be the same title used on the bug. So this title should be:
[CMake] Build fails on RISC-V with GCC 11

The explanation "Check for 1 byte atomic operations along with 64bit ones, some architevtures (e.g. RISCV) operations on less than 4 bytes are not lock-free" should go under the "Reviewed by ... line

> ChangeLog:7
> +        Reviewed by Reviewed by NOBODY (OOPS!).

This reviewed line has errors. It should be "Reviewed by NOBODY (OOPS!)." and not " Reviewed by Reviewed by NOBODY (OOPS!)."

> Source/JavaScriptCore/ChangeLog:3
> +        Use renamed variable ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC

Here the title should be the same than above

> Source/JavaScriptCore/ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=222959
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Below this line is where the explanation "Use renamed variable ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC" should be written, but the subject (what goes above the bugzilla bug link) should be the same than the bug title.

> Source/WTF/ChangeLog:3
> +        Link with libatomic if ATOMICS_REQUIRE_LIBATOMIC is set.

Here the same issues: the title should be the one of the bug, and this explanation should go below the "Reviewed by" line

> Source/WebKit/ChangeLog:3
> +        Use renamed ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC.

Same issue with the title (should be the same title than the bug)

[Khem Raj]

Created attachment 423303 [details]
potential fix 5

[Khem Raj]

(In reply to Carlos Alberto Lopez Perez from comment #22)
> Comment on attachment 422979 [details]
> potential fix 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422979&action=review
> 
> Some nitpicks with the format used for the Changelogs, commenting in-line.
> Other than that I think the patch is fine.
> Thanks for your patience :)
> 
> > ChangeLog:4
> > +        Check for 1 byte atomic operations along with 64bit ones, some
> > +        architevtures (e.g. RISCV) operations on less than 4 bytes are not lock-free
> 
> The title used on the Changelog files should be the same title used on the
> bug. So this title should be:
> [CMake] Build fails on RISC-V with GCC 11
> 
> The explanation "Check for 1 byte atomic operations along with 64bit ones,
> some architevtures (e.g. RISCV) operations on less than 4 bytes are not
> lock-free" should go under the "Reviewed by ... line
> 
> > ChangeLog:7
> > +        Reviewed by Reviewed by NOBODY (OOPS!).
> 
> This reviewed line has errors. It should be "Reviewed by NOBODY (OOPS!)."
> and not " Reviewed by Reviewed by NOBODY (OOPS!)."
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        Use renamed variable ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC
> 
> Here the title should be the same than above
> 
> > Source/JavaScriptCore/ChangeLog:7
> > +        https://bugs.webkit.org/show_bug.cgi?id=222959
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> Below this line is where the explanation "Use renamed variable
> ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC" should
> be written, but the subject (what goes above the bugzilla bug link) should
> be the same than the bug title.
> 
> > Source/WTF/ChangeLog:3
> > +        Link with libatomic if ATOMICS_REQUIRE_LIBATOMIC is set.
> 
> Here the same issues: the title should be the one of the bug, and this
> explanation should go below the "Reviewed by" line
> 
> > Source/WebKit/ChangeLog:3
> > +        Use renamed ATOMICS_REQUIRE_LIBATOMIC instead of ATOMIC_INT64_REQUIRES_LIBATOMIC.
> 
> Same issue with the title (should be the same title than the bug)

OK thanks for help. I have used ./Tools/./Scripts/prepare-ChangeLog
this time, but then again I had to edit it manually so hopefully its ok this time

[Carlos Alberto Lopez Perez]

Comment on attachment 423303 [details]
potential fix 5

Looks good. Thanks!

[EWS]

Committed r274476: <https://commits.webkit.org/r274476>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423303 [details].