| Message ID | 20171201205352.24287-14-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Per-package host/target directory support | expand |
Thomas, All, On 2017-12-01 21:53 +0100, Thomas Petazzoni spake thusly: > Currently, our strategy for the RPATH of host binaries is as follows: > > - During the build, we force an absolute RPATH to be encoded into > binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that > host binaries will find their libraries. > > - In the "make sdk" target, when preparing the SDK to be relocatable, > we use patchelf to replace those absolute RPATHs by relative RPATHs > that use $ORIGIN. > > Unfortunately, the use of absolute RPATH during the build is not > compatible with the move to per-package SDK, where a different host > directory is used for the build of each package. Therefore, we need to > move to a different strategy for RPATH handling. > > The new strategy is as follows: > > - We no longer pass -Wl,-rpath,$(HOST_DIR)/lib when building > packages, so the host binaries no longer have any rpath encoded. > > - At the end of the installation of every package, we just the > fix-rpath logic on the host binaries, so that all host binaries > that don't already have $ORIGIN/../lib as their RPATH are updated > to have such a RPATH. > > In order to achieve this, the following changes are made: > > - HOST_LDFLAGS no longer contains -Wl,-rpath,$(HOST_DIR) > > - The hook that ran at the end of every package installation > (check_host_rpath) to verify that our hardcoded RPATH is indeed > present in all binaries is removed, as it is no longer needed: we > will force $ORIGIN/../LIB as an RPATH in all binaries. Then you can just get rid of support/scripts/check-host-rpath that is now no longer used. > - host-patchelf is added as a dependency of all packages, except > itself. This is necessary as all packages now need to run patchelf > at the end of their installation process. > > TODO: things like host-tar, host-lzip and host-ccache will have to > be handled properly. Indeed, because the very moment that patchelf is available, the next package will trigger patchelf on all executables, which means that package will be responsible for touching files from previous packages, and thus will trigger the no-two-packages-should-touch-the-same-file check. > - "fix-rpath host" is called at the end of the package installation, > in the recently introduced .stamp_installed, which guarantees to be > executed after all of target, staging, images and host > installations have completed. Indeed, while in theory only host > packages install files into $(HOST_DIR), in practice a number of > target packages also install host binaries. So to be reliable, we > do the "fix-rpath host" logic at the end of the installation of > every package. > > - The fix-rpath script is modified: > > * The "host" target now simply hardcodes setting $ORIGIN/../lib as > an RPATH, instead of using --relative-to-file > > * We simply exclude the patchelf program instead of making a copy > of it: since patchelf depends only on the C/C++ libraries, it > doesn't need to have a RPATH set to $ORIGIN/../lib > > * Also, in order to avoid re-writing host binaries over and over > again, we only set the RPATH if it is not already set. > > This change is independent from the per-package SDK functionality, and > could be applied separately. Except it requires the host-tar et al. stuff to be handled first. Regards, Yann E. MORIN. > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > NOTE: an alternative to running "fix-rpath host" at the end of every > package installation would be to run it only for host packages > (unconditionally) and have a special flag that target packages can use > to indicate that they have installed things to $(HOST_DIR). Not sure > if this is worth the complexity, though. > > NOTE2: is it OK to force $ORIGIN/../lib as the RPATH of all host > binaries? This is OK for binaries in host/{bin,sbin} of course, but do > we have host binaries in other places, for which this $ORIGIN/../lib > RPATH wouldn't be appropriate? > > Changes since v2: > - No changes > > Changes since v1: > - Following the feedback from Yann and Arnout, the approach in v2 was > completely changed: instead of using LD_LIBRARY_PATH, we now run > patchelf at the end of each package installation to fix the RPATH. > --- > Makefile | 1 - > package/Makefile.in | 2 +- > package/pkg-generic.mk | 14 ++++++-------- > support/scripts/fix-rpath | 31 +++++++++++++++++++------------ > 4 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/Makefile b/Makefile > index 090b3ba191..d8fa91120b 100644 > --- a/Makefile > +++ b/Makefile > @@ -555,7 +555,6 @@ world: target-post-image > .PHONY: sdk > sdk: world > @$(call MESSAGE,"Rendering the SDK relocatable") > - $(TOPDIR)/support/scripts/fix-rpath host > $(TOPDIR)/support/scripts/fix-rpath staging > $(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh > echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location > diff --git a/package/Makefile.in b/package/Makefile.in > index a1a5316051..e94a75c230 100644 > --- a/package/Makefile.in > +++ b/package/Makefile.in > @@ -220,7 +220,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include > HOST_CFLAGS ?= -O2 > HOST_CFLAGS += $(HOST_CPPFLAGS) > HOST_CXXFLAGS += $(HOST_CFLAGS) > -HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib > +HOST_LDFLAGS += -L$(HOST_DIR)/lib > > # The macros below are taken from linux 4.11 and adapted slightly. > # Copy more when needed. > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 8c6e34cc9f..f4a943e3ce 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -118,14 +118,6 @@ endef > > GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch > > -# This hook checks that host packages that need libraries that we build > -# have a proper DT_RPATH or DT_RUNPATH tag > -define check_host_rpath > - $(if $(filter install-host,$(2)),\ > - $(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR))) > -endef > -GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath > - > define step_check_build_dir_one > if [ -d $(2) ]; then \ > printf "%s: installs files in %s\n" $(1) $(2) >&2; \ > @@ -343,6 +335,8 @@ $(BUILD_DIR)/%/.stamp_target_installed: > # Finalize installation > $(BUILD_DIR)/%/.stamp_installed: > @$(call step_start,install) > + $(if $(filter host-patchelf,$($(PKG)_FINAL_DEPENDENCIES)), \ > + $(TOPDIR)/support/scripts/fix-rpath host) > @$(call step_end,install) > $(Q)touch $@ > > @@ -587,6 +581,10 @@ ifneq ($(1),host-skeleton) > $(2)_DEPENDENCIES += host-skeleton > endif > > +ifeq ($(filter host-patchelf host-skeleton host-tar host-xz host-lzip host-ccache host-fakedate,$(1)),) > +$(2)_DEPENDENCIES += host-patchelf > +endif > + > ifeq ($(filter host-tar host-skeleton host-fakedate,$(1)),) > $(2)_EXTRACT_DEPENDENCIES += $(BR2_TAR_HOST_DEPENDENCY) > endif > diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath > index 15705a3b0d..5b2f24ed32 100755 > --- a/support/scripts/fix-rpath > +++ b/support/scripts/fix-rpath > @@ -61,7 +61,7 @@ main() { > local rootdir > local tree="${1}" > local find_args=( ) > - local sanitize_extra_args=( ) > + local sanitize_args=( ) > > if ! "${PATCHELF}" --version > /dev/null 2>&1; then > echo "Error: can't execute patchelf utility '${PATCHELF}'" > @@ -84,12 +84,14 @@ main() { > find_args+=( "-path" "${HOST_DIR}""${excludepath}" "-prune" "-o" ) > done > > - # do not process the patchelf binary but a copy to work-around "file in use" > + # do not process the patchelf binary, as it is ourselves > + # (and it doesn't need a rpath as it doesn't use libraries > + # from HOST_DIR) > find_args+=( "-path" "${PATCHELF}" "-prune" "-o" ) > - cp "${PATCHELF}" "${PATCHELF}.__to_be_patched" > > # we always want $ORIGIN-based rpaths to make it relocatable. > - sanitize_extra_args+=( "--relative-to-file" ) > + sanitize_args+=( "--set-rpath" ) > + sanitize_args+=( "\$ORIGIN/../lib" ) > ;; > > staging) > @@ -101,14 +103,18 @@ main() { > done > > # should be like for the target tree below > - sanitize_extra_args+=( "--no-standard-lib-dirs" ) > + sanitize_args+=( "--no-standard-lib-dirs" ) > + sanitize_args+=( "--make-rpath-relative" ) > + sanitize_args+=( "${rootdir}" ) > ;; > > target) > rootdir="${TARGET_DIR}" > # we don't want $ORIGIN-based rpaths but absolute paths without rootdir. > # we also want to remove rpaths pointing to /lib or /usr/lib. > - sanitize_extra_args+=( "--no-standard-lib-dirs" ) > + sanitize_args+=( "--no-standard-lib-dirs" ) > + sanitize_args+=( "--make-rpath-relative" ) > + sanitize_args+=( "${rootdir}" ) > ;; > > *) > @@ -120,20 +126,21 @@ main() { > find_args+=( "-type" "f" "-print" ) > > while read file ; do > - # check if it's an ELF file > - if ${PATCHELF} --print-rpath "${file}" > /dev/null 2>&1; then > + rpath=$(${PATCHELF} --print-rpath "${file}" 2>/dev/null) > + if test $? -eq 0; then > + # For host binaries, if the rpath is already correct, skip > + if test "${tree}" = "host" -a "${rpath}" = "\$ORIGIN/../lib" ; then > + continue > + fi > # make files writable if necessary > changed=$(chmod -c u+w "${file}") > # call patchelf to sanitize the rpath > - ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}" > + ${PATCHELF} ${sanitize_args[@]} "${file}" > # restore the original permission > test "${changed}" != "" && chmod u-w "${file}" > fi > done < <(find "${rootdir}" ${find_args[@]}) > > - # Restore patched patchelf utility > - test "${tree}" = "host" && mv "${PATCHELF}.__to_be_patched" "${PATCHELF}" > - > # ignore errors > return 0 > } > -- > 2.13.6 >
Hello, On Fri, 29 Dec 2017 18:31:18 +0100, Yann E. MORIN wrote: > > In order to achieve this, the following changes are made: > > > > - HOST_LDFLAGS no longer contains -Wl,-rpath,$(HOST_DIR) > > > > - The hook that ran at the end of every package installation > > (check_host_rpath) to verify that our hardcoded RPATH is indeed > > present in all binaries is removed, as it is no longer needed: we > > will force $ORIGIN/../LIB as an RPATH in all binaries. > > Then you can just get rid of support/scripts/check-host-rpath that is > now no longer used. True. > > > - host-patchelf is added as a dependency of all packages, except > > itself. This is necessary as all packages now need to run patchelf > > at the end of their installation process. > > > > TODO: things like host-tar, host-lzip and host-ccache will have to > > be handled properly. In fact, this TODO was no longer accurate: I did take care of host-tar, host-lzip and host-ccache as part of this RFCv3. However... > Indeed, because the very moment that patchelf is available, the next > package will trigger patchelf on all executables, which means that > package will be responsible for touching files from previous packages, > and thus will trigger the no-two-packages-should-touch-the-same-file > check. ... I indeed didn't think of this problem. I see two possibilities to address this: 1. Have host-patchelf built earlier. I.e have only host-tar be a dependency of host-patchelf and nothing else (in my current patch series, host-patchelf is built after host-tar, host-xz, host-lzip, host-ccache). This requires using HOSTCC_NOCCACHE to build host-tar and host-patchelf, but I don't think it's a big deal. And then do a special case in fix-rpath to not fix the rpath of the tar program, since it should not have dependencies on libraries in HOST_DIR/lib 2. Make the RPATH fixing logic smarter, and don't set a RPATH to $ORIGIN/../lib if the program doesn't use a library that is in HOST_DIR/lib. This is perhaps the most pretty solution. > > This change is independent from the per-package SDK functionality, and > > could be applied separately. > > Except it requires the host-tar et al. stuff to be handled first. Which is why the host-tar and al. stuff is earlier. What I meant with this sentence is that reworking the host RPATH handling can be applied *before* we switch to per-package SDK. Best regards, Thomas
On 2017-12-29 21:20 +0100, Thomas Petazzoni spake thusly: > Hello, > > On Fri, 29 Dec 2017 18:31:18 +0100, Yann E. MORIN wrote: > > > > In order to achieve this, the following changes are made: > > > > > > - HOST_LDFLAGS no longer contains -Wl,-rpath,$(HOST_DIR) > > > > > > - The hook that ran at the end of every package installation > > > (check_host_rpath) to verify that our hardcoded RPATH is indeed > > > present in all binaries is removed, as it is no longer needed: we > > > will force $ORIGIN/../LIB as an RPATH in all binaries. > > > > Then you can just get rid of support/scripts/check-host-rpath that is > > now no longer used. > > True. > > > > > > - host-patchelf is added as a dependency of all packages, except > > > itself. This is necessary as all packages now need to run patchelf > > > at the end of their installation process. > > > > > > TODO: things like host-tar, host-lzip and host-ccache will have to > > > be handled properly. > > In fact, this TODO was no longer accurate: I did take care of host-tar, > host-lzip and host-ccache as part of this RFCv3. However... You said on IRC that the uniq-file check did not trigger for host-tar, and I think I know why: we're looking at installed files in the step hook named 'step_pkg_size' which gets run as part of the 'install-host' step. But the patchelf will only happen in the new step named 'install', which you introduced in patch 05/15 (pkg-generic: add .stamp_installed step), and which is called after 'install-host' (guaranteed, because 'install' depends on 'install-host'). So, the uniq-file check will not get triggered for host-patchelf. It will not even be trigger by a later package either, because the hashes are taken before and after the install step, not from a previous install step. So, we are lucky. However, I don't think this is nice... > > Indeed, because the very moment that patchelf is available, the next > > package will trigger patchelf on all executables, which means that > > package will be responsible for touching files from previous packages, > > and thus will trigger the no-two-packages-should-touch-the-same-file > > check. > > ... I indeed didn't think of this problem. I see two possibilities to > address this: > > 1. Have host-patchelf built earlier. I.e have only host-tar be a > dependency of host-patchelf and nothing else (in my current patch > series, host-patchelf is built after host-tar, host-xz, host-lzip, > host-ccache). This requires using HOSTCC_NOCCACHE to build host-tar > and host-patchelf, but I don't think it's a big deal. And then do a > special case in fix-rpath to not fix the rpath of the tar program, > since it should not have dependencies on libraries in HOST_DIR/lib > > 2. Make the RPATH fixing logic smarter, and don't set a RPATH to > $ORIGIN/../lib if the program doesn't use a library that is in > HOST_DIR/lib. This is perhaps the most pretty solution. 3. limit patchelf on the files actually installed by the current package, looking at $(BUILD_DIR)/packages-file-list-host.txt to get the list... Actually, this might prompt for storing a per-package list of installed file, something like (untested): diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index b4aa22e38c..4a086600dd 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -89,8 +89,11 @@ define step_pkg_size_end $(call _step_pkg_size_get_file_list,$($(PKG)_DIR)/.br_filelist_after,$(2)) comm -13 $($(PKG)_DIR)/.br_filelist_before $($(PKG)_DIR)/.br_filelist_after | \ while read hash file ; do \ - echo "$(1),$${file}" ; \ - done >> $(BUILD_DIR)/packages-file-list$(3).txt + echo "$${file}" ; \ + done >> $($(PKG)_DIR)/.br_filelist.txt + sed -r -e 's/^/$(1),/' \ + < $($(PKG)_DIR)/.br_filelist.txt \ + >> $(BUILD_DIR)/packages-file-list$(3).txt rm -f $($(PKG)_DIR)/.br_filelist_before $($(PKG)_DIR)/.br_filelist_after endef ... and then re-using $($(PKG)_DIR)/.br_filelist.txt as the list of files installed by the current package, and only patchelf thos liste in there. My 2ct... Regards, Yann E. MORIN. > > > This change is independent from the per-package SDK functionality, and > > > could be applied separately. > > > > Except it requires the host-tar et al. stuff to be handled first. > > Which is why the host-tar and al. stuff is earlier. What I meant with > this sentence is that reworking the host RPATH handling can be applied > *before* we switch to per-package SDK. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Thomas, Yann, On Fri, Dec 29, 2017 at 9:20 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Fri, 29 Dec 2017 18:31:18 +0100, Yann E. MORIN wrote: > [snip] > 2. Make the RPATH fixing logic smarter, and don't set a RPATH to > $ORIGIN/../lib if the program doesn't use a library that is in > HOST_DIR/lib. This is perhaps the most pretty solution. > What about the case of autotools tests which now don't have an absolute path to the library? I ran into an issue with this and the host-libglibc2 testing for PCRE unicode. The test worked fine once I set the LD LIBRARY PATH. Matt
Thomas, On Fri, Dec 1, 2017 at 9:53 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: [snip] > --- a/package/Makefile.in > +++ b/package/Makefile.in > @@ -220,7 +220,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include > HOST_CFLAGS ?= -O2 > HOST_CFLAGS += $(HOST_CPPFLAGS) > HOST_CXXFLAGS += $(HOST_CFLAGS) > -HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib > +HOST_LDFLAGS += -L$(HOST_DIR)/lib Reproduced with the following config BR2_aarch64=y BR2_TOOLCHAIN_EXTERNAL=y BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0" BR2_SYSTEM_DHCP="eth0" BR2_LINUX_KERNEL=y BR2_LINUX_KERNEL_CUSTOM_VERSION=y BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.13.6" BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/qemu/aarch64-virt/linux-4.13.config" BR2_PACKAGE_CHECKPOLICY=y BR2_PACKAGE_POLICYCOREUTILS=y BR2_PACKAGE_SETOOLS=y BR2_PACKAGE_HOST_CHECKPOLICY=y That build will first fail at host-libglib2 (mentioned in previous email on this thread) because the autotools test of pcre unicode will fail as it can't find the libraries. See patch https://patchwork.ozlabs.org/patch/870130/ Next it will fail at setools because of a python header include now only pointing at the python host/include vs the full host/include. It seems setuptools by default inherits an include path to python headers install location. See patch https://patchwork.ozlabs.org/patch/870131/. Last it looks like the rpath still needs to be set so that the linker can evaluate the shared library's shared library dependency. I tested the changes below and it builds, plus doesn't leave an rpath in the executable (Thanks Roman). <<<<<<<<< CUT --- a/package/Makefile.in +++ b/package/Makefile.in @@ -238,7 +238,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include HOST_CFLAGS ?= -O2 HOST_CFLAGS += $(HOST_CPPFLAGS) HOST_CXXFLAGS += $(HOST_CFLAGS) -HOST_LDFLAGS += -L$(HOST_DIR)/lib +HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath-link,$(HOST_DIR)/lib # The macros below are taken from linux 4.11 and adapted slightly. # Copy more when needed. <<<<<<<<< CUT The use of LD_LIBRARY_PATH set to the same value also works. Here's my understanding as pulled from the ld man page. When using ELF or SunOS, one shared library may require another. This happens when an ld -shared link includes a shared library as one of the input files. When the linker encounters such a dependency when doing a non-shared, non-relocateable link, it will automatically try to locate the required shared library and include it in the link, if it is not included explicitly. In such a case, the -rpath-link option specifies the first set of directories to search. The -rpath-link option may specify a sequence of directory names either by specifying a list of names separated by colons, or by appearing multiple times. The linker uses the following search paths to locate required shared libraries. 1) Any directories specified by -rpath-link options. 2) Any directories specified by -rpath options. The difference between -rpath and -rpath-link is that directories specified by -rpath options are included in the executable and used at runtime, whereas the -rpath-link option is only effective at link time. 3) On an ELF system, if the -rpath and rpath-link options were not used, search the contents of the environment variable LD_RUN_PATH. 4) On SunOS, if the -rpath option was not used, search any directories specified using -L options. 5) For a native linker, the contents of the environment variable LD_LIBRARY_PATH. 6) The default directories, normally `/lib' and `/usr/lib'. Matt
On 06-02-18 23:04, Matthew Weber wrote: > Thomas, > > On Fri, Dec 1, 2017 at 9:53 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > [snip] >> --- a/package/Makefile.in >> +++ b/package/Makefile.in >> @@ -220,7 +220,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include >> HOST_CFLAGS ?= -O2 >> HOST_CFLAGS += $(HOST_CPPFLAGS) >> HOST_CXXFLAGS += $(HOST_CFLAGS) >> -HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib >> +HOST_LDFLAGS += -L$(HOST_DIR)/lib > > Reproduced with the following config > BR2_aarch64=y > BR2_TOOLCHAIN_EXTERNAL=y > BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0" > BR2_SYSTEM_DHCP="eth0" > BR2_LINUX_KERNEL=y > BR2_LINUX_KERNEL_CUSTOM_VERSION=y > BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.13.6" > BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y > BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/qemu/aarch64-virt/linux-4.13.config" > BR2_PACKAGE_CHECKPOLICY=y > BR2_PACKAGE_POLICYCOREUTILS=y > BR2_PACKAGE_SETOOLS=y > BR2_PACKAGE_HOST_CHECKPOLICY=y > > That build will first fail at host-libglib2 (mentioned in previous > email on this thread) because the autotools test of pcre unicode will > fail as it can't find the libraries. See patch > https://patchwork.ozlabs.org/patch/870130/ IIUC this patch should no longer be needed if we do -Wl,-rpath-link instead, right? > Next it will fail at setools because of a python header include now > only pointing at the python host/include vs the full host/include. It > seems setuptools by default inherits an include path to python headers > install location. See patch > https://patchwork.ozlabs.org/patch/870131/. If setuptools completely ignores the CFLAGS we pass to it, then it should be fixed in setuptools, not in a per-package patch... > Last it looks like the rpath still needs to be set so that the linker > can evaluate the shared library's shared library dependency. I tested > the changes below and it builds, plus doesn't leave an rpath in the > executable (Thanks Roman). > <<<<<<<<< CUT > --- a/package/Makefile.in > +++ b/package/Makefile.in > @@ -238,7 +238,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include > HOST_CFLAGS ?= -O2 > HOST_CFLAGS += $(HOST_CPPFLAGS) > HOST_CXXFLAGS += $(HOST_CFLAGS) > -HOST_LDFLAGS += -L$(HOST_DIR)/lib > +HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath-link,$(HOST_DIR)/lib > > # The macros below are taken from linux 4.11 and adapted slightly. > # Copy more when needed. > <<<<<<<<< CUT > The use of LD_LIBRARY_PATH set to the same value also works. Here's > my understanding as pulled from the ld man page. > > When using ELF or SunOS, one shared library may require another. This > happens when an ld -shared link includes a shared library as one of > the input files. When the linker encounters such a dependency when > doing a non-shared, non-relocateable link, it will automatically try > to locate the required shared library and include it in the link, if > it is not included explicitly. In such a case, the -rpath-link option > specifies the first set of directories to search. The -rpath-link > option may specify a sequence of directory names either by specifying > a list of names separated by colons, or by appearing multiple times. > The linker uses the following search paths to locate required shared > libraries. > 1) Any directories specified by -rpath-link options. > 2) Any directories specified by -rpath options. The difference between > -rpath and -rpath-link is that directories specified by -rpath options > are included in the executable and used at runtime, whereas the > -rpath-link option is only effective at link time. > 3) On an ELF system, if the -rpath and rpath-link options were not > used, search the contents of the environment variable LD_RUN_PATH. > 4) On SunOS, if the -rpath option was not used, search any directories > specified using -L options. > 5) For a native linker, the contents of the environment variable > LD_LIBRARY_PATH. > 6) The default directories, normally `/lib' and `/usr/lib'. I have read this section of the manpage several times over the years, and now is the first time it makes sense to me :-) So indeed, for NEEDED libraries within libraries that you link with, ld will not look at -L but it will only look at -rpath-link and -rpath (the rest is not relevant in our context). So if we remove -rpath, it does make sense that we pass -rpath-link instead (we already pass -L as well). That said, I still don't understand why it didn't work without -rpath-link. Indeed, there *is* a DT_RUNPATH set in the library (at least according to the commit log of this patch, since fix-rpath is run at the end of installation), but it's set to $ORIGIN/../lib instead of to an absolute path. So, does ld not look at DT_RUNPATH? Or does it evaluate $ORIGIN in an unexpected way? Finally, note that we said in the Buildroot meeting that this patch is in fact probably NOT needed for per-package SDK, because the NEEDed libraries are in fact installed in the location specified by the absolute RPATH. It's just weird to have an RPATH referring outside of the per-package staging, but it's not an actual problem. Regards, Arnout
Arnout, On Wed, Feb 7, 2018 at 12:19 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > > > On 06-02-18 23:04, Matthew Weber wrote: >> Thomas, >> >> On Fri, Dec 1, 2017 at 9:53 PM, Thomas Petazzoni >> <thomas.petazzoni@free-electrons.com> wrote: >> [snip] >>> --- a/package/Makefile.in >>> +++ b/package/Makefile.in >>> @@ -220,7 +220,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include >>> HOST_CFLAGS ?= -O2 >>> HOST_CFLAGS += $(HOST_CPPFLAGS) >>> HOST_CXXFLAGS += $(HOST_CFLAGS) >>> -HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib >>> +HOST_LDFLAGS += -L$(HOST_DIR)/lib >> >> Reproduced with the following config >> BR2_aarch64=y >> BR2_TOOLCHAIN_EXTERNAL=y >> BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0" >> BR2_SYSTEM_DHCP="eth0" >> BR2_LINUX_KERNEL=y >> BR2_LINUX_KERNEL_CUSTOM_VERSION=y >> BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.13.6" >> BR2_LINUX_KERNEL_USE_CUSTOM_CONFIG=y >> BR2_LINUX_KERNEL_CUSTOM_CONFIG_FILE="board/qemu/aarch64-virt/linux-4.13.config" >> BR2_PACKAGE_CHECKPOLICY=y >> BR2_PACKAGE_POLICYCOREUTILS=y >> BR2_PACKAGE_SETOOLS=y >> BR2_PACKAGE_HOST_CHECKPOLICY=y >> >> That build will first fail at host-libglib2 (mentioned in previous >> email on this thread) because the autotools test of pcre unicode will >> fail as it can't find the libraries. See patch >> https://patchwork.ozlabs.org/patch/870130/ > > IIUC this patch should no longer be needed if we do -Wl,-rpath-link instead, right? > > >> Next it will fail at setools because of a python header include now >> only pointing at the python host/include vs the full host/include. It >> seems setuptools by default inherits an include path to python headers >> install location. See patch >> https://patchwork.ozlabs.org/patch/870131/. > > If setuptools completely ignores the CFLAGS we pass to it, then it should be > fixed in setuptools, not in a per-package patch... > > >> Last it looks like the rpath still needs to be set so that the linker >> can evaluate the shared library's shared library dependency. I tested >> the changes below and it builds, plus doesn't leave an rpath in the >> executable (Thanks Roman). >> <<<<<<<<< CUT >> --- a/package/Makefile.in >> +++ b/package/Makefile.in >> @@ -238,7 +238,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include >> HOST_CFLAGS ?= -O2 >> HOST_CFLAGS += $(HOST_CPPFLAGS) >> HOST_CXXFLAGS += $(HOST_CFLAGS) >> -HOST_LDFLAGS += -L$(HOST_DIR)/lib >> +HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath-link,$(HOST_DIR)/lib >> >> # The macros below are taken from linux 4.11 and adapted slightly. >> # Copy more when needed. >> <<<<<<<<< CUT >> The use of LD_LIBRARY_PATH set to the same value also works. Here's >> my understanding as pulled from the ld man page. >> >> When using ELF or SunOS, one shared library may require another. This >> happens when an ld -shared link includes a shared library as one of >> the input files. When the linker encounters such a dependency when >> doing a non-shared, non-relocateable link, it will automatically try >> to locate the required shared library and include it in the link, if >> it is not included explicitly. In such a case, the -rpath-link option >> specifies the first set of directories to search. The -rpath-link >> option may specify a sequence of directory names either by specifying >> a list of names separated by colons, or by appearing multiple times. >> The linker uses the following search paths to locate required shared >> libraries. >> 1) Any directories specified by -rpath-link options. >> 2) Any directories specified by -rpath options. The difference between >> -rpath and -rpath-link is that directories specified by -rpath options >> are included in the executable and used at runtime, whereas the >> -rpath-link option is only effective at link time. >> 3) On an ELF system, if the -rpath and rpath-link options were not >> used, search the contents of the environment variable LD_RUN_PATH. >> 4) On SunOS, if the -rpath option was not used, search any directories >> specified using -L options. >> 5) For a native linker, the contents of the environment variable >> LD_LIBRARY_PATH. >> 6) The default directories, normally `/lib' and `/usr/lib'. > > I have read this section of the manpage several times over the years, and now > is the first time it makes sense to me :-) > > So indeed, for NEEDED libraries within libraries that you link with, ld will > not look at -L but it will only look at -rpath-link and -rpath (the rest is not > relevant in our context). So if we remove -rpath, it does make sense that we > pass -rpath-link instead (we already pass -L as well). > > That said, I still don't understand why it didn't work without -rpath-link. > Indeed, there *is* a DT_RUNPATH set in the library (at least according to the > commit log of this patch, since fix-rpath is run at the end of installation), > but it's set to $ORIGIN/../lib instead of to an absolute path. So, does ld not > look at DT_RUNPATH? Or does it evaluate $ORIGIN in an unexpected way? > It seems $ORIGIN is being evaluated against default lib search dirs (doesn't include -L paths) so it's going to need the additional base path from -rpath-link or LD_LIBRARY_PATH (either seemed to behave the same). > > Finally, note that we said in the Buildroot meeting that this patch is in fact > probably NOT needed for per-package SDK, because the NEEDed libraries are in > fact installed in the location specified by the absolute RPATH. It's just weird > to have an RPATH referring outside of the per-package staging, but it's not an > actual problem. Ah now that I reread the initial patch, I see that note at the bottom about optional. True. Matt
diff --git a/Makefile b/Makefile index 090b3ba191..d8fa91120b 100644 --- a/Makefile +++ b/Makefile @@ -555,7 +555,6 @@ world: target-post-image .PHONY: sdk sdk: world @$(call MESSAGE,"Rendering the SDK relocatable") - $(TOPDIR)/support/scripts/fix-rpath host $(TOPDIR)/support/scripts/fix-rpath staging $(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location diff --git a/package/Makefile.in b/package/Makefile.in index a1a5316051..e94a75c230 100644 --- a/package/Makefile.in +++ b/package/Makefile.in @@ -220,7 +220,7 @@ HOST_CPPFLAGS = -I$(HOST_DIR)/include HOST_CFLAGS ?= -O2 HOST_CFLAGS += $(HOST_CPPFLAGS) HOST_CXXFLAGS += $(HOST_CFLAGS) -HOST_LDFLAGS += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib +HOST_LDFLAGS += -L$(HOST_DIR)/lib # The macros below are taken from linux 4.11 and adapted slightly. # Copy more when needed. diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 8c6e34cc9f..f4a943e3ce 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -118,14 +118,6 @@ endef GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch -# This hook checks that host packages that need libraries that we build -# have a proper DT_RPATH or DT_RUNPATH tag -define check_host_rpath - $(if $(filter install-host,$(2)),\ - $(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR))) -endef -GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath - define step_check_build_dir_one if [ -d $(2) ]; then \ printf "%s: installs files in %s\n" $(1) $(2) >&2; \ @@ -343,6 +335,8 @@ $(BUILD_DIR)/%/.stamp_target_installed: # Finalize installation $(BUILD_DIR)/%/.stamp_installed: @$(call step_start,install) + $(if $(filter host-patchelf,$($(PKG)_FINAL_DEPENDENCIES)), \ + $(TOPDIR)/support/scripts/fix-rpath host) @$(call step_end,install) $(Q)touch $@ @@ -587,6 +581,10 @@ ifneq ($(1),host-skeleton) $(2)_DEPENDENCIES += host-skeleton endif +ifeq ($(filter host-patchelf host-skeleton host-tar host-xz host-lzip host-ccache host-fakedate,$(1)),) +$(2)_DEPENDENCIES += host-patchelf +endif + ifeq ($(filter host-tar host-skeleton host-fakedate,$(1)),) $(2)_EXTRACT_DEPENDENCIES += $(BR2_TAR_HOST_DEPENDENCY) endif diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath index 15705a3b0d..5b2f24ed32 100755 --- a/support/scripts/fix-rpath +++ b/support/scripts/fix-rpath @@ -61,7 +61,7 @@ main() { local rootdir local tree="${1}" local find_args=( ) - local sanitize_extra_args=( ) + local sanitize_args=( ) if ! "${PATCHELF}" --version > /dev/null 2>&1; then echo "Error: can't execute patchelf utility '${PATCHELF}'" @@ -84,12 +84,14 @@ main() { find_args+=( "-path" "${HOST_DIR}""${excludepath}" "-prune" "-o" ) done - # do not process the patchelf binary but a copy to work-around "file in use" + # do not process the patchelf binary, as it is ourselves + # (and it doesn't need a rpath as it doesn't use libraries + # from HOST_DIR) find_args+=( "-path" "${PATCHELF}" "-prune" "-o" ) - cp "${PATCHELF}" "${PATCHELF}.__to_be_patched" # we always want $ORIGIN-based rpaths to make it relocatable. - sanitize_extra_args+=( "--relative-to-file" ) + sanitize_args+=( "--set-rpath" ) + sanitize_args+=( "\$ORIGIN/../lib" ) ;; staging) @@ -101,14 +103,18 @@ main() { done # should be like for the target tree below - sanitize_extra_args+=( "--no-standard-lib-dirs" ) + sanitize_args+=( "--no-standard-lib-dirs" ) + sanitize_args+=( "--make-rpath-relative" ) + sanitize_args+=( "${rootdir}" ) ;; target) rootdir="${TARGET_DIR}" # we don't want $ORIGIN-based rpaths but absolute paths without rootdir. # we also want to remove rpaths pointing to /lib or /usr/lib. - sanitize_extra_args+=( "--no-standard-lib-dirs" ) + sanitize_args+=( "--no-standard-lib-dirs" ) + sanitize_args+=( "--make-rpath-relative" ) + sanitize_args+=( "${rootdir}" ) ;; *) @@ -120,20 +126,21 @@ main() { find_args+=( "-type" "f" "-print" ) while read file ; do - # check if it's an ELF file - if ${PATCHELF} --print-rpath "${file}" > /dev/null 2>&1; then + rpath=$(${PATCHELF} --print-rpath "${file}" 2>/dev/null) + if test $? -eq 0; then + # For host binaries, if the rpath is already correct, skip + if test "${tree}" = "host" -a "${rpath}" = "\$ORIGIN/../lib" ; then + continue + fi # make files writable if necessary changed=$(chmod -c u+w "${file}") # call patchelf to sanitize the rpath - ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}" + ${PATCHELF} ${sanitize_args[@]} "${file}" # restore the original permission test "${changed}" != "" && chmod u-w "${file}" fi done < <(find "${rootdir}" ${find_args[@]}) - # Restore patched patchelf utility - test "${tree}" = "host" && mv "${PATCHELF}.__to_be_patched" "${PATCHELF}" - # ignore errors return 0 }
Currently, our strategy for the RPATH of host binaries is as follows: - During the build, we force an absolute RPATH to be encoded into binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that host binaries will find their libraries. - In the "make sdk" target, when preparing the SDK to be relocatable, we use patchelf to replace those absolute RPATHs by relative RPATHs that use $ORIGIN. Unfortunately, the use of absolute RPATH during the build is not compatible with the move to per-package SDK, where a different host directory is used for the build of each package. Therefore, we need to move to a different strategy for RPATH handling. The new strategy is as follows: - We no longer pass -Wl,-rpath,$(HOST_DIR)/lib when building packages, so the host binaries no longer have any rpath encoded. - At the end of the installation of every package, we just the fix-rpath logic on the host binaries, so that all host binaries that don't already have $ORIGIN/../lib as their RPATH are updated to have such a RPATH. In order to achieve this, the following changes are made: - HOST_LDFLAGS no longer contains -Wl,-rpath,$(HOST_DIR) - The hook that ran at the end of every package installation (check_host_rpath) to verify that our hardcoded RPATH is indeed present in all binaries is removed, as it is no longer needed: we will force $ORIGIN/../LIB as an RPATH in all binaries. - host-patchelf is added as a dependency of all packages, except itself. This is necessary as all packages now need to run patchelf at the end of their installation process. TODO: things like host-tar, host-lzip and host-ccache will have to be handled properly. - "fix-rpath host" is called at the end of the package installation, in the recently introduced .stamp_installed, which guarantees to be executed after all of target, staging, images and host installations have completed. Indeed, while in theory only host packages install files into $(HOST_DIR), in practice a number of target packages also install host binaries. So to be reliable, we do the "fix-rpath host" logic at the end of the installation of every package. - The fix-rpath script is modified: * The "host" target now simply hardcodes setting $ORIGIN/../lib as an RPATH, instead of using --relative-to-file * We simply exclude the patchelf program instead of making a copy of it: since patchelf depends only on the C/C++ libraries, it doesn't need to have a RPATH set to $ORIGIN/../lib * Also, in order to avoid re-writing host binaries over and over again, we only set the RPATH if it is not already set. This change is independent from the per-package SDK functionality, and could be applied separately. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- NOTE: an alternative to running "fix-rpath host" at the end of every package installation would be to run it only for host packages (unconditionally) and have a special flag that target packages can use to indicate that they have installed things to $(HOST_DIR). Not sure if this is worth the complexity, though. NOTE2: is it OK to force $ORIGIN/../lib as the RPATH of all host binaries? This is OK for binaries in host/{bin,sbin} of course, but do we have host binaries in other places, for which this $ORIGIN/../lib RPATH wouldn't be appropriate? Changes since v2: - No changes Changes since v1: - Following the feedback from Yann and Arnout, the approach in v2 was completely changed: instead of using LD_LIBRARY_PATH, we now run patchelf at the end of each package installation to fix the RPATH. --- Makefile | 1 - package/Makefile.in | 2 +- package/pkg-generic.mk | 14 ++++++-------- support/scripts/fix-rpath | 31 +++++++++++++++++++------------ 4 files changed, 26 insertions(+), 22 deletions(-)