qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* libqemuutil
@ 2021-03-16  9:07 Markus Armbruster
  2021-03-16  9:24 ` libqemuutil Thomas Huth
  2021-03-16  9:28 ` libqemuutil Paolo Bonzini
  0 siblings, 2 replies; 6+ messages in thread
From: Markus Armbruster @ 2021-03-16  9:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
	Marc-André Lureau

I rebased my "[PATCH v6 00/10] Configurable policy for handling
deprecated interfaces" to master, and it surprisingly fails to link
several utility programs.  Here's the first error:

    gcc  -o tests/bench/benchmark-crypto-hmac tests/bench/benchmark-crypto-hmac.p/benchmark-crypto-hmac.c.o -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libcrypto.fa libauthz.fa libqom.fa -Wl,--no-whole-archive -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -m64 -fstack-protector-strong -Wl,--start-group libqemuutil.a subprojects/libvhost-user/libvhost-user-glib.a subprojects/libvhost-user/libvhost-user.a libcrypto.fa libauthz.fa libqom.fa -pthread -lgthread-2.0 -lglib-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lgnutls -lutil -lm -lgthread-2.0 -lglib-2.0 -lnettle -lgnutls -lpam -Wl,--end-group
    /usr/bin/ld: libqemuutil.a(util_main-loop.c.o): in function `qemu_set_fd_handler':
    /work/armbru/qemu/bld-x86/../util/main-loop.c:581: multiple definition of `qemu_set_fd_handler'; libqemuutil.a(stubs_set-fd-handler.c.o):/work/armbru/qemu/bld-x86/../stubs/set-fd-handler.c:8: first defined here
    collect2: error: ld returned 1 exit status

Both master and PATCH 01 still link fine, PATCH 02 doesn't.  PATCH 02
doesn't go anywhere near qemu_set_fd_handler().

Turns out libqemuutil.a contains two definitions of
qemu_set_fd_handler().  In master:

    $ nm --defined-only libqemuutil.a | awk '/:$/ { f=$0 } / qemu_set_fd_handler/ { if (f) { print f; f="" } print $0 }'
    util_main-loop.c.o:
    00000000000007fe T qemu_set_fd_handler
    stubs_set-fd-handler.c.o:
    0000000000000000 T qemu_set_fd_handler

This is obviously unhealthy.

I suspect the linker happens to pick the one that makes things work,
until something in my patch makes it pick the other one.

Is qemu_set_fd_handler() the only one?  Nope:

    $ nm --defined-only bld-x86/libqemuutil.a | awk '/ T / { print $NF }' | sort | uniq -c | grep -v '^ *1 '
          2 qemu_set_fd_handler
          2 yank_generic_iochannel
          2 yank_register_function
          2 yank_register_instance
          2 yank_unregister_function
          2 yank_unregister_instance

I didn't run into this issue when I posted my series last Friday.  The
issue now blocks its merge, and today is the soft freeze.  Help!



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: libqemuutil
  2021-03-16  9:07 libqemuutil Markus Armbruster
@ 2021-03-16  9:24 ` Thomas Huth
  2021-03-16  9:41   ` libqemuutil Paolo Bonzini
  2021-03-16  9:28 ` libqemuutil Paolo Bonzini
  1 sibling, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2021-03-16  9:24 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel, Paolo Bonzini
  Cc: Marc-André Lureau, Philippe Mathieu-Daudé

On 16/03/2021 10.07, Markus Armbruster wrote:
> I rebased my "[PATCH v6 00/10] Configurable policy for handling
> deprecated interfaces" to master, and it surprisingly fails to link
> several utility programs.  Here's the first error:
> 
>      gcc  -o tests/bench/benchmark-crypto-hmac tests/bench/benchmark-crypto-hmac.p/benchmark-crypto-hmac.c.o -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive libcrypto.fa libauthz.fa libqom.fa -Wl,--no-whole-archive -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -m64 -fstack-protector-strong -Wl,--start-group libqemuutil.a subprojects/libvhost-user/libvhost-user-glib.a subprojects/libvhost-user/libvhost-user.a libcrypto.fa libauthz.fa libqom.fa -pthread -lgthread-2.0 -lglib-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lgnutls -lutil -lm -lgthread-2.0 -lglib-2.0 -lnettle -lgnutls -lpam -Wl,--end-group
>      /usr/bin/ld: libqemuutil.a(util_main-loop.c.o): in function `qemu_set_fd_handler':
>      /work/armbru/qemu/bld-x86/../util/main-loop.c:581: multiple definition of `qemu_set_fd_handler'; libqemuutil.a(stubs_set-fd-handler.c.o):/work/armbru/qemu/bld-x86/../stubs/set-fd-handler.c:8: first defined here
>      collect2: error: ld returned 1 exit status
> 
> Both master and PATCH 01 still link fine, PATCH 02 doesn't.  PATCH 02
> doesn't go anywhere near qemu_set_fd_handler().
> 
> Turns out libqemuutil.a contains two definitions of
> qemu_set_fd_handler().  In master:
> 
>      $ nm --defined-only libqemuutil.a | awk '/:$/ { f=$0 } / qemu_set_fd_handler/ { if (f) { print f; f="" } print $0 }'
>      util_main-loop.c.o:
>      00000000000007fe T qemu_set_fd_handler
>      stubs_set-fd-handler.c.o:
>      0000000000000000 T qemu_set_fd_handler
> 
> This is obviously unhealthy.
> 
> I suspect the linker happens to pick the one that makes things work,
> until something in my patch makes it pick the other one.
> 
> Is qemu_set_fd_handler() the only one?  Nope:
> 
>      $ nm --defined-only bld-x86/libqemuutil.a | awk '/ T / { print $NF }' | sort | uniq -c | grep -v '^ *1 '
>            2 qemu_set_fd_handler
>            2 yank_generic_iochannel
>            2 yank_register_function
>            2 yank_register_instance
>            2 yank_unregister_function
>            2 yank_unregister_instance
> 
> I didn't run into this issue when I posted my series last Friday.  The
> issue now blocks its merge, and today is the soft freeze.  Help!

A very, very quick-n-dirty band-aid is likely to mark the function in stubs 
as weak:

diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
--- a/stubs/set-fd-handler.c
+++ b/stubs/set-fd-handler.c
@@ -1,6 +1,7 @@
  #include "qemu/osdep.h"
  #include "qemu/main-loop.h"

+__attribute__((weak))
  void qemu_set_fd_handler(int fd,
                           IOHandler *fd_read,
                           IOHandler *fd_write,

  ... should IMHO be good enough for the soft freeze. In the long run, you 
might want to analyze the problem more thoroughly, of course. I had similar 
problems in the past already, and solved them by moving the stubs around. See:

  b0476d6602adbf818132dc896b585e01f47eaf96
  stubs: Move qemu_timer_notify_cb() and remove
  qemu_notify_event() stub

  8c2787629eee73ca8ce4f100cff4f4946583b4e8
  stubs: Move qemu_fd_register stub to util/main-loop.c

HTH,
  Thomas



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: libqemuutil
  2021-03-16  9:07 libqemuutil Markus Armbruster
  2021-03-16  9:24 ` libqemuutil Thomas Huth
@ 2021-03-16  9:28 ` Paolo Bonzini
  2021-03-16 10:09   ` libqemuutil Markus Armbruster
  1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-16  9:28 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel
  Cc: Marc-André Lureau, Philippe Mathieu-Daudé

On 16/03/21 10:07, Markus Armbruster wrote:
> I suspect the linker happens to pick the one that makes things work,
> until something in my patch makes it pick the other one.

Ouch.  Fortunately the stub is unnecessary and can be removed.

----------- 8< ------------
 From fe45350cc11434efe3461c540bb0f258bbe010f7 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 16 Mar 2021 05:25:48 -0400
Subject: [PATCH] qemuutil: remove qemu_set_fd_handler duplicate symbol

libqemuutil has two definitions of qemu_set_fd_handler.  This
is not needed since the only users of the function are
qemu-io.c and the emulators, both of which already include
util/main-loop.c.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/stubs/meson.build b/stubs/meson.build
index a054d5877f..8a3e804cf0 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -34,7 +34,6 @@ stub_ss.add(files('ram-block.c'))
  stub_ss.add(files('ramfb.c'))
  stub_ss.add(files('replay.c'))
  stub_ss.add(files('runstate-check.c'))
-stub_ss.add(files('set-fd-handler.c'))
  stub_ss.add(files('sysbus.c'))
  stub_ss.add(files('target-get-monitor-def.c'))
  stub_ss.add(files('target-monitor-defs.c'))
diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
deleted file mode 100644
index bff7e0a45a..0000000000
--- a/stubs/set-fd-handler.c
+++ /dev/null
@@ -1,10 +0,0 @@
-#include "qemu/osdep.h"
-#include "qemu/main-loop.h"
-
-void qemu_set_fd_handler(int fd,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
-{
-    abort();
-}

> Is qemu_set_fd_handler() the only one?  Nope:
> 
>      $ nm --defined-only bld-x86/libqemuutil.a | awk '/ T / { print $NF }' | sort | uniq -c | grep -v '^ *1 '
>            2 qemu_set_fd_handler
>            2 yank_generic_iochannel
>            2 yank_register_function
>            2 yank_register_instance
>            2 yank_unregister_function
>            2 yank_unregister_instance
> 
> I didn't run into this issue when I posted my series last Friday.  The
> issue now blocks its merge, and today is the soft freeze.  Help!

For yank_*, I suggest moving the non-stub version to monitor/ and adding 
it to the qmp_ss sourceset.

Paolo



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: libqemuutil
  2021-03-16  9:24 ` libqemuutil Thomas Huth
@ 2021-03-16  9:41   ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-16  9:41 UTC (permalink / raw)
  To: Thomas Huth, Markus Armbruster, qemu-devel
  Cc: Marc-André Lureau, Philippe Mathieu-Daudé

On 16/03/21 10:24, Thomas Huth wrote:
> A very, very quick-n-dirty band-aid is likely to mark the function in 
> stubs as weak:

This probably does not work on some of our porting targets (OS X)?  The 
static library was introduced as a low-tech alternative to weak symbols, 
IIRC.

Paolo



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: libqemuutil
  2021-03-16  9:28 ` libqemuutil Paolo Bonzini
@ 2021-03-16 10:09   ` Markus Armbruster
  2021-03-16 10:15     ` libqemuutil Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2021-03-16 10:09 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Marc-André Lureau, Philippe Mathieu-Daudé, qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 16/03/21 10:07, Markus Armbruster wrote:
>> I suspect the linker happens to pick the one that makes things work,
>> until something in my patch makes it pick the other one.
>
> Ouch.  Fortunately the stub is unnecessary and can be removed.
>
> ----------- 8< ------------
> From fe45350cc11434efe3461c540bb0f258bbe010f7 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Tue, 16 Mar 2021 05:25:48 -0400
> Subject: [PATCH] qemuutil: remove qemu_set_fd_handler duplicate symbol
>
> libqemuutil has two definitions of qemu_set_fd_handler.  This
> is not needed since the only users of the function are
> qemu-io.c and the emulators, both of which already include
> util/main-loop.c.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> diff --git a/stubs/meson.build b/stubs/meson.build
> index a054d5877f..8a3e804cf0 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -34,7 +34,6 @@ stub_ss.add(files('ram-block.c'))
>  stub_ss.add(files('ramfb.c'))
>  stub_ss.add(files('replay.c'))
>  stub_ss.add(files('runstate-check.c'))
> -stub_ss.add(files('set-fd-handler.c'))
>  stub_ss.add(files('sysbus.c'))
>  stub_ss.add(files('target-get-monitor-def.c'))
>  stub_ss.add(files('target-monitor-defs.c'))
> diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
> deleted file mode 100644
> index bff7e0a45a..0000000000
> --- a/stubs/set-fd-handler.c
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -#include "qemu/osdep.h"
> -#include "qemu/main-loop.h"
> -
> -void qemu_set_fd_handler(int fd,
> -                         IOHandler *fd_read,
> -                         IOHandler *fd_write,
> -                         void *opaque)
> -{
> -    abort();
> -}

Tested-by: Markus Armbruster <armbru@redhat.com>

I'll include this in my pull request, if you don't mind.

>> Is qemu_set_fd_handler() the only one?  Nope:
>>      $ nm --defined-only bld-x86/libqemuutil.a | awk '/ T / { print
>> $NF }' | sort | uniq -c | grep -v '^ *1 '
>>            2 qemu_set_fd_handler
>>            2 yank_generic_iochannel
>>            2 yank_register_function
>>            2 yank_register_instance
>>            2 yank_unregister_function
>>            2 yank_unregister_instance
>> I didn't run into this issue when I posted my series last Friday.
>> The
>> issue now blocks its merge, and today is the soft freeze.  Help!
>
> For yank_*, I suggest moving the non-stub version to monitor/ and
> adding it to the qmp_ss sourceset.

I can give it a try after the soft freeze.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: libqemuutil
  2021-03-16 10:09   ` libqemuutil Markus Armbruster
@ 2021-03-16 10:15     ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-16 10:15 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Marc-André Lureau, Philippe Mathieu-Daudé, qemu-devel

On 16/03/21 11:09, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 16/03/21 10:07, Markus Armbruster wrote:
>>> I suspect the linker happens to pick the one that makes things work,
>>> until something in my patch makes it pick the other one.
>>
>> Ouch.  Fortunately the stub is unnecessary and can be removed.
>>
>> ----------- 8< ------------
>>  From fe45350cc11434efe3461c540bb0f258bbe010f7 Mon Sep 17 00:00:00 2001
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Date: Tue, 16 Mar 2021 05:25:48 -0400
>> Subject: [PATCH] qemuutil: remove qemu_set_fd_handler duplicate symbol
>>
>> libqemuutil has two definitions of qemu_set_fd_handler.  This
>> is not needed since the only users of the function are
>> qemu-io.c and the emulators, both of which already include
>> util/main-loop.c.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> diff --git a/stubs/meson.build b/stubs/meson.build
>> index a054d5877f..8a3e804cf0 100644
>> --- a/stubs/meson.build
>> +++ b/stubs/meson.build
>> @@ -34,7 +34,6 @@ stub_ss.add(files('ram-block.c'))
>>   stub_ss.add(files('ramfb.c'))
>>   stub_ss.add(files('replay.c'))
>>   stub_ss.add(files('runstate-check.c'))
>> -stub_ss.add(files('set-fd-handler.c'))
>>   stub_ss.add(files('sysbus.c'))
>>   stub_ss.add(files('target-get-monitor-def.c'))
>>   stub_ss.add(files('target-monitor-defs.c'))
>> diff --git a/stubs/set-fd-handler.c b/stubs/set-fd-handler.c
>> deleted file mode 100644
>> index bff7e0a45a..0000000000
>> --- a/stubs/set-fd-handler.c
>> +++ /dev/null
>> @@ -1,10 +0,0 @@
>> -#include "qemu/osdep.h"
>> -#include "qemu/main-loop.h"
>> -
>> -void qemu_set_fd_handler(int fd,
>> -                         IOHandler *fd_read,
>> -                         IOHandler *fd_write,
>> -                         void *opaque)
>> -{
>> -    abort();
>> -}
> 
> Tested-by: Markus Armbruster <armbru@redhat.com>
> 
> I'll include this in my pull request, if you don't mind.

Yes, of course.

Paolo



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-16 10:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16  9:07 libqemuutil Markus Armbruster
2021-03-16  9:24 ` libqemuutil Thomas Huth
2021-03-16  9:41   ` libqemuutil Paolo Bonzini
2021-03-16  9:28 ` libqemuutil Paolo Bonzini
2021-03-16 10:09   ` libqemuutil Markus Armbruster
2021-03-16 10:15     ` libqemuutil Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).