Regression in v7.2.10 - ui-dbus.so requires -fPIC

Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

ui-dbus.so is a shared library. But it is apparently handled differently
than all the other shared libraries: it is not compiled with -fPIC.

As a result it fails to link. Not sure why this happens only here.
Everything up to v7.2.9 was fine.

Looking at some random other library like libui-spice-core.a,
every object is compiled with -fPIC. 

But ui/dbus-display1.c is compiled with -fPIE instead.

Is this intentional? 

Olaf

ld: ui/libdbus-display1.a.p/meson-generated_.._dbus-display1.c.o: warning: relocation against `qemu_dbus_display1_audio_get_type' in read-only section `.text'

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Michael Tokarev]

15.03.2024 00:00, Olaf Hering wrote:
> ui-dbus.so is a shared library. But it is apparently handled differently
> than all the other shared libraries: it is not compiled with -fPIC.
> 
> As a result it fails to link. Not sure why this happens only here.
> Everything up to v7.2.9 was fine.
> 
> Looking at some random other library like libui-spice-core.a,
> every object is compiled with -fPIC.
> 
> But ui/dbus-display1.c is compiled with -fPIE instead.
> 
> Is this intentional?
> 
> Olaf
> 
> ld: ui/libdbus-display1.a.p/meson-generated_.._dbus-display1.c.o: warning: relocation against `qemu_dbus_display1_audio_get_type' in read-only section `.text'

Hi!

This seems to be the following patch which I picked up for 7.2.10:

commit c172136ea3320fa285c39bfb07298bbe1a14ba5e
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Thu Aug 11 15:59:40 2022 +0400

     meson: ensure dbus-display generated code is built before other units

     It's simply by luck that dbus-display header is built first before the
     other units using it.

     With sourceset, I can't find an easier way out than declaring an extra
     dependency for dbus-display1 generate code.

     Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
     Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
     (cherry picked from commit 1222070e772833c6875e0ca63565db12c22df39e)
     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

This has bitten me multiple times, when dbus-display1.c isn't generated
promptly.  So I thought it's a good pick.  Apparently not :)

And indeed, on master the same issue exists now, - after the above commit,
dbus-display1.o is being built with -fPIE instead of -fPIC. Reverting this
commit makes it use -fPIC again.

Apparently we don't build with --enable-modules and with a recent enough
compiler to catch this (my gcc-12.2 on debian bookworm does not care about
this stuff).

Cc'ing Marc-André.

An interesting side-effect :)

Thanks,

/mjt

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

Sat, 16 Mar 2024 22:40:14 +0300 Michael Tokarev <mjt@tls.msk.ru>:

>      meson: ensure dbus-display generated code is built before other units
>      (cherry picked from commit 1222070e772833c6875e0ca63565db12c22df39e)

"static_library" is used often. Some use the 'pic' option, which fixes the issue.

I think every usage that ends up in a shared library requires 'pic:true'.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Michael Tokarev]

17.03.2024 01:19, Olaf Hering:
> Sat, 16 Mar 2024 22:40:14 +0300 Michael Tokarev <mjt@tls.msk.ru>:
> 
>>       meson: ensure dbus-display generated code is built before other units
>>       (cherry picked from commit 1222070e772833c6875e0ca63565db12c22df39e)
> 
> "static_library" is used often. Some use the 'pic' option, which fixes the issue.
> 
> I think every usage that ends up in a shared library requires 'pic:true'.

The prob here seems to be that while fixing other issue (header file isn't generated
early enough), we all forgot about the fact dbus-display1.o can be used inside a
shared/loadable object when qemu is built with --enable-modules.

/mjt

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Marc-André Lureau]

Hi

On Sun, Mar 17, 2024 at 11:10 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 17.03.2024 01:19, Olaf Hering:
> > Sat, 16 Mar 2024 22:40:14 +0300 Michael Tokarev <mjt@tls.msk.ru>:
> >
> >>       meson: ensure dbus-display generated code is built before other units
> >>       (cherry picked from commit 1222070e772833c6875e0ca63565db12c22df39e)
> >
> > "static_library" is used often. Some use the 'pic' option, which fixes the issue.
> >
> > I think every usage that ends up in a shared library requires 'pic:true'.
>
> The prob here seems to be that while fixing other issue (header file isn't generated
> early enough), we all forgot about the fact dbus-display1.o can be used inside a
> shared/loadable object when qemu is built with --enable-modules.
>

dbus-display1 is also used with static linking for the unit test.

It looks like the simplest is to let the actual target decide how it
is built, even if it is compiled multiple time then:

-  dbus_display1_lib = static_library('dbus-display1', dbus_display1,
dependencies: gio)
-  dbus_display1_dep = declare_dependency(link_with:
dbus_display1_lib, sources: dbus_display1[0])
+  dbus_display1_dep = declare_dependency(sources: dbus_display1,
dependencies: gio)

This makes commit 186acfbaf7 ("tests/qtest: Depend on
dbus_display1_dep") no longer relevant, as dbus-display1.c will be
recompiled.

Alternatively, we could always build with pic: true (or pic:
enable_modules), but that's not ideal either.

-- 
Marc-André Lureau

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 294 bytes --]

Mon, 18 Mar 2024 11:35:54 +0400 Marc-André Lureau <marcandre.lureau@gmail.com>:

> Alternatively, we could always build with pic: true (or pic:
> enable_modules), but that's not ideal either.

I'm sure that unconditionally building with -fPIC has no downsides in this context.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression in v7.2.10 - ui-dbus.so requires -fPIC

[Michael Tokarev]

18.03.2024 10:35, Marc-André Lureau wrote:
..
> dbus-display1 is also used with static linking for the unit test.
> 
> It looks like the simplest is to let the actual target decide how it
> is built, even if it is compiled multiple time then:
> 
> -  dbus_display1_lib = static_library('dbus-display1', dbus_display1,
> dependencies: gio)
> -  dbus_display1_dep = declare_dependency(link_with:
> dbus_display1_lib, sources: dbus_display1[0])
> +  dbus_display1_dep = declare_dependency(sources: dbus_display1,
> dependencies: gio)
> 
> This makes commit 186acfbaf7 ("tests/qtest: Depend on
> dbus_display1_dep") no longer relevant, as dbus-display1.c will be
> recompiled.

I definitely prefer this approach.

> Alternatively, we could always build with pic: true (or pic:
> enable_modules), but that's not ideal either.

Here, we as well might enable pic unconditionally for just
everything.  But this is not for this change, and it needs
to be discussed first.  So far the consensus was to only
enable pic for shared objects.  Also, with --enable-static
it has other implications.

/mjt