[RFCv3,13/15] core: change host RPATH handling

Message ID 20171201205352.24287-14-thomas.petazzoni@free-electrons.com
State New
Headers show
Series
  • Per-package host/target directory support
Related show

Commit Message

Thomas Petazzoni Dec. 1, 2017, 8:53 p.m.
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(-)

Comments

Yann E. MORIN Dec. 29, 2017, 5:31 p.m. | #1
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
>
Thomas Petazzoni Dec. 29, 2017, 8:20 p.m. | #2
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
Yann E. MORIN Dec. 30, 2017, 1:56 p.m. | #3
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
Matthew Weber Feb. 6, 2018, midnight | #4
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
Matthew Weber Feb. 6, 2018, 10:04 p.m. | #5
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
Arnout Vandecappelle (Essensium/Mind) Feb. 6, 2018, 11:19 p.m. | #6
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
Matthew Weber Feb. 7, 2018, 6:30 a.m. | #7
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

Patch

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
 }