Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

supplied .pc files link un-needed libraries and also don't link the dependencies of those libraries #1013

Open
ali1234 opened this issue Jun 28, 2018 · 12 comments

Comments

@ali1234
Copy link

ali1234 commented Jun 28, 2018

Ask pkg-config how to build a program for brcmegl:

PKG_CONFIG_LIBDIR=/opt/vc/lib/pkgconfig pkg-config --libs-only-l brcmegl

Result:

-lbrcmEGL -lbrcmGLESv2 -lbcm_host -lvchostif -lbcm_host -lvcos -lvchiq_arm

There are two problems with the above output. The first is that brcmEGL and brcmGLESv2 don't need to link bcm_host, vchostif, bcm_host, vcos, or vchiq_arm. The second problem is that if you do link vcoshostif then you must also link pthreads, and this .pc does not. As a result, any build using it will fail:

> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_notify_func: error: undefined reference to 'sem_wait'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_client_callback: error: undefined reference to 'sem_getvalue'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_client_callback: error: undefined reference to 'sem_post'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_notify_callback: error: undefined reference to 'sem_getvalue'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_notify_callback: error: undefined reference to 'sem_post'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_send_command: error: undefined reference to 'sem_wait'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function dispmanx_get_handle: error: undefined reference to 'sem_wait'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_vchi_dispmanx_init: error: undefined reference to 'sem_init'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_vchi_dispmanx_init: error: undefined reference to 'sem_init'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_vchi_dispmanx_init: error: undefined reference to 'sem_destroy'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_vchi_dispmanx_init: error: undefined reference to 'sem_destroy'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_dispmanx_stop: error: undefined reference to 'sem_getvalue'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_dispmanx_stop: error: undefined reference to 'sem_post'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_dispmanx_stop: error: undefined reference to 'sem_destroy'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_dispmanx_stop: error: undefined reference to 'sem_destroy'
> /home/al/Source/rpi-ramdisk/packages/sysroot/opt/vc/lib/libvchostif.a(vc_vchi_dispmanx.c.o):vc_vchi_dispmanx.c:function vc_dispmanx_query_image_formats: error: undefined reference to 'sem_wait'
> collect2: error: ld returned 1 exit status
> Makefile:67: recipe for target 'egl' failed
> make[2]: *** [egl] Error 1
> make[2]: Leaving directory '/home/al/Source/rpi-ramdisk/packages/qt/qtbase/config.tests/egl'
@ali1234 ali1234 changed the title supplied .pc files don't work properly supplied .pc files link un-needed libraries and also don't link the dependencies of those libraries Jun 28, 2018
ali1234 added a commit to ali1234/rpi-ramdisk that referenced this issue Jun 28, 2018
The workaround is to install fixed pkgconfig .pc files into the
sysroot. The ones installed from the official Raspbian package
have several problems.

First, they are installed somewhere that pkg-config doesn't know
about. The fixed .pc are install into $(SYSROOT)/usr/lib/pkgconfig
which is part of the list of directories Qt build searches.

Second, they are named like "brcmegl.pc" instead of egl.pc. The
fixed configs are installed as "egl.pc" etc, because this is what
Qt will look for. They still tell Qt to link to the brcm sonames.

Third, they link unnecessary libraries and then don't link to the
dependencies of those libraries, which means they don't work at
all. The fixed versions remove all reverences to bcm_host, vchi
and etc. This means they won't break the build due to no pthreads.

See raspberrypi/firmware#1013

pkg-config is only used during building, so these files are only
needed in the sysroot. The final binaries will "just work" without
hacking paths and environment variables.
@JamesH65
Copy link
Contributor

JamesH65 commented Jan 9, 2019

@XECDesign Is this one for you? Seems that the brcmegl.pc file has unnecessary dependencies, but I am no expert.

@XECDesign
Copy link
Contributor

IIRC, you have to call the init function from bcm_host before you can use them, so why should it be removed? Or is the idea that you should be calling pkg-config separately for bcm_host anyway?

@ali1234
Copy link
Author

ali1234 commented Jan 10, 2019

The problem was that configure tests check pkg-config --libs-only-l and then try to link but not run an executable with those libs. That brings in -lbcm_host due to the dependency which requires -pthread, but because that isn't a -l it isn't used when building the test executable. The test executable doesn't call bcm_host_init() because it isn't testing for that. It doesn't need to run, just link.

For some reason I can no longer reproduce the problem and I am not sure what changed. My toolchain seems to implicitly link pthreads now, meaning the tests work even without -pthread.

@ali1234
Copy link
Author

ali1234 commented Jan 10, 2019

Further investigation: the problem seems related to brcmegl.pc both requiring bcm_host and also explicitly linking bcm_host and vcoshostif.

brcmglesv2.pc and brcmvg.pc don't do this. I am not sure why.

@ali1234
Copy link
Author

ali1234 commented Jan 10, 2019

The problem is specifically caused by -lvcoshostif in brcmegl.pc. This is the library that needs pthreads. It does still cause Qt build failures when configure tests fail. The different behaviour I saw today happened because Qt refactored some code into a submodule which I didn't test (specifically qtdeclarative).

So I think the following should be done:

  1. brcmegl.pc shouldn't contain -lvcoshostif because it isn't needed.
  2. brcmegl.pc shouldn't contain -lbcm_host because Requires: bcm_host makes it redundant, and the gles and vg pcs don't have it.

@popcornmix
Copy link
Contributor

@ali1234 can you submit a PR?

@ali1234
Copy link
Author

ali1234 commented Jan 15, 2019

Which repository should it be made on? userland or firmware?

@popcornmix
Copy link
Contributor

Userland is best. It'll be cherry-picked to firmware tree on next firmware update.

@ali1234
Copy link
Author

ali1234 commented Jan 16, 2019

I just found out something new about this.

EGL does use vchostif functions, but because it is static, they all get bundled into bcm_host. However, other people build it as a shared lib, and then it needs to be linked separately. That is why patches like this exist: https://patchwork.openembedded.org/patch/140309/ - and see the comments, they are building it as a shared lib and they want to upstream it.

So vchostif can be removed from the pc in firmware as it is now. But if you make it into a shared library like some people want then you'll have to put it back. And for userland, the content of the pc should depend on how you build it.

I still don't understand why bcm_host doesn't bring a dependency on pthreads if it contains all the vchostif functions, or how building vchostif as a shared library will affect configure tests when the pcs are actually used. Nobody but me seems to use them.

ali1234 added a commit to ali1234/rpi-ramdisk that referenced this issue Jan 16, 2019
The previous workaround in de70457 does a bit more than it should
by removing the pkgconfig dependency links to bcm_host. This library
is actually required. Qt will add it without the help from pkgconfig
but keeping the dependency is still correct.

This means only brcmegl.pc needs to have its contents modified. It
now depends on bcm_host and also explicitly links it, which is
redundant. Linking vcoshostif is still unnecessary.

This patch restores the bcm_host dependencies and moves the patched
.pc files into /opt/vc next to the originals.

Unmodified .pcs are now symlinked instead of overlayed.

The new path for .pc files is added in the build script so that the
configs can be found.

See raspberrypi/firmware#1013
@ali1234
Copy link
Author

ali1234 commented Jan 16, 2019

For reference raspberrypi/userland#304 and #149

ali1234 added a commit to ali1234/userland that referenced this issue Jan 16, 2019
These pkgconfig files don't need to explicitly link bcm_host
because they contain "Requires: bcm_host" which will include
it and also any necessary dependencies.

They don't need to link vchostif because that static library
is included within bcm_host.so.

If vchostif is ever made shared the pkgconfigs will need to
be updated by adding -lvchostif back in.

Fixes: raspberrypi/firmware#1013
@ali1234
Copy link
Author

ali1234 commented Jan 17, 2019

Still investigating this. I found some new things.

pkg-config says the libs should be:

-lbrcmEGL -lbrcmGLESv2 -lbcm_host -lvchostif -lbcm_host -lvcos -lvchiq_arm

Notice that bcm_host appears twice. If you link the test executable with those libs it works. However, something in the Qt build is de-duplicating the list of libs so it tries to link with:

-lbrcmEGL -lbrcmGLESv2 -lvchostif -lbcm_host -lvcos -lvchiq_arm

This fails because now vchostif appears before bcm_host.

If you swap those two libs around in the command line, the build succeeds. I assume this happens because bcm_host supplies all the needed libs, so the compiler ends up ignoring vchostif.

I will keep investigating why Qt does that de-duplication. I think it should take the first appearance of a library rather than the last in order to not change the meaning when duplicates are present.

I also tested shared build with pcs that don't link vchostif (ie the above patch) and it still works, so what I said before about having to put it back is not true. I suppose this is because egl.so does not directly need anything from vchostif.so.

So at this point I'm fairly happy that my proposed fix is correct and I will open a pull request.

@ali1234
Copy link
Author

ali1234 commented Jan 17, 2019

Okay so I spoke too soon. While you dont need to link vchostif for egl, you do need to link it if it is shared and you want to call vc_dispmanx_display_open which egl programs generally do. In fact Qt has a separate test for this (egl-brcm) which fails when vchostif is shared and it isn't mentioned in any pc. In this case it should really be in a separate pc.

In fact all these pc files are over-linking things. EGL only needs brcmEGL.so at link time. You don't need to link bcm_host or brcmGLESv2 if they are dynamic.

Seems I need to think about this some more.

ali1234 added a commit to ali1234/userland that referenced this issue Jan 17, 2019
It isn't necessary to link bcm_host when linking EGL etc
unless you actually want to use a function from bcm_host
in your code. In that case you should be calling pkgconfig
for bcm_host separately. It also isn't necessary to link
GLESv2 when using EGL for the same reason.

These changes have been tested for building Qt.

I have not attempted to make the same fixes in MMAL etc
as I don't currently know how to test them.

Partially fixes: raspberrypi/firmware#1013
ali1234 added a commit to ali1234/userland that referenced this issue Jan 17, 2019
It isn't necessary to link bcm_host when linking EGL etc
unless you actually want to use a function from bcm_host
in your code. In that case you should be calling pkgconfig
for bcm_host separately. It also isn't necessary to link
GLESv2 when using EGL for the same reason.

These changes have been tested for building Qt.

I have not attempted to make the same fixes in MMAL etc
as I don't currently know how to test them.

Partially fixes: raspberrypi/firmware#1013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants