diff mbox series

[2/2] meson: fix CONFIG_ATOMIC128 check

Message ID 20220228120720.722632-2-marcandre.lureau@redhat.com
State New
Headers show
Series [1/2] meson: move int128 checks from configure | expand

Commit Message

Marc-André Lureau Feb. 28, 2022, 12:07 p.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

testfile.c: In function 'main':
testfile.c:5:11: error: incorrect number of arguments to function '__atomic_load'
    5 |       y = __atomic_load(&x, 0);
      |           ^~~~~~~~~~~~~
testfile.c:6:7: error: argument 2 of '__atomic_store' must be a pointer type
    6 |       __atomic_store(&x, y, 0);
      |       ^~~~~~~~~~~~~~
testfile.c:7:7: error: argument 3 of '__atomic_compare_exchange' must be a pointer type
    7 |       __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~

And it must be linked with -latomic.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 28, 2022, 12:43 p.m. UTC | #1 
On 28/2/22 13:07, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> testfile.c: In function 'main':
> testfile.c:5:11: error: incorrect number of arguments to function '__atomic_load'
>      5 |       y = __atomic_load(&x, 0);
>        |           ^~~~~~~~~~~~~
> testfile.c:6:7: error: argument 2 of '__atomic_store' must be a pointer type
>      6 |       __atomic_store(&x, y, 0);
>        |       ^~~~~~~~~~~~~~
> testfile.c:7:7: error: argument 3 of '__atomic_compare_exchange' must be a pointer type
>      7 |       __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
>        |       ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> And it must be linked with -latomic.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   meson.build | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index a9ec3974bc67..a3d8af7a501b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1841,13 +1841,17 @@ if has_int128
>       int main(void)
>       {
>         unsigned __int128 x = 0, y = 0;
> -      y = __atomic_load(&x, 0);
> -      __atomic_store(&x, y, 0);
> -      __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
> +      __atomic_load(&x, &y, 0);
> +      __atomic_store(&x, &y, 0);
> +      __atomic_compare_exchange(&x, &y, &x, 0, 0, 0);
>         return 0;
> -    }''')
> +    }''', args: ['-latomic'])
>   
>     config_host_data.set('CONFIG_ATOMIC128', has_atomic128)
> +  if has_atomic128
> +    add_global_link_arguments('-latomic',
> +                              native: false, language: ['c', 'cpp', 'objc'])
> +  endif
>   
>     if not has_atomic128
>       has_cmpxchg128 = cc.links('''

Shouldn't this be 1/2 for bisectability?

Anyhow "We deliberately don't link against libatomic", see:
https://lore.kernel.org/qemu-devel/CAFEAcA8V-PAwbtPWghvyjgKtzi949F6g9TnBJpLsGGHt51J64w@mail.gmail.com/
Peter Maydell Feb. 28, 2022, 1:24 p.m. UTC | #2 
On Mon, 28 Feb 2022 at 12:19, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> testfile.c: In function 'main':
> testfile.c:5:11: error: incorrect number of arguments to function '__atomic_load'
>     5 |       y = __atomic_load(&x, 0);
>       |           ^~~~~~~~~~~~~
> testfile.c:6:7: error: argument 2 of '__atomic_store' must be a pointer type
>     6 |       __atomic_store(&x, y, 0);
>       |       ^~~~~~~~~~~~~~
> testfile.c:7:7: error: argument 3 of '__atomic_compare_exchange' must be a pointer type
>     7 |       __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~
>
> And it must be linked with -latomic.

As Philippe says, this isn't right. What the configure test
is checking for is "do we have 128-bit atomics which are
handled inline and specifically not via libatomic". The
reason we can't use libatomic is documented in the comment
starting "GCC is a house divided" in include/qemu/atomic128.h.

thanks
-- PMM
Marc-André Lureau Feb. 28, 2022, 1:28 p.m. UTC | #3 
Hi

On Mon, Feb 28, 2022 at 5:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 28 Feb 2022 at 12:19, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > testfile.c: In function 'main':
> > testfile.c:5:11: error: incorrect number of arguments to function '__atomic_load'
> >     5 |       y = __atomic_load(&x, 0);
> >       |           ^~~~~~~~~~~~~
> > testfile.c:6:7: error: argument 2 of '__atomic_store' must be a pointer type
> >     6 |       __atomic_store(&x, y, 0);
> >       |       ^~~~~~~~~~~~~~
> > testfile.c:7:7: error: argument 3 of '__atomic_compare_exchange' must be a pointer type
> >     7 |       __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
> >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > And it must be linked with -latomic.
>
> As Philippe says, this isn't right. What the configure test
> is checking for is "do we have 128-bit atomics which are
> handled inline and specifically not via libatomic". The
> reason we can't use libatomic is documented in the comment
> starting "GCC is a house divided" in include/qemu/atomic128.h.

The arguments fix is still valid, no?
Peter Maydell Feb. 28, 2022, 1:36 p.m. UTC | #4 
On Mon, 28 Feb 2022 at 13:28, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Mon, Feb 28, 2022 at 5:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 28 Feb 2022 at 12:19, <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > testfile.c: In function 'main':
> > > testfile.c:5:11: error: incorrect number of arguments to function '__atomic_load'
> > >     5 |       y = __atomic_load(&x, 0);
> > >       |           ^~~~~~~~~~~~~
> > > testfile.c:6:7: error: argument 2 of '__atomic_store' must be a pointer type
> > >     6 |       __atomic_store(&x, y, 0);
> > >       |       ^~~~~~~~~~~~~~
> > > testfile.c:7:7: error: argument 3 of '__atomic_compare_exchange' must be a pointer type
> > >     7 |       __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
> > >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > And it must be linked with -latomic.
> >
> > As Philippe says, this isn't right. What the configure test
> > is checking for is "do we have 128-bit atomics which are
> > handled inline and specifically not via libatomic". The
> > reason we can't use libatomic is documented in the comment
> > starting "GCC is a house divided" in include/qemu/atomic128.h.
>
> The arguments fix is still valid, no?

Probably. (Does this mean we've never been correctly setting
CONFIG_ATOMIC128?)

I think we should have the 128-bit atomics check in meson.build
look the same as the existing one for 64-bit atomics, though,
unless there's a reason why they should look different.
For the 64-bit version we explicitly pass __ATOMIC_RELAXED,
and we use __atomic_load_n(), not __atomic_load().

thanks
-- PMM
Richard Henderson Feb. 28, 2022, 6:42 p.m. UTC | #5 
On 2/28/22 03:36, Peter Maydell wrote:
> I think we should have the 128-bit atomics check in meson.build
> look the same as the existing one for 64-bit atomics, though,
> unless there's a reason why they should look different.
> For the 64-bit version we explicitly pass __ATOMIC_RELAXED,
> and we use __atomic_load_n(), not __atomic_load().

Yep, since that's what we use in qatomic_read__nocheck, etc.


r~
diff mbox series

Patch

diff --git a/meson.build b/meson.build
index a9ec3974bc67..a3d8af7a501b 100644
--- a/meson.build
+++ b/meson.build
@@ -1841,13 +1841,17 @@  if has_int128
     int main(void)
     {
       unsigned __int128 x = 0, y = 0;
-      y = __atomic_load(&x, 0);
-      __atomic_store(&x, y, 0);
-      __atomic_compare_exchange(&x, &y, x, 0, 0, 0);
+      __atomic_load(&x, &y, 0);
+      __atomic_store(&x, &y, 0);
+      __atomic_compare_exchange(&x, &y, &x, 0, 0, 0);
       return 0;
-    }''')
+    }''', args: ['-latomic'])
 
   config_host_data.set('CONFIG_ATOMIC128', has_atomic128)
+  if has_atomic128
+    add_global_link_arguments('-latomic',
+                              native: false, language: ['c', 'cpp', 'objc'])
+  endif
 
   if not has_atomic128
     has_cmpxchg128 = cc.links('''