Make the buildroot toolchain relocatable

The patch allows to use patchelf to sanitize the rpath of the buildroot libraries and binaries.

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 ...to-make-the-rpath-relative-within-a-speci.patch | 313 +++++++++++++++++++++
 package/patchelf/patchelf.hash                     |   2 +-
 package/patchelf/patchelf.mk                       |   6 +-
 3 files changed, 317 insertions(+), 4 deletions(-)
 create mode 100644 package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +0 -0

new file mode 100644
index 0000000..44b1742

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +6 -0

+
+[1] http://lists.busybox.net/pipermail/buildroot/2016-April/159422.html
+---
+ src/patchelf.cc | 179 ++++++++++++++++++++++++++++++++++++++++++++++++--------
+ 1 file changed, 153 insertions(+), 26 deletions(-)
+

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +44 -0

+ 
+ #define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Verneed
+ #define ElfFileParamNames Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Verneed
+@@ -83,6 +85,36 @@ static unsigned int getPageSize()
+     return pageSize;
+ }
+ 
++static bool absolutePathExists(const std::string & path, std::string & canonicalPath)
++{
++    char *cpath = realpath(path.c_str(), NULL);
++    if (cpath) {
++        canonicalPath = cpath;
++        free(cpath);
++        return true;
++    } else {
++        return false;
++    }
++}
++
++static std::string makePathRelative(const std::string & path,
++    const std::string & refPath, const std::string & rootDir)
++{
++    std::string relPath = "$ORIGIN";
++
++    /* Strip root path first */
++    std::string p = path.substr(rootDir.length());
++    std::string refP = refPath.substr(rootDir.length());
++
++    std::size_t pos = refP.find_first_of('/');
++    while (pos != std::string::npos) {
++        pos =refP.find_first_of('/', pos + 1);
++        relPath.append("/..");
++    }
++    relPath.append(p);
++
++    return relPath;
++}
+ 
+ template<ElfFileParams>
+ class ElfFile
+@@ -191,9 +223,14 @@ public:
+ 
+     void setInterpreter(const std::string & newInterpreter);
+ 

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +21 -0

+ 
+     void addNeeded(const std::set<std::string> & libs);
+ 
+@@ -1099,10 +1136,35 @@ static void concatToRPath(std::string & rpath, const std::string & path)
+     rpath += path;
+ }
+ 
++template<ElfFileParams>
++bool ElfFile<ElfFileParamNames>::libFoundInRPath(const std::string & dirName,
++    const std::vector<std::string> neededLibs)
++{
++    std::vector<bool> neededLibFound(neededLibs.size(), false);
++
++    /* For each library that we haven't found yet, see if it
++       exists in this directory. */
++    bool libFound = false;
++    for (unsigned int j = 0; j < neededLibs.size(); ++j)
++	if (!neededLibFound[j]) {
++	    std::string libName = dirName + "/" + neededLibs[j];
++	    try {
++		if (getElfType(readFile(libName, sizeof(Elf32_Ehdr))).machine == rdi(hdr->e_machine)) {

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +7 -0

+ 
+ template<ElfFileParams>
+ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
+-    const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath)
++    const std::vector<std::string> & allowedRpathPrefixes,
++    std::string rootDir, int flags, std::string newRPath)
+ {

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +26 -0

++
++            /* Figure out if we should keep or discard the path; there are several
++               cases to handled:
++               "dirName" starts with "$ORIGIN":
++                   The original build-system already took care of setting a relative
++                   RPATH, resolve it and test if it is worthwhile to keep it.
++               "dirName" start with "rootDir":
++                   The original build-system added some absolute RPATH (absolute on
++                   the build machine). While this is wrong, it can still be fixed; so
++                   test if it is worthwhile to keep it.
++               "rootDir"/"dirName" exists:
++                    The original build-system already took care of setting an absolute
++                    RPATH (absolute in the final rootfs), resolve it and test if it is
++                    worthwhile to keep it;
++                "dirName" points somewhere else:
++                    (can be anywhere: build trees, staging tree, host location,
++                    non-existing location, etc.). Just discard such a path. */
++            if (!dirName.compare(0, 7, "$ORIGIN")) {
++                path = fileDir + dirName.substr(7);
++                if (!absolutePathExists(path, canonicalPath)) {
++                    debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++                    continue;
++                }
++            } else if (!dirName.compare(0, rootDir.length(), rootDir)) {
++                if (!absolutePathExists(dirName, canonicalPath)) {
++                    debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

++
++            if (flags & MODIFY_FLAG_NO_STD_LIB_DIRS) {
++                if (!canonicalPath.compare(rootDir + "/lib") ||
++                    !canonicalPath.compare(rootDir + "/usr/lib")) {

package/patchelf/patchelf.mk +3 -3

index cf2e43a..c880222 100644
@@ -4,9 +4,9 @@
 #
 ################################################################################
 
-PATCHELF_VERSION = 0.9
-PATCHELF_SITE = http://releases.nixos.org/patchelf/patchelf-$(PATCHELF_VERSION)
-PATCHELF_SOURCE = patchelf-$(PATCHELF_VERSION).tar.bz2
+PATCHELF_VERSION = c1f89c077e44a495c62ed0dcfaeca21510df93ef
+PATCHELF_SITE = $(call github,NixOS,patchelf,$(PATCHELF_VERSION))
+PATCHELF_AUTORECONF = YES
 PATCHELF_LICENSE = GPLv3+
 PATCHELF_LICENSE_FILES = COPYING
 

Hi Wolfgang,

The "and" in your subject indicates that this should be two patches: one to
bump the version, a second one to add the feature.

For the version bump, it would be useful to explain why it is needed, because
we bump from a released version to a development commit.

Quoted 22 lines

The patch allows to use patchelf to sanitize the rpath of the buildroot libraries and binaries.

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 ...to-make-the-rpath-relative-within-a-speci.patch | 313 +++++++++++++++++++++
 package/patchelf/patchelf.hash                     |   2 +-
 package/patchelf/patchelf.mk                       |   6 +-
 3 files changed, 317 insertions(+), 4 deletions(-)
 create mode 100644 package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +5 -0

new file mode 100644
index 0000000..44b1742
@@ -0,0 +1,313 @@
+From cc9f15bb80cf36b4a88e10000c3b1c1989b51197 Mon Sep 17 00:00:00 2001
+From: Wolfgang Grandegger <[email protected]>
+Date: Mon, 20 Feb 2017 16:29:24 +0100
+Subject: [PATCH] Add option to make the rpath relative within a specified root
+ directory

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 9

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+
+Running "patchelf" with the option "--make-rpath-relative ROOTDIR" will
+modify or delete the RPATHDIRs according the following rules, similar
+to Martin's patches [1] making the build/SDK relocatable.

For upstream, you probably should start the commit message with an explanation
why we want this, i.e. to be able to move binaries to a different runtime location then where they were built.

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 13

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+
+RPATHDIR starts with "$ORIGIN":
+    The original build-system already took care of setting a relative
+    RPATH, resolve it and test if it is worthwhile to keep it.

IIUC, the --make-rpath-relative by itself doesn't evaluate the "worthwhile to
keep it", so I'd just say "leave unchanged". Same for the others below.

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 18

Quoted 5 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +5 -0

+
+RPATHDIR starts with ROOTDIR:
+    The original build-system added some absolute RPATH (absolute on
+    the build machine). While this is wrong, it can still be fixed; so
+    test if it is worthwhile to keep it.

It is not wrong per se, it's only wrong if you move the binary (i.e. it is
wrong for cross-compilation).

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 23

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+ROOTDIR/RPATHDIR exists:
+    The original build-system already took care of setting an absolute
+    RPATH (absolute in the final rootfs), resolve it and test if it's
+    worthwhile to keep it.

*This* bit is in fact only relevant if --no-standard-libs is specified (i.e.
not for host binaries, only for target binaries).

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 27

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+
+RPATHDIR points somewhere else:
+    (can be anywhere: build trees, staging tree, host location,
+    non-existing location, etc.). Just discard such a path.

I think a warning should be printed in this case, certainly if
--no-standard-libs is specified.

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 32

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+In addition, the option "--no-standard-libs" will discard RPATHDIRs
+ROOTDIR/lib and ROOTDIR/usr/lib. Like "--shrink-rpath", RPATHDIRs
+are discarded if the directories do not contain a library
+referenced by DT_NEEDED fields.

Again, since you're doing two separate things here, I would think that it makes
sense to split this patch in two for upstream. You can still include both upstream patches in a single Buildroot commit, but I think for upstream it is easier to accept the changes if they're separated.

Quoted 12 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +12 -0

+
+[1] http://lists.busybox.net/pipermail/buildroot/2016-April/159422.html
+---
+ src/patchelf.cc | 179 ++++++++++++++++++++++++++++++++++++++++++++++++--------
+ 1 file changed, 153 insertions(+), 26 deletions(-)
+
+diff --git a/src/patchelf.cc b/src/patchelf.cc
+index 5077cd5..24ebe89 100644
+--- a/src/patchelf.cc
++++ b/src/patchelf.cc
+@@ -49,6 +49,8 @@ static int pageSize = PAGESIZE;
+ 

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 48

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+ typedef std::shared_ptr<std::vector<unsigned char>> FileContents;
+ 
++#define MODIFY_FLAG_NO_STD_LIB_DIRS 0x1
++static int modifyFlags;

Since this flag can have just one value, it makes more sense to make it a
boolean, no? Also it should be together with the other command line options like shrinkRPath.

Quoted 44 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +44 -0

+ 
+ #define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Verneed
+ #define ElfFileParamNames Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Verneed
+@@ -83,6 +85,36 @@ static unsigned int getPageSize()
+     return pageSize;
+ }
+ 
++static bool absolutePathExists(const std::string & path, std::string & canonicalPath)
++{
++    char *cpath = realpath(path.c_str(), NULL);
++    if (cpath) {
++        canonicalPath = cpath;
++        free(cpath);
++        return true;
++    } else {
++        return false;
++    }
++}
++
++static std::string makePathRelative(const std::string & path,
++    const std::string & refPath, const std::string & rootDir)
++{
++    std::string relPath = "$ORIGIN";
++
++    /* Strip root path first */
++    std::string p = path.substr(rootDir.length());
++    std::string refP = refPath.substr(rootDir.length());
++
++    std::size_t pos = refP.find_first_of('/');
++    while (pos != std::string::npos) {
++        pos =refP.find_first_of('/', pos + 1);
++        relPath.append("/..");
++    }
++    relPath.append(p);
++
++    return relPath;
++}
+ 
+ template<ElfFileParams>
+ class ElfFile
+@@ -191,9 +223,14 @@ public:
+ 
+     void setInterpreter(const std::string & newInterpreter);
+ 

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 102

Quoted 10 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +10 -0

+-    typedef enum { rpPrint, rpShrink, rpSet, rpRemove } RPathOp;
++    typedef enum { rpPrint, rpShrink, rpMakeRelative, rpSet, rpRemove} RPathOp;
+ 
+-    void modifyRPath(RPathOp op, const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath);
++    bool libFoundInRPath(const std::string & dirName,
++			 const std::vector<std::string> neededLibs);
++
++    void modifyRPath(RPathOp op,
++                     const std::vector<std::string> & allowedRpathPrefixes,
++                     std::string rootDir, int flags, std::string newRPath);

So here I think it should be 'bool noStdLibDirs' rather than flags.

However, wouldn't it be better to instead add forbiddenRpathPrefixes? That
makes it symmetrical with allowedRpathPrefixes and easier to understand IMHO.

Quoted 21 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +21 -0

+ 
+     void addNeeded(const std::set<std::string> & libs);
+ 
+@@ -1099,10 +1136,35 @@ static void concatToRPath(std::string & rpath, const std::string & path)
+     rpath += path;
+ }
+ 
++template<ElfFileParams>
++bool ElfFile<ElfFileParamNames>::libFoundInRPath(const std::string & dirName,
++    const std::vector<std::string> neededLibs)
++{
++    std::vector<bool> neededLibFound(neededLibs.size(), false);
++
++    /* For each library that we haven't found yet, see if it
++       exists in this directory. */
++    bool libFound = false;
++    for (unsigned int j = 0; j < neededLibs.size(); ++j)
++	if (!neededLibFound[j]) {
++	    std::string libName = dirName + "/" + neededLibs[j];
++	    try {
++		if (getElfType(readFile(libName, sizeof(Elf32_Ehdr))).machine == rdi(hdr->e_machine)) {

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 133

Quoted 10 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +10 -0

++		    neededLibFound[j] = true;
++		    libFound = true;
++		} else
++		    debug("ignoring library '%s' because its machine type differs\n", libName.c_str());
++	    } catch (SysError & e) {
++		if (e.errNo != ENOENT) throw;
++	    }
++	}
++    return libFound;
++}

I would do this refactoring part in a separate patch as well.

Quoted 7 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +7 -0

+ 
+ template<ElfFileParams>
+ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
+-    const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath)
++    const std::vector<std::string> & allowedRpathPrefixes,
++    std::string rootDir, int flags, std::string newRPath)
+ {

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 150

Quoted 10 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +10 -0

+     Elf_Shdr & shdrDynamic = findSection(".dynamic");
+ 
+@@ -1153,11 +1215,14 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
+         return;
+     }
+ 
++    if (op == rpMakeRelative && !rpath) {
++        debug("no RPATH to make relative\n");
++        return;
++    }

Minor nit: this should either be merged in the condition for rpShrink above, or
it should move just before the handling of rpMakeRelative below.

[snip]

Quoted 8 lines

+
++ /* Make the the RPATH relative to the specified path */
++ if (op == rpMakeRelative) {
++ std::string fileDir = fileName.substr(0, fileName.find_last_of("/"));
++ newRPath = "";
++
++ for (auto & dirName : splitColonDelimitedString(rpath)) {
++ std::string canonicalPath;

Call it absolutePath, since it is the return value of absolutePathExists().

Quoted 2 lines

++ std::string path;

This doesn't say much. How about resolvedPath? Because basically you're
resolving the path relative to $ORIGIN and rootDir.

Quoted 29 lines

++
++ /* Figure out if we should keep or discard the path; there are several
++ cases to handled:
++ "dirName" starts with "$ORIGIN":
++ The original build-system already took care of setting a relative
++ RPATH, resolve it and test if it is worthwhile to keep it.
++ "dirName" start with "rootDir":

++                   The original build-system added some absolute RPATH (absolute on
++                   the build machine). While this is wrong, it can still be fixed; so
++                   test if it is worthwhile to keep it.
++               "rootDir"/"dirName" exists:
++                    The original build-system already took care of setting an absolute
++                    RPATH (absolute in the final rootfs), resolve it and test if it is
++                    worthwhile to keep it;

++ "dirName" points somewhere else:
++ (can be anywhere: build trees, staging tree, host location,
++ non-existing location, etc.). Just discard such a path. */
++ if (!dirName.compare(0, 7, "$ORIGIN")) {
++ path = fileDir + dirName.substr(7);
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }
++ } else if (!dirName.compare(0, rootDir.length(), rootDir)) {
++ if (!absolutePathExists(dirName, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }

Quoted 8 lines

++ } else {
++ path = rootDir + dirName;
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it's not under the root directory\n",
++ dirName.c_str());
++ continue;
++ }
++ }

This bit could be refactored a little, where you assign path = ... in each
branch of the condition, and check absolutePathExists afterwards.

Quoted 15 lines

++
++ if (flags & MODIFY_FLAG_NO_STD_LIB_DIRS) {
++ if (!canonicalPath.compare(rootDir + "/lib") ||
++ !canonicalPath.compare(rootDir + "/usr/lib")) {
++ debug("removing directory '%s' from RPATH because it's a standard library directory\n",
++ dirName.c_str());
++ continue;
++ }
++ }
++
++ if (!libFoundInRPath(canonicalPath, neededLibs)) {
++ debug("removing directory '%s' from RPATH\n", dirName.c_str());
++ continue;
++ }

Tab-space mixup.

Quoted 5 lines

++
++ /* Finally make "canonicalPath" relative to "filedir" in "rootDir" */
++ concatToRPath(newRPath, makePathRelative(canonicalPath, fileDir, rootDir));
++ debug("keeping relative path of %s", canonicalPath.c_str());

Also tab-space mixup.

Quoted 2 lines

++ }
++ }

Looking at the whole of this functionality, it seems to me that it makes more
sense to completely separate the rpMakeRelative functionality from the rpShrink functionality. You would be allowed to combine the --make-rpath-relative option with the --shrink-rpath option.

So something like the following sequence of changes (each a separate patch of
course):

1. Extend the rpShrink functionality to also remove paths that don't exist (including relative paths). This is something that could be considered a fix of the current shrink functionality.

2. Extend the rpShrink functionality with removal of standard search paths. This would probably be an extra flag similar to --allowed-rpath-prefixes. Actually, I'd not even speak of "standard search paths", instead require the user to pass a list of paths to remove, with an argument e.g. --remove-rpaths.

3. Add an option which *only* does conversion to relative paths, prior to the shrink step.

For the conversion to relative paths, you pass the rootDir. Strictly speaking,
this is not needed for conversion to relative paths; you could have a separate step that verifies that the relative path doesn't go out of the rootDir. Still, we also have to deal with the case where the cross-compilation correctly used an absolute path relative to rootDir rather than a full absolute path including the build directory. So this is probably more elegant.

I realize that what I'm suggesting here is a big change, and I'm not even sure
that this approach would work. So it's completely optional.

[snip]

package/patchelf/patchelf.hash line 2

Quoted 8 lines

package/patchelf/patchelf.hash +1 -1

index 653eb46..7188b2e 100644
@@ -1,2 +1,2 @@
 # Locally calculated
-sha256	a0f65c1ba148890e9f2f7823f4bedf7ecad5417772f64f994004f59a39014f83	patchelf-0.9.tar.bz2
+sha256	d90cbedb2efa516b2373640560aa6b37f0e29c4967611c3312981e30dbbab966  patchelf-c1f89c077e44a495c62ed0dcfaeca21510df93ef.tar.gz

If you convert the tab to two spaces (which is a good idea), also do it for the
first tab.

Hello Arnout,

Am 04.03.2017 um 12:26 schrieb Arnout Vandecappelle:

Quoted 4 lines

Quoted 3 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +3 -0

+RPATHDIR points somewhere else:
+    (can be anywhere: build trees, staging tree, host location,
+    non-existing location, etc.). Just discard such a path.

Quoted 2 lines

I think a warning should be printed in this case, certainly if
--no-standard-libs is specified.

This is very often the case. Here is the host rpath from pulseaudio:

/opt/bdo/alqt5/build/pulseaudio-9.0/src/.libs:/opt/bdo/alqt5/host/usr/x86_64-buildroot-linux-gnu/sysroot/usr/lib:/usr/lib/pulseaudio

Especially references to .libs and sysroot do show up often.

Quoted 9 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 32

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+In addition, the option "--no-standard-libs" will discard RPATHDIRs
+ROOTDIR/lib and ROOTDIR/usr/lib. Like "--shrink-rpath", RPATHDIRs
+are discarded if the directories do not contain a library
+referenced by DT_NEEDED fields.

Again, since you're doing two separate things here, I would think that it makes
sense to split this patch in two for upstream. You can still include both upstream patches in a single Buildroot commit, but I think for upstream it is easier to accept the changes if they're separated.

If you look to the patch, it uses a separate "if" block to implement "--make-rpath-relative". It does not interfere with the "--shrink-rpath" block. Actually, I didn't find an elegant way to extend the "--shrink-rpath" logic. It's to much different.

Quoted 8 lines

Quoted 3 lines

++ void modifyRPath(RPathOp op,
++ const std::vector<std::string> & allowedRpathPrefixes,
++ std::string rootDir, int flags, std::string newRPath);

So here I think it should be 'bool noStdLibDirs' rather than flags.

However, wouldn't it be better to instead add forbiddenRpathPrefixes? That
makes it symmetrical with allowedRpathPrefixes and easier to understand IMHO.

I implemented "forbiddenRpathPrefixes" but I did not find it useful for our purpose. What do you have in mind?

Quoted 9 lines

Quoted 7 lines

++ /* Make the the RPATH relative to the specified path */
++ if (op == rpMakeRelative) {
++ std::string fileDir = fileName.substr(0, fileName.find_last_of("/"));
++ newRPath = "";
++
++ for (auto & dirName : splitColonDelimitedString(rpath)) {
++ std::string canonicalPath;

Call it absolutePath, since it is the return value of absolutePathExists().

The path returned is the canonical one. The absolutePath is an input parameter of that function.

Quoted 6 lines

Quoted 3 lines

++ std::string path;

This doesn't say much. How about resolvedPath? Because basically you're
resolving the path relative to $ORIGIN and rootDir.

It's just a helper variable used for different things.

Quoted 38 lines

Quoted 37 lines

++
++ /* Figure out if we should keep or discard the path; there are several
++ cases to handled:
++ "dirName" starts with "$ORIGIN":
++ The original build-system already took care of setting a relative
++ RPATH, resolve it and test if it is worthwhile to keep it.
++ "dirName" start with "rootDir":

++                   The original build-system added some absolute RPATH (absolute on
++                   the build machine). While this is wrong, it can still be fixed; so
++                   test if it is worthwhile to keep it.
++               "rootDir"/"dirName" exists:
++                    The original build-system already took care of setting an absolute
++                    RPATH (absolute in the final rootfs), resolve it and test if it is
++                    worthwhile to keep it;

++ "dirName" points somewhere else:
++ (can be anywhere: build trees, staging tree, host location,
++ non-existing location, etc.). Just discard such a path. */
++ if (!dirName.compare(0, 7, "$ORIGIN")) {
++ path = fileDir + dirName.substr(7);
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }
++ } else if (!dirName.compare(0, rootDir.length(), rootDir)) {
++ if (!absolutePathExists(dirName, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }
++ } else {
++ path = rootDir + dirName;
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it's not under the root directory\n",
++ dirName.c_str());
++ continue;
++ }
++ }

Quoted 2 lines

This bit could be refactored a little, where you assign path = ... in each
branch of the condition, and check absolutePathExists afterwards.

It makes the code more complex and the reason for the reject needs then to be handled by a separate variable.

Quoted 4 lines

Looking at the whole of this functionality, it seems to me that it makes more
sense to completely separate the rpMakeRelative functionality from the rpShrink functionality. You would be allowed to combine the --make-rpath-relative option with the --shrink-rpath option.

rpMakeRelative *is* separated from the rShrinkRpath case here:

    if (shrinkRPath)
        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "", modifyFlags, "");
    else if (removeRPath)
        elfFile.modifyRPath(elfFile.rpRemove, {}, "", modifyFlags, "");
    else if (setRPath)
        elfFile.modifyRPath(elfFile.rpSet, {}, "", modifyFlags, newRPath);
    else if (makeRPathRelative)
        elfFile.modifyRPath(elfFile.rpMakeRelative, {}, rootDir, modifyFlags, "");

But maybe I have missed something.

Quoted 6 lines

So something like the following sequence of changes (each a separate patch of
course):

1. Extend the rpShrink functionality to also remove paths that don't exist (including relative paths). This is something that could be considered a fix of the current shrink functionality.

You mean pathes not within within a specified rootfs tree?

Quoted 4 lines

2. Extend the rpShrink functionality with removal of standard search paths. This would probably be an extra flag similar to --allowed-rpath-prefixes. Actually, I'd not even speak of "standard search paths", instead require the user to pass a list of paths to remove, with an argument e.g. --remove-rpaths.

Quoted 10 lines

3. Add an option which *only* does conversion to relative paths, prior to the shrink step.

For the conversion to relative paths, you pass the rootDir. Strictly speaking,
this is not needed for conversion to relative paths; you could have a separate step that verifies that the relative path doesn't go out of the rootDir. Still, we also have to deal with the case where the cross-compilation correctly used an absolute path relative to rootDir rather than a full absolute path including the build directory. So this is probably more elegant.

Yes, "rootDir"/"dirName" exists. But to check if the path is OK we need to extend it to an absolute path first.

The question is if the single steps are useful for other purposes as well or if we need to apply them all together anyway... and just screw-up/misuse the "--shrink-rpath" case.

Quoted 2 lines

I realize that what I'm suggesting here is a big change, and I'm not even sure
that this approach would work. So it's completely optional.

See above. Let's wait and see what the patchelf maintainers think?

Quoted 3 lines

Hello Arnout,

Am 04.03.2017 um 12:26 schrieb Arnout Vandecappelle:

[snip]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +3 -0

+RPATHDIR points somewhere else:
+    (can be anywhere: build trees, staging tree, host location,
+    non-existing location, etc.). Just discard such a path.

Quoted 8 lines

Quoted 2 lines

I think a warning should be printed in this case, certainly if
--no-standard-libs is specified.

This is very often the case. Here is the host rpath from pulseaudio:

/opt/bdo/alqt5/build/pulseaudio-9.0/src/.libs:/opt/bdo/alqt5/host/usr/x86_64-buildroot-linux-gnu/sysroot/usr/lib:/usr/lib/pulseaudio

Especially references to .libs and sysroot do show up often.

Ah, yes of course, I didn't think of that. Indeed, pointing to the build
directory or to staging is kind of expected.

Quoted 10 lines

Quoted 9 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch line 32

Quoted 4 lines

package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch new file +4 -0

+In addition, the option "--no-standard-libs" will discard RPATHDIRs
+ROOTDIR/lib and ROOTDIR/usr/lib. Like "--shrink-rpath", RPATHDIRs
+are discarded if the directories do not contain a library
+referenced by DT_NEEDED fields.

Again, since you're doing two separate things here, I would think that it makes
sense to split this patch in two for upstream. You can still include both upstream patches in a single Buildroot commit, but I think for upstream it is easier to accept the changes if they're separated.

Quoted 4 lines

If you look to the patch, it uses a separate "if" block to implement "--make-rpath-relative". It does not interfere with the "--shrink-rpath" block. Actually, I didn't find an elegant way to extend the "--shrink-rpath" logic. It's to much different.

Even if both option are part of the --make-rpath-relative bit, it makes sense
to treat them in two separate commits.

[snip]

Quoted 11 lines

Quoted 8 lines

Quoted 3 lines

++ void modifyRPath(RPathOp op,
++ const std::vector<std::string> & allowedRpathPrefixes,
++ std::string rootDir, int flags, std::string newRPath);

So here I think it should be 'bool noStdLibDirs' rather than flags.

However, wouldn't it be better to instead add forbiddenRpathPrefixes? That
makes it symmetrical with allowedRpathPrefixes and easier to understand IMHO.

I implemented "forbiddenRpathPrefixes" but I did not find it useful for our purpose. What do you have in mind?

For us, the forbiddenRpathPrefixes will always be /lib:/usr/lib. But for
upstream, it is more useful to have a general solution that can also be applied in other situations. Concretely, I'm thinkinf of /lib64 and /lib/x86_64-linux-gnu - in Buildroot they are symlinks to /lib so will already be caught by the path canonification, but in other distros they may be distinct directories that are still in the default search path.

Note that forbiddenRpathPrefixes is actually wrong, since it shouldn't be
prefixes, it should be full paths. So forbiddenRpaths.

[snip]

Quoted 11 lines

Quoted 9 lines

Quoted 7 lines

++ /* Make the the RPATH relative to the specified path */
++ if (op == rpMakeRelative) {
++ std::string fileDir = fileName.substr(0, fileName.find_last_of("/"));
++ newRPath = "";
++
++ for (auto & dirName : splitColonDelimitedString(rpath)) {
++ std::string canonicalPath;

Call it absolutePath, since it is the return value of absolutePathExists().

The path returned is the canonical one. The absolutePath is an input parameter of that function.

Ah, I interpreted "absolute" to mean "with all symlinks expanded", but of
course it just means "starts with /".

Quoted 47 lines

Quoted 6 lines

Quoted 3 lines

++ std::string path;

This doesn't say much. How about resolvedPath? Because basically you're
resolving the path relative to $ORIGIN and rootDir.

It's just a helper variable used for different things.

Quoted 38 lines

Quoted 37 lines

++
++ /* Figure out if we should keep or discard the path; there are several
++ cases to handled:
++ "dirName" starts with "$ORIGIN":
++ The original build-system already took care of setting a relative
++ RPATH, resolve it and test if it is worthwhile to keep it.
++ "dirName" start with "rootDir":

++                   The original build-system added some absolute RPATH (absolute on
++                   the build machine). While this is wrong, it can still be fixed; so
++                   test if it is worthwhile to keep it.
++               "rootDir"/"dirName" exists:
++                    The original build-system already took care of setting an absolute
++                    RPATH (absolute in the final rootfs), resolve it and test if it is
++                    worthwhile to keep it;

++ "dirName" points somewhere else:
++ (can be anywhere: build trees, staging tree, host location,
++ non-existing location, etc.). Just discard such a path. */
++ if (!dirName.compare(0, 7, "$ORIGIN")) {
++ path = fileDir + dirName.substr(7);
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }
++ } else if (!dirName.compare(0, rootDir.length(), rootDir)) {
++ if (!absolutePathExists(dirName, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
++ continue;
++ }
++ } else {
++ path = rootDir + dirName;
++ if (!absolutePathExists(path, canonicalPath)) {
++ debug("removing directory '%s' from RPATH because it's not under the root directory\n",
++ dirName.c_str());
++ continue;
++ }
++ }

Quoted 5 lines

Quoted 2 lines

This bit could be refactored a little, where you assign path = ... in each
branch of the condition, and check absolutePathExists afterwards.

It makes the code more complex and the reason for the reject needs then to be handled by a separate variable.

Fair enough. But then make the path variable local to the contexts that need it
(which solves my undescriptive name comment as well).

[snip]

Quoted 16 lines

Quoted 4 lines

Looking at the whole of this functionality, it seems to me that it makes more
sense to completely separate the rpMakeRelative functionality from the rpShrink functionality. You would be allowed to combine the --make-rpath-relative option with the --shrink-rpath option.

rpMakeRelative *is* separated from the rShrinkRpath case here:

    if (shrinkRPath)
        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "", modifyFlags, "");
    else if (removeRPath)
        elfFile.modifyRPath(elfFile.rpRemove, {}, "", modifyFlags, "");
    else if (setRPath)
        elfFile.modifyRPath(elfFile.rpSet, {}, "", modifyFlags, newRPath);
    else if (makeRPathRelative)
        elfFile.modifyRPath(elfFile.rpMakeRelative, {}, rootDir, modifyFlags, "");

Quoted 1 line

But maybe I have missed something.

With "separated", I meant a "separate pass", like rpPrint, that can be combined
with one of the other passes. So first do rpPrint, then do rpMakeRelative, then do one of rpShrink, rpRemove or rpSet.

Quoted 8 lines

Quoted 6 lines

So something like the following sequence of changes (each a separate patch of
course):

1. Extend the rpShrink functionality to also remove paths that don't exist (including relative paths). This is something that could be considered a fix of the current shrink functionality.

You mean pathes not within within a specified rootfs tree?

In this stage there is no specified rootfs tree yet. So for our use case this
would indeed be wrong, because (without roots tree specification) the correct absolute rpaths will not exist on the host. But this will be fixed in step 4.

However, looking a second time, this is in fact already done in the rpShrink
step, because any directory which doesn't contain a NEEDED library is removed. A non-existent directory *certainly* doesn't contain a NEEDED library.

Quoted 4 lines

Quoted 4 lines

2. Extend the rpShrink functionality with removal of standard search paths. This would probably be an extra flag similar to --allowed-rpath-prefixes. Actually, I'd not even speak of "standard search paths", instead require the user to pass a list of paths to remove, with an argument e.g. --remove-rpaths.

For clarity: this is the forbiddenRpaths that I talked about before.

Quoted 18 lines

Quoted 10 lines

3. Add an option which *only* does conversion to relative paths, prior to the shrink step.

For the conversion to relative paths, you pass the rootDir. Strictly speaking,
this is not needed for conversion to relative paths; you could have a separate step that verifies that the relative path doesn't go out of the rootDir. Still, we also have to deal with the case where the cross-compilation correctly used an absolute path relative to rootDir rather than a full absolute path including the build directory. So this is probably more elegant.

Yes, "rootDir"/"dirName" exists. But to check if the path is OK we need to extend it to an absolute path first.

The question is if the single steps are useful for other purposes as well or if we need to apply them all together anyway... and just screw-up/misuse the "--shrink-rpath" case.

Quoted 4 lines

Quoted 2 lines

I realize that what I'm suggesting here is a big change, and I'm not even sure
that this approach would work. So it's completely optional.

See above. Let's wait and see what the patchelf maintainers think?

OK.

Am 06.03.2017 um 00:13 schrieb Arnout Vandecappelle:

Quoted 6 lines

For us, the forbiddenRpathPrefixes will always be /lib:/usr/lib. But for
upstream, it is more useful to have a general solution that can also be applied in other situations. Concretely, I'm thinkinf of /lib64 and /lib/x86_64-linux-gnu - in Buildroot they are symlinks to /lib so will already be caught by the path canonification, but in other distros they may be distinct directories that are still in the default search path.

I just see... we have a problem here. There are no symlinks:

patching ELF file '/opt/bdo/dcu_host/host/usr/x86_64-pc-linux-gnu/x86_64-buildroot-linux-gnu/lib/libopcodes-2.25.51.so' removing directory '/opt/bdo/dcu_host/host/usr/lib' from RPATH
new rpath is ''

The rpath is removed because it did not find the needed library in "HOST_DIR/usr/lib/". Actually it's in "HOST_DIR/usr/x86_64-pc-linux-gnu/x86_64-buildroot-linux-gnu/lib.

I think I now understand your proposal... Here are the steps assuming that we extend --shrink-rpath. We also need --make-rpath-relative and --remove-rpaths. The patchelf command than would be:

$ patchelf --shrink-rpath --remove-rpaths $ROOTDIR/lib:$ROOTDIR/usr/lib
	   --make-rpath-relative $ROOTDIR

Actions before --shrink-rpath to deal with relative paths:
- resolve $ORIGIN
- resolve $ROOTDIR/path-within-rootdir if it exists

Actions in --shrink-rpath:
- remove rpath dir if not within $ROOTDIR (we cannot use allowed-rpath-prefixes)

- remove rpath dirs specified by "--remove-rpaths"
- remove rpath dirs if they do not contain any needed library

Actions in --make-rpath-relative:
- Replace the valid absolute path with a relative one using $ORIGIN

I'm going to check if I can integrate it nicely into the existing code. I think it will be more intrusive than my first proposal. More soon...

Quoted 1 line

Am 06.03.2017 um 00:13 schrieb Arnout Vandecappelle:

[snip]

Quoted 13 lines

Quoted 6 lines

For us, the forbiddenRpathPrefixes will always be /lib:/usr/lib. But for
upstream, it is more useful to have a general solution that can also be applied in other situations. Concretely, I'm thinkinf of /lib64 and /lib/x86_64-linux-gnu - in Buildroot they are symlinks to /lib so will already be caught by the path canonification, but in other distros they may be distinct directories that are still in the default search path.

I just see... we have a problem here. There are no symlinks:

patching ELF file '/opt/bdo/dcu_host/host/usr/x86_64-pc-linux-gnu/x86_64-buildroot-linux-gnu/lib/libopcodes-2.25.51.so' removing directory '/opt/bdo/dcu_host/host/usr/lib' from RPATH
new rpath is ''

Quoted 3 lines

The rpath is removed because it did not find the needed library in "HOST_DIR/usr/lib/". Actually it's in "HOST_DIR/usr/x86_64-pc-linux-gnu/x86_64-buildroot-linux-gnu/lib.

I don't know what you're trying to say here... We were talking about the
--no-standard-libs (which is used for target packages) while here you give an example of a host package. Also, the path you mention *can* be removed because it is not helpful, the library you need is in a subdirectory of that directory.

[snip]

Quoted 6 lines

I think I now understand your proposal... Here are the steps assuming that we extend --shrink-rpath. We also need --make-rpath-relative and --remove-rpaths. The patchelf command than would be:

$ patchelf --shrink-rpath --remove-rpaths $ROOTDIR/lib:$ROOTDIR/usr/lib
	   --make-rpath-relative $ROOTDIR

Exactly, and for the host packages it would just be --shrink-rpath
--make-rpath-relative.

Quoted 4 lines

Actions before --shrink-rpath to deal with relative paths:
- resolve $ORIGIN
- resolve $ROOTDIR/path-within-rootdir if it exists

Yes, so these are only executed when --make-rpath-relative is specified
(otherwise there is no $ROOTDIR, and otherwise you loose the $ORIGIN).

Quoted 2 lines

Actions in --shrink-rpath:
- remove rpath dir if not within $ROOTDIR (we cannot use allowed-rpath-prefixes)

Hm, I didn't think of that before - why can't we use allowed-rpath-prefixes?

Quoted 6 lines

- remove rpath dirs specified by "--remove-rpaths"
- remove rpath dirs if they do not contain any needed library

Actions in --make-rpath-relative:
- Replace the valid absolute path with a relative one using $ORIGIN

Quoted 2 lines

I'm going to check if I can integrate it nicely into the existing code. I think it will be more intrusive than my first proposal. More soon...

Maybe it's best to already post your current patches (split into two patches
and maybe with some minor cleanups that we agreed on) to upstream, and add in the cover letter the idea that it could be done in a different way. If what you have now is good enough for upstream, there is no need to change it.

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

Hello,

Quoted 7 lines · Wolfgang Grandegger

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Quoted 6 lines

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

Now that patchelf is much smarter, I am wondering if we still really need a utility script just to call patchelf. Have you tried bringing this logic into the make logic?

Hello,

Am 03.03.2017 um 15:48 schrieb Thomas Petazzoni:

Quoted 18 lines

Hello,

Quoted 14 lines · Wolfgang Grandegger

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106
++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

Quoted 3 lines

Now that patchelf is much smarter, I am wondering if we still really need a utility script just to call patchelf. Have you tried bringing this logic into the make logic?

Well, I think three magic "find" oneliners could do the job. I will try and see.

Quoted 1 line

Quoted 1 line

Sanitizing RPATH in target and staging directory

Quoted 1 line

Quoted 1 line

Rendering the SDK relocatable

Quoted 31 lines

Hello,

Am 03.03.2017 um 17:39 schrieb Wolfgang Grandegger:

Quoted 27 lines

Hello,

Am 03.03.2017 um 15:48 schrieb Thomas Petazzoni:

Quoted 18 lines

Hello,

Quoted 7 lines · Wolfgang Grandegger

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Quoted 7 lines

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106
++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

Quoted 3 lines

Now that patchelf is much smarter, I am wondering if we still really need a utility script just to call patchelf. Have you tried bringing this logic into the make logic?

Well, I think three magic "find" oneliners could do the job. I will try and see.

Quoted 2 lines

Here are my current oneliners in the Makefile. It mainly shows what needs to be done.

I think they're sufficiently complicated to warrant offloading to a script
after all. Especially since you need the same structure three times, so it would have to be factored into a macro, which makes the makefile more complicated. Alternatively, you could keep the find command in make, and use -exec support/scripts/fix-rpath '{}' ';' - this avoids all the tricky xargs stuff.

Quoted 6 lines

Unfortunately, the scanning for ELF files may take rather long:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Sanitizing RPATH in target and staging directory

 time find /opt/bdo/x86_host/target -type f -print0 | xargs -0 -I {} sh -c \
	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \

You can actually include this in the find command itself, it's almost twice as
fast:

find /opt/bdo/x86_host/target -type f \
	-exec patchelf --print-rpath '{}' ';' \
	-exec  support/scripts/fix-rpath '{}' ';'

Quoted 4 lines

	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/target \
	--no-standard-lib-dirs {}; fi"

real 0m9.644s

Can you quantify which bit is taking the time here? Is it the find itself, or
the patchelf --print-rpath, or the final patchelf?

Also, how does the time compare to the rest of the finalize step and creating a
tarball?

Quoted 4 lines

user 0m0.032s
sys 0m0.360s

  time find /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot -type f -print0 | xargs -0 -I {} sh -c \

We could optimise this a little by eliminating directories that certainly don't
contain interesting files, like /usr/include. Or alternatively, explicitly select only "\( -path lib -o -path bin -o -path sbin \)".

However, now I think of it: why do we do this for staging? Binaries in staging
are never executed... Is it just to eliminate all references to HOST_DIR from the binaries?

Quoted 8 lines

	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \
	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot \
	--no-standard-lib-dirs {}; fi"

real 0m46.433s
user 0m0.240s
sys 0m1.980s

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Rendering the SDK relocatable

cp /opt/bdo/x86_host/host/usr/bin/patchelf /opt/bdo/x86_host/host/usr/bin/patchelf.__copy__

Why do you need this? To make sure patchelf itself is processed as well? Does
it contain an invalid rpath? If yes, isn't it easier to patch the patchelf build system so it uses $ORIGIN already?

Quoted 13 lines

  time find /opt/bdo/x86_host/host -path /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot -prune -o \
	-path /opt/bdo/x86_host/host/usr/bin/patchelf -prune -o -type f -print0 | xargs -0 -I {} sh -c \
	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \
	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/host {}; \
	fi"

mv /opt/bdo/dcu_host/host/usr/bin/patchelf.__copy__ /opt/bdo/dcu_host/host/usr/bin/patchelf

real 0m23.154s
user 0m0.144s
sys 0m1.124s

Quoted 3 lines

Using "file" to test if it's an ELF files is much slower. Using "patchelf --print-rpath {} >/dev/null 2>&1" is much smarter and allows to run the path sanitation without ignoring errors.

readelf -h is still a little faster, but it also matches files without rpath.

To just check if it's an ELF file, it's actually enough to do "cmp -n 4" with another ELF file (e.g. patchelf itself). That gives even more false positives (e.g. object files). So if one of those is chosen, a further check with patchelf --print-rpath is still needed, or errors have to be ignored.

Quoted 2 lines

Because some ELF files are not writeable, we need a chmod first.

Shouldn't we restore the original permissions?

Quoted 5 lines

For the
host tree, we also need special handling for patchelf to work around the "file in use" issue.

"xargs" could be speedup using "-P NUM". Would that be an option?

Yes, we could use PARALLEL_JOBS.

Quoted 6 lines

Am 07.03.2017 um 00:49 schrieb Arnout Vandecappelle:

Quoted 5 lines · Wolfgang Grandegger

Quoted 2 lines

Hello,

[snip]

Quoted 11 lines

Quoted 10 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Sanitizing RPATH in target and staging directory

 time find /opt/bdo/x86_host/target -type f -print0 | xargs -0 -I {} sh -c \
	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \

You can actually include this in the find command itself, it's almost twice as
fast:

find /opt/bdo/x86_host/target -type f \
	-exec patchelf --print-rpath '{}' ';' \
	-exec  support/scripts/fix-rpath '{}' ';'

Quoted 1 line

OK, it' two times faster, indeed

Actually with such a construct, and assuming we don't bother with restoring the
permissions, a helper shell script isn't needed because we can just chain the chmod and patchelf calls with further -exec calls. Of course with -exec, xargs -P is not possible so you have to evaluate a little what is the best approach.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/target \
	--no-standard-lib-dirs {}; fi"

real 0m9.644s

Can you quantify which bit is taking the time here? Is it the find itself, or
the patchelf --print-rpath, or the final patchelf?

The initial ELF file testing takes most of the time. The processing of the ELF file itself doesn't matter much:

Quoted 6 lines

  $ find target -type f | wc -l
  3902
  $ find host/usr/x86_64-buildroot-linux-gnu/sysroot -type f | wc -l
  21413
  $ find host -path host/usr/x86_64-buildroot-linux-gnu/sysroot -prune -o -type f | wc -l
  10861

What kind of config is this? Many packages? Or just an internal toolchain?

Quoted 20 lines

Quoted 2 lines

Also, how does the time compare to the rest of the finalize step and creating a
tarball?

The rest of "target-finalize" takes half a second and the tar:

$ time tar cjf /tmp/target.tar.bz2 target
real	0m21.742s

But it depends on the compression, of course.

Quoted 8 lines

Quoted 4 lines

user 0m0.032s
sys 0m0.360s

  time find /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot -type f -print0 | xargs -0 -I {} sh -c \

We could optimise this a little by eliminating directories that certainly don't
contain interesting files, like /usr/include. Or alternatively, explicitly select only "\( -path lib -o -path bin -o -path sbin \)".

Quoted 6 lines

  $ find host/usr/x86_64-buildroot-linux-gnu/sysroot/usr/include/ -type f | wc -l
  6946
  $ find host/usr/x86_64-buildroot-linux-gnu/sysroot/usr/share/ -type f | wc -l
  9273

But there is one ELF file in ".../usr/share".

Ah yes, I also found one:
/usr/share/bash-completion/helpers/gst-completion-helper-1.0

So in that case we can't skip share. We certainly can skip include, I think,
though the benefit is perhaps limited.

Of course, if the entire staging can be skipped it's even easier :-)

Quoted 17 lines

Quoted 3 lines

However, now I think of it: why do we do this for staging? Binaries in staging
are never executed... Is it just to eliminate all references to HOST_DIR from the binaries?

Good question! So far I followed Samuels proposal (now on CC).

Quoted 11 lines

Quoted 10 lines

	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \
	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot \
	--no-standard-lib-dirs {}; fi"

real 0m46.433s
user 0m0.240s
sys 0m1.980s

Quoted 1 line

Quoted 1 line

Quoted 1 line

Rendering the SDK relocatable

cp /opt/bdo/x86_host/host/usr/bin/patchelf /opt/bdo/x86_host/host/usr/bin/patchelf.__copy__

Quoted 5 lines

Quoted 3 lines

Why do you need this? To make sure patchelf itself is processed as well? Does
it contain an invalid rpath? If yes, isn't it easier to patch the patchelf build system so it uses $ORIGIN already?

We cannot update the "patchelf" binary while it's in use.

Yes, but that's avoided with the -name patchelf -prune bit.

Quoted 11 lines

There no
need to touch it if it already uses a proper rpath, of course. Currently it uses:

  $ readelf -d host/usr/bin/patchelf
  ...
  0x0000000000000001 (NEEDED)             Gemeinsame Bibliothek [libstdc++.so.6]
  0x0000000000000001 (NEEDED)             Gemeinsame Bibliothek [libgcc_s.so.1]
  0x0000000000000001 (NEEDED)             Gemeinsame Bibliothek [libc.so.6]
  0x000000000000000f (RPATH)              Bibliothek rpath: [/opt/bdo/dcu_host/host/usr/lib]

Quoted 2 lines

"patchelf --make-relative" will drop the rpath above, because the first
two needed libs are not in the listed rpath but in "host/usr/x86_64-buildroot-linux-gnu/lib64".

That's the staging dir - it should certainly NOT use anything from there.

Quoted 2 lines

Running patchelf with "LD_DEBUG" tells me that it will take the libraries from the host (/usr/lib). Just wondering if that's correct!?

Yes it's correct, those 3 libraries are standard host libraries that can be
found in the standard paths.

So I've checked where this rpath comes from. Turns out it is added by
Buildroot, through HOST_LDFLAGS. This is in fact needed to make sure that an executable that uses libraries from HOST_DIR works - see commit 4fdecac9d692b8d6f071ba6ad938b6ad68b675fd. So we can either:

- keep this __copy__ manipulation in the patchelf step; or
- override HOST_LDFLAGS in patchelf.mk.

I'm cool either way, so since you already have this workaround, just keep it.
Perhaps a comment above it explaining why would be useful though.

Quoted 20 lines

Quoted 12 lines

Quoted 12 lines

  time find /opt/bdo/x86_host/host -path /opt/bdo/x86_host/host/usr/x86_64-buildroot-linux-gnu/sysroot -prune -o \
	-path /opt/bdo/x86_host/host/usr/bin/patchelf -prune -o -type f -print0 | xargs -0 -I {} sh -c \
	"if patchelf --print-rpath {} >/dev/null 2>&1; then chmod +w {}; \
	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/host {}; \
	fi"

mv /opt/bdo/dcu_host/host/usr/bin/patchelf.__copy__ /opt/bdo/dcu_host/host/usr/bin/patchelf

real 0m23.154s
user 0m0.144s
sys 0m1.124s

Quoted 5 lines

Quoted 3 lines

Using "file" to test if it's an ELF files is much slower. Using "patchelf --print-rpath {} >/dev/null 2>&1" is much smarter and allows to run the path sanitation without ignoring errors.

readelf -h is still a little faster, but it also matches files without rpath.

Yes, 38 vs. 44 seconds.

Quoted 6 lines

Quoted 4 lines

To just check if it's an ELF file, it's actually enough to do "cmp -n 4" with another ELF file (e.g. patchelf itself). That gives even more false positives (e.g. object files). So if one of those is chosen, a further check with patchelf --print-rpath is still needed, or errors have to be ignored.

Yep.

To be evaluated if the speedup from using cmp is worth the false positives.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Because some ELF files are not writeable, we need a chmod first.

Shouldn't we restore the original permissions?

Maybe! There are actually two libraries not being writeable. Can't tell if that's by purpose.

Perhaps as an easier way to restore permissions, we could do it in patchelf
itself: add something like --force that does a chmod if needed, similar to how some editors do it.

There are actually quite a few packages that install library read-only, e.g.
Python and openssl. I guess it is on purpose, but I'm not sure if it is important.

Am 07.03.2017 um 18:40 schrieb Arnout Vandecappelle:

Quoted 2 lines

Quoted 4 lines

So in that case we can't skip share. We certainly can skip include, I think,
though the benefit is perhaps limited.

Of course, if the entire staging can be skipped it's even easier :-)

Yep.

Quoted 5 lines

Quoted 3 lines

"patchelf --make-relative" will drop the rpath above, because the first two needed libs are not in the listed rpath but in "host/usr/x86_64-buildroot-linux-gnu/lib64".

That's the staging dir - it should certainly NOT use anything from there.

No, the staging dir is "host/usr/x86_64-buildroot-linux-gnu/sysroot/".

Quoted 6 lines

Quoted 3 lines

Running patchelf with "LD_DEBUG" tells me that it will take the libraries from the host (/usr/lib). Just wondering if that's correct!?

Yes it's correct, those 3 libraries are standard host libraries that can be
found in the standard paths.

Hm, what are the libraries in "host/usr/x86_64-buildroot-linux-gnu/lib64" then good for? They work if I use "LD_LIBRARY_PATH" to run the executable.

Quoted 1 line

To be evaluated if the speedup from using cmp is worth the false positives.

I wrote a little C program just checking the first 4 bytes of the file. The saving is 19 vs. 22 seconds. With "readelf" I have similar results.

patchelf is
written in C++... maybe that's the reason why it's slower.

Quoted 3 lines

Am 07.03.2017 um 18:40 schrieb Arnout Vandecappelle:

Quoted 2 lines

[snip]

Quoted 6 lines

Quoted 4 lines

So in that case we can't skip share. We certainly can skip include, I think,
though the benefit is perhaps limited.

Of course, if the entire staging can be skipped it's even easier :-)

Yep.

I propose that you start with the simple option, but don't do it for staging
for the time being. Until Samuel comes with an argument why staging is necessary :-)

[snip]

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

"patchelf --make-relative" will drop the rpath above, because the first two needed libs are not in the listed rpath but in "host/usr/x86_64-buildroot-linux-gnu/lib64".

That's the staging dir - it should certainly NOT use anything from there.

No, the staging dir is "host/usr/x86_64-buildroot-linux-gnu/sysroot/".

My bad. Still, it's a cross-directory. It contains libgcc and other stuff from
the cross-compiler. So it is NOT meant to be used by the host binaries.

It's easier to see when you do actual cross-compilation, then you'll see the
binaries there are for the target, not the host.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Running patchelf with "LD_DEBUG" tells me that it will take the libraries from the host (/usr/lib). Just wondering if that's correct!?

Yes it's correct, those 3 libraries are standard host libraries that can be
found in the standard paths.

Hm, what are the libraries in "host/usr/x86_64-buildroot-linux-gnu/lib64" then good for? They work if I use "LD_LIBRARY_PATH" to run the executable.

Since your target is x86_64, just like your host, running a target binary will
just work as long as it can find the libraries (i.e. as long as you set LD_LIBRARY_PATH).

[snip]

Quoted 2 lines

Quoted 1 line

To be evaluated if the speedup from using cmp is worth the false positives.

Quoted 2 lines

I wrote a little C program just checking the first 4 bytes of the file. The saving is 19 vs. 22 seconds. With "readelf" I have similar results.

You mean 19 seconds for the C program vs 22 seconds for cmp? That's what I
would expect indeed because cmp basically does the same. In my measurements, readelf still was significantly slower - I didn't write down the numbers but from memory it was like 25%.

Quoted 3 lines

patchelf is
written in C++... maybe that's the reason why it's slower.

Well, it's rather because patchelf reads the entire file into a std::vector,
while readelf will just seek to the bits that are requested.

By the way, did you already post your patches to the patchelf list?

Am 12.03.2017 um 21:53 schrieb Arnout Vandecappelle:

Quoted 2 lines

Quoted 4 lines

You mean 19 seconds for the C program vs 22 seconds for cmp? That's what I
would expect indeed because cmp basically does the same. In my measurements, readelf still was significantly slower - I didn't write down the numbers but from memory it was like 25%.

No, 19 sec for the C program just checking the first 4 bytes. 22 with patchelf. And with readelf I think it was close to 19 sec as well. I will doublecheck, though.

Quoted 10 lines

Quoted 4 lines

patchelf is
written in C++... maybe that's the reason why it's slower.

Well, it's rather because patchelf reads the entire file into a std::vector,
while readelf will just seek to the bits that are requested.

Quoted 1 line

By the way, did you already post your patches to the patchelf list?

I'm currently preparing a new patch series. I did not find a mailing list for the patchelf project. Maybe I have to use the GIT hub interface.

Quoted 3 lines

Am 12.03.2017 um 21:53 schrieb Arnout Vandecappelle:

Quoted 2 lines

[snip]

Quoted 5 lines

Quoted 4 lines

You mean 19 seconds for the C program vs 22 seconds for cmp? That's what I
would expect indeed because cmp basically does the same. In my measurements, readelf still was significantly slower - I didn't write down the numbers but from memory it was like 25%.

Quoted 3 lines

No, 19 sec for the C program just checking the first 4 bytes. 22 with patchelf. And with readelf I think it was close to 19 sec as well. I will doublecheck, though.

I think I had a bigger difference, but anyway even 20% isn't really worth it,
probably.

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

patchelf is
written in C++... maybe that's the reason why it's slower.

Well, it's rather because patchelf reads the entire file into a std::vector,
while readelf will just seek to the bits that are requested.

Quoted 4 lines

Quoted 1 line

By the way, did you already post your patches to the patchelf list?

I'm currently preparing a new patch series. I did not find a mailing list for the patchelf project. Maybe I have to use the GIT hub interface.

Yes, github pull request is the way to go.

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

support/scripts/fix-rpath new file +10 -0

new file mode 100755
index 0000000..bde2c17
@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+
+# Copyright (C) 2016 Samuel Martin <[email protected]>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,

support/scripts/fix-rpath new file +45 -0

+
+usage() {
+  cat <<EOF >&2
+Usage:  ${0} TREE_KIND TREE_ROOT
+
+Description:
+
+        This script scans a tree and sanitize ELF files' RPATH found in there.
+
+        Sanitization behaves the same whatever the kindd of the processed tree, but
+        the resulting RPATH differs.
+
+        Sanitization action:
+        - remove RPATH pointing outside of the tree
+        - for RPATH pointing in the tree:
+          - if they point to standard location (/lib, /usr/lib): remove them
+          - otherwise: make them relative using \$ORIGIN
+
+        For the target tree:
+        - scan the whole tree for sanitization
+
+        For the staging tree :
+        - scan the whole tree for sanitization
+
+        For the host tree:
+        - skip the staging tree for sanitization
+        - add \$HOST_DIR/{lib,usr/lib} to RPATH (as relative pathes)
+
+Arguments:
+
+        TREE_KIND   Kind of tree to be processed.
+                    Allowed values: host, target, staging
+
+        TREE_ROOT   Path to the root of the tree to be scaned
+
+Environment:
+
+        PATCHELF        patchelf program to use
+                        (default: patchelf)
+EOF
+}
+
+: ${PATCHELF:=patchelf}
+
+main() {

support/scripts/fix-rpath new file +27 -0

+
+            ;;
+        staging|target)
+            # discard ${hostdir}/lib and ${hostdir}/usr/lib
+            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+
+            ;;
+        *)
+            usage
+            exit 1
+            ;;
+    esac
+
+    find_args+=( "-type" "f" "-print" )
+
+    while read file ; do
+        # some files are not writable
+        chmod u+w $file
+        # call patchelf for each regular file; error will silently be ignored.
+        ${PATCHELF} --debug --make-rpath-relative "${basedir}" ${sanitize_extra_args[@]} "${file}" >> /dev/null 2>&1
+    done < <(find ${find_args[@]})
+
+    # ignore errors
+    return 0
+}
+
+main ${@}

Hi Wolfgang,

I realize that this patch is going to be dropped because it is going to be done
directly from the makefiles, but I have some hopefully useful comments still.

First of all: make sure the original author is explicitly in Cc. git send-email
normally does that automatically so I'm surprised Samuel isn't there.

Quoted 34 lines

From: Samuel Martin <[email protected]>

This commit introduces a fix-rpath script able to scan a tree, detect ELF files, check their RPATH and fix it in a proper way. The RPATH fixup is done by the patchelf utility using the option "--make-rpath-relative".

Signed-off-by Samuel Martin <[email protected]>

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

support/scripts/fix-rpath new file +14 -0

new file mode 100755
index 0000000..bde2c17
@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+
+# Copyright (C) 2016 Samuel Martin <[email protected]>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#

support/scripts/fix-rpath line 20

Quoted 6 lines

support/scripts/fix-rpath new file +6 -0

+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+
+#set -e
+#set -v

Don't include debugging cruft in the final patch.

Quoted 52 lines

support/scripts/fix-rpath new file +52 -0

+
+usage() {
+  cat <<EOF >&2
+Usage:  ${0} TREE_KIND TREE_ROOT
+
+Description:
+
+        This script scans a tree and sanitize ELF files' RPATH found in there.
+
+        Sanitization behaves the same whatever the kindd of the processed tree, but
+        the resulting RPATH differs.
+
+        Sanitization action:
+        - remove RPATH pointing outside of the tree
+        - for RPATH pointing in the tree:
+          - if they point to standard location (/lib, /usr/lib): remove them
+          - otherwise: make them relative using \$ORIGIN
+
+        For the target tree:
+        - scan the whole tree for sanitization
+
+        For the staging tree :
+        - scan the whole tree for sanitization
+
+        For the host tree:
+        - skip the staging tree for sanitization
+        - add \$HOST_DIR/{lib,usr/lib} to RPATH (as relative pathes)
+
+Arguments:
+
+        TREE_KIND   Kind of tree to be processed.
+                    Allowed values: host, target, staging
+
+        TREE_ROOT   Path to the root of the tree to be scaned
+
+Environment:
+
+        PATCHELF        patchelf program to use
+                        (default: patchelf)
+EOF
+}
+
+: ${PATCHELF:=patchelf}
+
+main() {
+    local tree="${1}"
+    local basedir="$(readlink -f "${2}")"
+
+    local find_args=( "${basedir}" )
+    local sanitize_extra_args=( )
+
+    case "${tree}" in

support/scripts/fix-rpath line 75

Quoted 3 lines

support/scripts/fix-rpath new file +3 -0

+        host)
+            # do not process the sysroot (only contains target binaries)
+            find_args+=( "-name" "sysroot" "-prune" "-o" )

I believe we should specify a full path here. If there is any other directory
in the tree that happens to be called 'sysroot' for whatever reason, it shouldn't be pruned. When this is done directly from make, it's pretty easy:
-path $(STAGING_DIR) -prune

support/scripts/fix-rpath line 79

Quoted 5 lines

support/scripts/fix-rpath new file +4 -0

+
+            # do not process the external toolchain installation directory to
+            # to avoid breaking it.
+            find_args+=( "-path" "*/opt/ext-toolchain" "-prune" "-o" )

Same here: $(TOOLCHAIN_EXTERNAL_DOWNLOAD_INSTALL_DIR) which has the full path.

Regards,
Arnout

Quoted 10 lines

support/scripts/fix-rpath new file +10 -0

+
+            ;;
+        staging|target)
+            # discard ${hostdir}/lib and ${hostdir}/usr/lib
+            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+
+            ;;
+        *)
+            usage
+            exit 1

support/scripts/fix-rpath line 97

Quoted 8 lines

support/scripts/fix-rpath new file +8 -0

+            ;;
+    esac
+
+    find_args+=( "-type" "f" "-print" )
+
+    while read file ; do
+        # some files are not writable
+        chmod u+w $file

support/scripts/fix-rpath line 102

Quoted 5 lines

support/scripts/fix-rpath new file +5 -0

+        # call patchelf for each regular file; error will silently be ignored.
+        ${PATCHELF} --debug --make-rpath-relative "${basedir}" ${sanitize_extra_args[@]} "${file}" >> /dev/null 2>&1
+    done < <(find ${find_args[@]})
+
+    # ignore errors

From: Samuel Martin <[email protected]>

This patch adds host-patchelf to the list of package to build, and use

it in the SANITIZE_RPATH_HOST helper.

This SANITIZE_RPATH_HOST helper is executed in the world target, which ensure:
- to be built after the target-post-image target (because of the dependency);
- to always be built since it is the only dependency of the all (and default) target.

The SANITIZE_RPATH_HOST helper calls patchelf to fix the ELF files' RPATH from the HOST_DIR location (excluding the sysroot).

After running this helper is run, the RPATH from any host ELF files is relative to the binary location itself.

Notes:
- we avoid to fix RPATH in the sysroot.
- we do not try to fix RPATH in the external toolchain installation location as they may have been built in a way, this is already

correct; furthermore, attempting to fix RPATH in those programs may result in breaking them.
- the whole host directory is processed because a number of host-package install programs that could be useful in places different from $(HOST_DIR)/{bin,sbin,usr/bin,usr/sbin}.
- the shared libraries are also processed in case they have a 'main' function.

As a step toward a fully relocatable SDK, this change allows to get the toolchain relocatable, but not yet the whole SDK.

Signed-off-by Samuel Martin <[email protected]>

---
 Makefile | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Makefile +12 -0

index 0308f9b..54c815b 100644
@@ -543,7 +543,19 @@ $(BUILD_DIR)/buildroot-config/auto.conf: $(BR2_CONFIG)
 
 prepare: $(BUILD_DIR)/buildroot-config/auto.conf
 
+# RPATH fixing
+# - The host hook sets RPATH in host ELF binaries, using relative paths to the
+#   library locations.
+PACKAGES += host-patchelf
+
+define SANITIZE_RPATH_HOST
+	PATCHELF=$(HOST_DIR)/usr/bin/patchelf \
+	$(TOPDIR)/support/scripts/fix-rpath host $(HOST_DIR)
+endef
+
 world: target-post-image
+	@$(call MESSAGE,"Rendering the SDK relocatable")
+	$(SANITIZE_RPATH_HOST)
 
 .PHONY: all world toolchain dirs clean distclean source outputmakefile \
 	legal-info legal-info-prepare legal-info-clean printvars help \

It will create the script "relocate-toolchain.sh" in $HOST_DIR/usr/ allowing to adjust the path to the toolchain directory in all text files after it has been moved to a new location.

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/create-relocation-script | 72 ++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 support/scripts/create-relocation-script

support/scripts/create-relocation-script new file +7 -0

new file mode 100755
index 0000000..0ceaeb2
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+# Creates the script "relocate-toolchain.sh" and the "location" file in
+# the buildroot toolchain (host/usr). After copying that toolchain tree
+# to a new location, the script in the top directory should be executed
+# to relocate the toolchain. It actually will replace the string of the
+# old location with the new one in *all* text files.

support/scripts/create-relocation-script new file +20 -0

+#!/bin/sh
+#
+# Automatically generated by $0: don't edit
+#
+if [ "\$#" -ne 0 ]; then
+    echo "Run this script to relocate the buildroot toolchain at that location"
+    exit 1
+fi
+
+FILEPATH="\$(readlink -f \$0)"
+NEWPATH="\$(dirname \${FILEPATH})"
+
+cd \${NEWPATH}
+if [ ! -r ./${LOCATIONFILE} ]; then
+    echo "Previous location of the buildroot toolchain not found!"
+    exit 1
+fi
+OLDPATH="\$(cat ./${LOCATIONFILE})"
+
+if [ \${NEWPATH} = \${OLDPATH} ]; then

support/scripts/create-relocation-script new file +5 -0

+EOF
+chmod +x "${SCRIPTFILE}"
+mkdir -p $(dirname "$1/usr/${LOCATIONFILE}")
+# Finally write the toolchain location
+echo "$TOOLCHAINDIR" > "${TOOLCHAINDIR}/${LOCATIONFILE}"

Quoted 27 lines

It will create the script "relocate-toolchain.sh" in $HOST_DIR/usr/ allowing to adjust the path to the toolchain directory in all text files after it has been moved to a new location.

Signed-off-by Wolfgang Grandegger <[email protected]>

---
 support/scripts/create-relocation-script | 72 ++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 support/scripts/create-relocation-script

support/scripts/create-relocation-script new file +11 -0

new file mode 100755
index 0000000..0ceaeb2
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+# Creates the script "relocate-toolchain.sh" and the "location" file in
+# the buildroot toolchain (host/usr). After copying that toolchain tree
+# to a new location, the script in the top directory should be executed
+# to relocate the toolchain. It actually will replace the string of the
+# old location with the new one in *all* text files.
+
+if [ "$#" -ne 1 -o ! -d "$1" ]; then
+    echo "Usage: $0 <buildroot-host-directory-path>"
+    exit 1

support/scripts/create-relocation-script line 17

Quoted 6 lines

support/scripts/create-relocation-script new file +6 -0

+fi
+
+# The toolchain is in buildroot's "host/usr"
+TOOLCHAINDIR="$1/usr"
+# We create the script in the top directory of the toolchain
+SCRIPTFILE="${TOOLCHAINDIR}/relocate-toolchain.sh"

I think installing the script in host/usr/bin would be more appropriate.

Quoted 4 lines

support/scripts/create-relocation-script new file +4 -0

+# File holding the location of the buildroot toolchain
+LOCATIONFILE="share/buildroot/toolchain-location"
+
+echo "Creating ${SCRIPTFILE} for toolchain relocation ..."

support/scripts/create-relocation-script line 23

Quoted 2 lines

support/scripts/create-relocation-script new file +2 -0

+
+cat << EOF > "${SCRIPTFILE}"

Instead of using cat to instantiate the template, it's easier to understand
what happens by creating a template file (support/misc/relocate-toolchain.sh.in) and using sed to instantiate the variables that need to be instantiated. Such a sed command can typically be called directly from Makefile, without helper script.

However, as far as I can see, the only thing that is instantiated in this
script is LOCATIONFILE. But that variable is actually constant, so it could be coded directly in the script. So why not just $(INSTALL) the script itself, like is done for e.g. support/misc/target-dir-warning.txt ?

Quoted 20 lines

support/scripts/create-relocation-script new file +20 -0

+#!/bin/sh
+#
+# Automatically generated by $0: don't edit
+#
+if [ "\$#" -ne 0 ]; then
+    echo "Run this script to relocate the buildroot toolchain at that location"
+    exit 1
+fi
+
+FILEPATH="\$(readlink -f \$0)"
+NEWPATH="\$(dirname \${FILEPATH})"
+
+cd \${NEWPATH}
+if [ ! -r ./${LOCATIONFILE} ]; then
+    echo "Previous location of the buildroot toolchain not found!"
+    exit 1
+fi
+OLDPATH="\$(cat ./${LOCATIONFILE})"
+
+if [ \${NEWPATH} = \${OLDPATH} ]; then

support/scripts/create-relocation-script line 53

Quoted 10 lines

support/scripts/create-relocation-script new file +10 -0

+    echo "This buildroot toolchain has already been relocated!"
+    exit 0
+fi
+
+echo "Relocating the buildroot toolchain from \${OLDPATH} to \${NEWPATH} ..."
+
+# Make sure file uses the right language
+export LC_ALL=C
+# Replace the old path with the new one in all text files
+for FILE in \$(grep -r  "\${OLDPATH}"  . -l ) ; do

We typically collect options before the regex, so grep -r -l ...

I think that there are a few packages that might install something with spaces
(though probably not in $HOST_DIR), so find -exec is probably more appropriate. Unfortunately, that doesn't allow you to do a test with a pipe however so a different solution would be needed to support files with spaces.

support/scripts/create-relocation-script line 61

Quoted 9 lines

support/scripts/create-relocation-script new file +8 -0

+    if [ ! -h \${FILE} ] && [ -n  "\$(file -b --mime-type \${FILE} | grep '^text/' )" ] && [ "\${FILE}" != "./${LOCATIONFILE}" ]
+    then
+        sed -i s,"\${OLDPATH}",\${NEWPATH},g \${FILE}
+    fi
+done
+
+# Finally, we update the location file itself
+sed -i s,"\${OLDPATH}",\${NEWPATH},g ./${LOCATIONFILE}

Why do this separately? It works within the loop above as well, doesn't it?

support/scripts/create-relocation-script line 67

Quoted 6 lines

support/scripts/create-relocation-script new file +6 -0

+
+# Check if the path substitution did work properly
+if [ "\${NEWPATH}" != "\$(cat ./${LOCATIONFILE})" ]; then
+    echo "Something went wrong with substituting the path!"
+    echo "Please choose another location for your toolchain!"
+fi

Is there any reason why this error handling is needed? The only thing I can
imagine is concurrently running two instances of the script, but otherwise I see no way that the sed could fail...

Am 16.03.2017 um 18:51 schrieb Arnout Vandecappelle:

Quoted 1 line · Wolfgang Grandegger

Quoted 6 lines

support/scripts/create-relocation-script line 17

Quoted 4 lines

support/scripts/create-relocation-script new file +4 -0

+# The toolchain is in buildroot's "host/usr"
+TOOLCHAINDIR="$1/usr"
+# We create the script in the top directory of the toolchain
+SCRIPTFILE="${TOOLCHAINDIR}/relocate-toolchain.sh"

I think installing the script in host/usr/bin would be more appropriate.

I thought it's more visible/present in the root directory.

Quoted 4 lines

Quoted 4 lines

support/scripts/create-relocation-script new file +4 -0

+# File holding the location of the buildroot toolchain
+LOCATIONFILE="share/buildroot/toolchain-location"
+
+echo "Creating ${SCRIPTFILE} for toolchain relocation ..."

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+cat << EOF > "${SCRIPTFILE}"

Instead of using cat to instantiate the template, it's easier to understand
what happens by creating a template file (support/misc/relocate-toolchain.sh.in) and using sed to instantiate the variables that need to be instantiated. Such a sed command can typically be called directly from Makefile, without helper script.

I just see that you are looking to v1 of my RFC patch series. I have sent v2 yesterday.

In v2 I use the name "SDK" instead of "toolchain".
IIUC, the SDK is under "HOST_DIR" and the toolchain under "HOST_DIR/usr". And we want to relocate the SDK, right?

Quoted 4 lines

However, as far as I can see, the only thing that is instantiated in this
script is LOCATIONFILE. But that variable is actually constant, so it could be coded directly in the script. So why not just $(INSTALL) the script itself, like is done for e.g. support/misc/target-dir-warning.txt ?

Will adapt that solution. I was just not sure about the locations.

Quoted 4 lines

support/scripts/create-relocation-script line 61

Quoted 2 lines

support/scripts/create-relocation-script new file +2 -0

+# Finally, we update the location file itself
+sed -i s,"\${OLDPATH}",\${NEWPATH},g ./${LOCATIONFILE}

Why do this separately? It works within the loop above as well, doesn't it?

If the script is interrupted with ^C, you can through away the tree. If I update the location file at the end, just re-running the script will work.

Quoted 11 lines

support/scripts/create-relocation-script line 67

Quoted 7 lines

support/scripts/create-relocation-script new file +6 -0

+
+# Check if the path substitution did work properly
+if [ "\${NEWPATH}" != "\$(cat ./${LOCATIONFILE})" ]; then
+    echo "Something went wrong with substituting the path!"
+    echo "Please choose another location for your toolchain!"
+fi

Is there any reason why this error handling is needed? The only thing I can
imagine is concurrently running two instances of the script, but otherwise I see no way that the sed could fail...

If the path /a/b/c/a/b/c is copied to /a/b/c, relocation (substitution) will not be correct. Maybe too much paranoia.

Quoted 2 lines

Am 16.03.2017 um 18:51 schrieb Arnout Vandecappelle:

Quoted 1 line · Wolfgang Grandegger

[snip]

Quoted 7 lines

Quoted 6 lines

support/scripts/create-relocation-script line 17

Quoted 4 lines

support/scripts/create-relocation-script new file +4 -0

+# The toolchain is in buildroot's "host/usr"
+TOOLCHAINDIR="$1/usr"
+# We create the script in the top directory of the toolchain
+SCRIPTFILE="${TOOLCHAINDIR}/relocate-toolchain.sh"

I think installing the script in host/usr/bin would be more appropriate.

Quoted 1 line

I thought it's more visible/present in the root directory.

Well, the root directory would be host, not host/usr, right?

Quoted 13 lines

Quoted 12 lines

support/scripts/create-relocation-script line 23

Quoted 6 lines

support/scripts/create-relocation-script new file +6 -0

+# File holding the location of the buildroot toolchain
+LOCATIONFILE="share/buildroot/toolchain-location"
+
+echo "Creating ${SCRIPTFILE} for toolchain relocation ..."
+
+cat << EOF > "${SCRIPTFILE}"

Instead of using cat to instantiate the template, it's easier to understand
what happens by creating a template file (support/misc/relocate-toolchain.sh.in) and using sed to instantiate the variables that need to be instantiated. Such a sed command can typically be called directly from Makefile, without helper script.

Quoted 2 lines

I just see that you are looking to v1 of my RFC patch series. I have sent v2 yesterday.

Yes, I wrote this mail while offline and didn't see your new series until later.

Quoted 3 lines

In v2 I use the name "SDK" instead of "toolchain".
IIUC, the SDK is under "HOST_DIR" and the toolchain under "HOST_DIR/usr". And we want to relocate the SDK, right?

There is no clear distinction between SDK or toolchain - SDK is a little bit
more, it can contain other host tools e.g. genimage. The usr part is just historical accident that we are going to remove (when someone (i.e. me :-) finally finds the time to do it).

Anyway, for this series SDK is indeed a better term than toolchain.

Quoted 7 lines

Quoted 4 lines

However, as far as I can see, the only thing that is instantiated in this
script is LOCATIONFILE. But that variable is actually constant, so it could be coded directly in the script. So why not just $(INSTALL) the script itself, like is done for e.g. support/misc/target-dir-warning.txt ?

Will adapt that solution. I was just not sure about the locations.

[snip]

Quoted 7 lines

Quoted 4 lines

support/scripts/create-relocation-script line 61

Quoted 2 lines

support/scripts/create-relocation-script new file +2 -0

+# Finally, we update the location file itself
+sed -i s,"\${OLDPATH}",\${NEWPATH},g ./${LOCATIONFILE}

Why do this separately? It works within the loop above as well, doesn't it?

If the script is interrupted with ^C, you can through away the tree. If I update the location file at the end, just re-running the script will work.

That's a very good reason. Add a comment above it to explain.

Quoted 12 lines

Quoted 11 lines

support/scripts/create-relocation-script line 67

Quoted 7 lines

support/scripts/create-relocation-script new file +6 -0

+
+# Check if the path substitution did work properly
+if [ "\${NEWPATH}" != "\$(cat ./${LOCATIONFILE})" ]; then
+    echo "Something went wrong with substituting the path!"
+    echo "Please choose another location for your toolchain!"
+fi

Is there any reason why this error handling is needed? The only thing I can
imagine is concurrently running two instances of the script, but otherwise I see no way that the sed could fail...

Quoted 2 lines

If the path /a/b/c/a/b/c is copied to /a/b/c, relocation (substitution) will not be correct. Maybe too much paranoia.

No, a very good point actually! Maybe then a better approach is to first
generate the new location file (in a temporary new file), check that it looks OK, and don't continue if it is not OK.

Do add a comment to to explain the risk. You found this one instance where it
goes wrong, but there may be others as well.

Am 17.03.2017 um 16:50 schrieb Arnout Vandecappelle:

Quoted 3 lines · Wolfgang Grandegger

  # Replace the old path with the new one in all text files
  while read FILE ; do
      if file -b --mime-type "${FILE}" | grep -q '^text/' && [ "${FILE}" != "./usr/share/buildroot/sdk-location" ]
      then
          sed -i s,"${OLDPATH}","${NEWPATH}",g "${FILE}"
      fi;
  done < <(grep -lr "${OLDPATH}" .)

As grep -rl is very fast; I now use the following construct handling file with names with space corectly. BTW, is ',' is also a valid name character. Are all characters allowed for file names? Is there a known trick?

$ORIGIN/../../usr/bin is also a valid rpath for binaries in "$hostdir/usr/bin".

Hello,

Quoted 2 lines · Wolfgang Grandegger

$ORIGIN/../../usr/bin is also a valid rpath for binaries in "$hostdir/usr/bin".

I guess you meant $ORIGIN/../../usr/lib and $hostdir/usr/lib here, right?

Isn't this patch 8/9 actually a fix, which should come first in the series, and that we could/should apply quickly, independent from the rest?

Hello,

Am 03.03.2017 um 15:46 schrieb Thomas Petazzoni:

Quoted 8 lines

Hello,

Quoted 2 lines · Wolfgang Grandegger

$ORIGIN/../../usr/bin is also a valid rpath for binaries in "$hostdir/usr/bin".

I guess you meant $ORIGIN/../../usr/lib and $hostdir/usr/lib here, right?

"$ORIGIN/../../usr/lib" in ELF files in "$hostdir/usr/bin" *or* "$hostdir/usr/lib". IIRC, I think the problem was with some binaries in "usr/bin".

Quoted 3 lines

Isn't this patch 8/9 actually a fix, which should come first in the series, and that we could/should apply quickly, independent from the rest?

Yes! I got it after an initial build. Can't remember what package made the problem. Going to a fresh re-make...