[Buildroot] [RFC PATCH 0/9] Make the buildroot toolchain relocatable

[Buildroot] [RFC PATCH 0/9] Make the buildroot toolchain relocatable

[Wolfgang Grandegger]

Hello,

this is a RFC patch series to make the buildroot toolchain relocatable.
It's based on Samuel's related patch series but it now uses a patched
version of patchelf to sanitize the RPATH in the ELF files of the "target",
"sysroot" and "host" tree. Actually "patchelf --make-rpath-relative" does
the job. The idea is to get that patch mainlined but first I would like to
get some feedback here.

Furthermore it creates the script "relocate-toolchain.sh" in "host/usr"
allowing to relocate the toolchain after it has been moved to a new
location. It replaces the old path with the new one in all text files
identified by "file --mime-type".

Things not yet addressed:

- "make toolchain" creates a toolchain tree which still has references
  to the build system (in ELF and text files).  

Wolfgang.

Samuel Martin (3):
  support/scripts: add fix-rpath script to sanitize the rpath
  core: sanitize HOST_DIR at the very end of the build
  core: add {TARGET, STAGING}_SANITIZE_RPATH_HOOK to
    TARGET_FINALIZE_HOOKS

Wolfgang Grandegger (6):
  package/patchelf: use a recent version and add "--make-rpath-relative"
    patch
  support/scripts: add create-relocation-script for toolchain relocation
  core: create relocate-toolchain.sh in HOST_DIR/usr at the end of the
    build
  external-toolchain: check if a buildroot toolchain has already been
    relocated
  support/scripts: check-host-rpath now handles $ORIGIN as well
  support/scripts: check-host-rpath now uses patchelf to get the rpath

 Makefile                                           |  33 +++
 ...to-make-the-rpath-relative-within-a-speci.patch | 313 +++++++++++++++++++++
 package/patchelf/patchelf.hash                     |   2 +-
 package/patchelf/patchelf.mk                       |   6 +-
 support/scripts/check-host-rpath                   |  14 +-
 support/scripts/create-relocation-script           |  72 +++++
 support/scripts/fix-rpath                          | 106 +++++++
 toolchain/helpers.mk                               |  14 +
 .../toolchain-external/pkg-toolchain-external.mk   |   1 +
 9 files changed, 547 insertions(+), 14 deletions(-)
 create mode 100644 package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
 create mode 100755 support/scripts/create-relocation-script
 create mode 100755 support/scripts/fix-rpath

-- 
1.9.1

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Wolfgang Grandegger]

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

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 ...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

diff --git a/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
new file mode 100644
index 0000000..44b1742
--- /dev/null
+++ b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
@@ -0,0 +1,313 @@
+From cc9f15bb80cf36b4a88e10000c3b1c1989b51197 Mon Sep 17 00:00:00 2001
+From: Wolfgang Grandegger <wg@grandegger.com>
+Date: Mon, 20 Feb 2017 16:29:24 +0100
+Subject: [PATCH] Add option to make the rpath relative within a specified root
+ directory
+
+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.
+
+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.
+
+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.
+
+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.
+
+RPATHDIR points somewhere else:
+    (can be anywhere: build trees, staging tree, host location,
+    non-existing location, etc.). Just discard such a path.
+
+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.
+
+[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;
+ 
+ typedef std::shared_ptr<std::vector<unsigned char>> FileContents;
+ 
++#define MODIFY_FLAG_NO_STD_LIB_DIRS 0x1
++static int modifyFlags;
+ 
+ #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);
+ 
+-    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);
+ 
+     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)) {
++		    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;
++}
+ 
+ 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)
+ {
+     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;
++    }
+ 
+     /* For each directory in the RPATH, check if it contains any
+        needed library. */
+     if (op == rpShrink) {
+-        std::vector<bool> neededLibFound(neededLibs.size(), false);
+ 
+         newRPath = "";
+ 
+@@ -1177,30 +1242,78 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
+                 continue;
+             }
+ 
+-            /* 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)) {
+-                            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;
+-                    }
+-                }
+-
+-            if (!libFound)
++            if (!libFoundInRPath(dirName, neededLibs))
+                 debug("removing directory '%s' from RPATH\n", dirName.c_str());
+             else
+                 concatToRPath(newRPath, dirName);
+         }
+     }
+ 
++    /* 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;
++            std::string path;
++
++            /* 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;
++                }
++            }
++
++            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;
++	    }
++
++            /* Finally make "canonicalPath" relative to "filedir" in "rootDir" */
++            concatToRPath(newRPath, makePathRelative(canonicalPath, fileDir, rootDir));
++	    debug("keeping relative path of %s", canonicalPath.c_str());
++        }
++    }
++
+     if (op == rpRemove) {
+         if (!rpath) {
+             debug("no RPATH to delete\n");
+@@ -1528,7 +1641,9 @@ static std::vector<std::string> allowedRpathPrefixes;
+ static bool removeRPath = false;
+ static bool setRPath = false;
+ static bool printRPath = false;
++static bool makeRPathRelative = false;
+ static std::string newRPath;
++static std::string rootDir;
+ static std::set<std::string> neededLibsToRemove;
+ static std::map<std::string, std::string> neededLibsToReplace;
+ static std::set<std::string> neededLibsToAdd;
+@@ -1551,14 +1666,16 @@ static void patchElf2(ElfFile && elfFile)
+         elfFile.setInterpreter(newInterpreter);
+ 
+     if (printRPath)
+-        elfFile.modifyRPath(elfFile.rpPrint, {}, "");
++        elfFile.modifyRPath(elfFile.rpPrint, {}, {}, modifyFlags, "");
+ 
+     if (shrinkRPath)
+-        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "");
++        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "", modifyFlags, "");
+     else if (removeRPath)
+-        elfFile.modifyRPath(elfFile.rpRemove, {}, "");
++        elfFile.modifyRPath(elfFile.rpRemove, {}, "", modifyFlags, "");
+     else if (setRPath)
+-        elfFile.modifyRPath(elfFile.rpSet, {}, newRPath);
++        elfFile.modifyRPath(elfFile.rpSet, {}, "", modifyFlags, newRPath);
++    else if (makeRPathRelative)
++        elfFile.modifyRPath(elfFile.rpMakeRelative, {}, rootDir, modifyFlags, "");
+ 
+     if (printNeeded) elfFile.printNeededLibs();
+ 
+@@ -1604,6 +1721,8 @@ void showHelp(const std::string & progName)
+   [--remove-rpath]\n\
+   [--shrink-rpath]\n\
+   [--allowed-rpath-prefixes PREFIXES]\t\tWith '--shrink-rpath', reject rpath entries not starting with the allowed prefix\n\
++  [--make-rpath-relative ROOTDIR]\n\
++  [--no-standard-lib-dirs]\n\
+   [--print-rpath]\n\
+   [--force-rpath]\n\
+   [--add-needed LIBRARY]\n\
+@@ -1664,6 +1783,14 @@ int mainWrapped(int argc, char * * argv)
+             setRPath = true;
+             newRPath = argv[i];
+         }
++        else if (arg == "--make-rpath-relative") {
++            if (++i == argc) error("missing argument to --make-rpath-relative");
++            makeRPathRelative = true;
++            rootDir = argv[i];
++        }
++        else if (arg == "--no-standard-lib-dirs") {
++            modifyFlags |= MODIFY_FLAG_NO_STD_LIB_DIRS;
++        }
+         else if (arg == "--print-rpath") {
+             printRPath = true;
+         }
+-- 
+1.9.1
+
diff --git a/package/patchelf/patchelf.hash b/package/patchelf/patchelf.hash
index 653eb46..7188b2e 100644
--- a/package/patchelf/patchelf.hash
+++ b/package/patchelf/patchelf.hash
@@ -1,2 +1,2 @@
 # Locally calculated
-sha256	a0f65c1ba148890e9f2f7823f4bedf7ecad5417772f64f994004f59a39014f83	patchelf-0.9.tar.bz2
+sha256	d90cbedb2efa516b2373640560aa6b37f0e29c4967611c3312981e30dbbab966  patchelf-c1f89c077e44a495c62ed0dcfaeca21510df93ef.tar.gz
diff --git a/package/patchelf/patchelf.mk b/package/patchelf/patchelf.mk
index cf2e43a..c880222 100644
--- a/package/patchelf/patchelf.mk
+++ b/package/patchelf/patchelf.mk
@@ -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
 
-- 
1.9.1

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

From: Samuel Martin <s.martin49@gmail.com>

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 <s.martin49@gmail.com>
Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 support/scripts/fix-rpath

diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
new file mode 100755
index 0000000..bde2c17
--- /dev/null
+++ b/support/scripts/fix-rpath
@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+
+# Copyright (C) 2016 Samuel Martin <s.martin49@gmail.com>
+#
+# 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.
+#
+# 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
+
+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
+        host)
+            # do not process the sysroot (only contains target binaries)
+            find_args+=( "-name" "sysroot" "-prune" "-o" )
+
+            # do not process the external toolchain installation directory to
+            # to avoid breaking it.
+            find_args+=( "-path" "*/opt/ext-toolchain" "-prune" "-o" )
+
+            ;;
+        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 ${@}
-- 
1.9.1

[Buildroot] [RFC PATCH 3/9] core: sanitize HOST_DIR at the very end of the build

[Wolfgang Grandegger]

From: Samuel Martin <s.martin49@gmail.com>

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 <s.martin49@gmail.com>
---
 Makefile | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Makefile b/Makefile
index 0308f9b..54c815b 100644
--- a/Makefile
+++ b/Makefile
@@ -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 \
-- 
1.9.1

[Buildroot] [RFC PATCH 4/9] core: add {TARGET, STAGING}_SANITIZE_RPATH_HOOK to TARGET_FINALIZE_HOOKS

[Wolfgang Grandegger]

From: Samuel Martin <s.martin49@gmail.com>

This patch introduces the TARGET_SANITIZE_RPATH_HOOK and
STAGING_SANITIZE_RPATH_HOOK hooks fixing the ELF files' RPATH of
binaries from, respectively, the TARGET_DIR and the STAGING_DIR
locations.

It is a fair assumption that all target package has been built before
reaching the target-finalize target; hence the execution of these hooks
as TARGET_FINALIZE_HOOKS.

After running these hooks, the RPATH from any target ELF files from both
the target and the staging locations won't contain any occurrence of the
sysroot or some build locations.

Signed-off-by: Samuel Martin <s.martin49@gmail.com>
---
 Makefile | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Makefile b/Makefile
index 54c815b..a68c9ce 100644
--- a/Makefile
+++ b/Makefile
@@ -661,6 +661,22 @@ endef
 TARGET_FINALIZE_HOOKS += PURGE_LOCALES
 endif
 
+# Function sanitizing target/staging ELF files' RPATH.
+# i.e. it removes paths pointing to the staging or build location from the ELF
+# files' RPATH.
+define TARGET_SANITIZE_RPATH_HOOK
+	PATCHELF=$(HOST_DIR)/usr/bin/patchelf \
+	$(TOPDIR)/support/scripts/fix-rpath target $(TARGET_DIR)
+endef
+
+define STAGING_SANITIZE_RPATH_HOOK
+	PATCHELF=$(HOST_DIR)/usr/bin/patchelf \
+	$(TOPDIR)/support/scripts/fix-rpath staging $(STAGING_DIR)
+endef
+
+TARGET_FINALIZE_HOOKS += TARGET_SANITIZE_RPATH_HOOK \
+       STAGING_SANITIZE_RPATH_HOOK
+
 $(TARGETS_ROOTFS): target-finalize
 
 target-finalize: $(PACKAGES)
-- 
1.9.1

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Wolfgang Grandegger]

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 <wg@grandegger.com>
---
 support/scripts/create-relocation-script | 72 ++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100755 support/scripts/create-relocation-script

diff --git a/support/scripts/create-relocation-script b/support/scripts/create-relocation-script
new file mode 100755
index 0000000..0ceaeb2
--- /dev/null
+++ b/support/scripts/create-relocation-script
@@ -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
+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"
+# File holding the location of the buildroot toolchain
+LOCATIONFILE="share/buildroot/toolchain-location"
+
+echo "Creating ${SCRIPTFILE} for toolchain relocation ..."
+
+cat << EOF > "${SCRIPTFILE}"
+#!/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
+    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
+    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}
+
+# 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
+EOF
+chmod +x "${SCRIPTFILE}"
+mkdir -p $(dirname "$1/usr/${LOCATIONFILE}")
+# Finally write the toolchain location
+echo "$TOOLCHAINDIR" > "${TOOLCHAINDIR}/${LOCATIONFILE}"
-- 
1.9.1

[Buildroot] [RFC PATCH 6/9] core: create relocate-toolchain.sh in HOST_DIR/usr at the end of the build

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index a68c9ce..afa7516 100644
--- a/Makefile
+++ b/Makefile
@@ -553,9 +553,14 @@ define SANITIZE_RPATH_HOST
 	$(TOPDIR)/support/scripts/fix-rpath host $(HOST_DIR)
 endef
 
+define CREATE_RELOCATION_SCRIPT
+	$(TOPDIR)/support/scripts/create-relocation-script $(HOST_DIR)
+endef
+
 world: target-post-image
 	@$(call MESSAGE,"Rendering the SDK relocatable")
 	$(SANITIZE_RPATH_HOST)
+	$(CREATE_RELOCATION_SCRIPT)
 
 .PHONY: all world toolchain dirs clean distclean source outputmakefile \
 	legal-info legal-info-prepare legal-info-clean printvars help \
-- 
1.9.1

[Buildroot] [RFC PATCH 7/9] external-toolchain: check if a buildroot toolchain has already been relocated

[Wolfgang Grandegger]

The location of the buildroot toolchain is stored in the file
"share/buildroot/toolchain-location". If it's contents does not match
the current toolchain location, ask the user to run the script
"relocate-toolchain.sh" in the top directory once.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 toolchain/helpers.mk                                   | 14 ++++++++++++++
 toolchain/toolchain-external/pkg-toolchain-external.mk |  1 +
 2 files changed, 15 insertions(+)

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index 72e7292..d037391 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -403,3 +403,17 @@ check_toolchain_ssp = \
 gen_gdbinit_file = \
 	mkdir -p $(STAGING_DIR)/usr/share/buildroot/ ; \
 	echo "set sysroot $(STAGING_DIR)" > $(STAGING_DIR)/usr/share/buildroot/gdbinit
+
+#
+# Check if it's a buildroot toolchain and if it's already relocatable by
+# reading and testing the toolchain location file
+#
+# $1: toolchain installation directory
+#
+check_buildroot_toolchain_relocated = \
+	if [ -r $(1)/share/buildroot/toolchain-location ]; then \
+		if [ "`cat $(1)/share/buildroot/toolchain-location`" != "$(1)" ]; then \
+			echo "Please relocate the buildroot toolchain at \"$(1)\" by executing \"$(1)/reloacte-toolchain.sh\" once!" ; \
+			exit 1 ; \
+		fi \
+	fi
diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
index 11a1bf5..1cc7922 100644
--- a/toolchain/toolchain-external/pkg-toolchain-external.mk
+++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
@@ -543,6 +543,7 @@ endif
 # matches the configuration provided in Buildroot: ABI, C++ support,
 # kernel headers version, type of C library and all C library features.
 define $(2)_CONFIGURE_CMDS
+	$$(Q)$$(call check_buildroot_toolchain_relocated,$$(TOOLCHAIN_EXTERNAL_INSTALL_DIR))
 	$$(Q)$$(call check_cross_compiler_exists,$$(TOOLCHAIN_EXTERNAL_CC))
 	$$(Q)$$(call check_unusable_toolchain,$$(TOOLCHAIN_EXTERNAL_CC))
 	$$(Q)SYSROOT_DIR="$$(call toolchain_find_sysroot,$$(TOOLCHAIN_EXTERNAL_CC))" ; \
-- 
1.9.1

[Buildroot] [RFC PATCH 8/9] support/scripts: check-host-rpath now handles $ORIGIN as well

[Wolfgang Grandegger]

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

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 support/scripts/check-host-rpath | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
index 6ce547c..020c123 100755
--- a/support/scripts/check-host-rpath
+++ b/support/scripts/check-host-rpath
@@ -58,7 +58,7 @@ check_elf_has_rpath() {
         for dir in ${rpath//:/ }; do
             # Remove duplicate and trailing '/' for proper match
             dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
-            [ "${dir}" = "${hostdir}/usr/lib" ] && return 0
+            [ "${dir}" = "${hostdir}/usr/lib" -o "${dir}" = "\$ORIGIN/../../usr/lib" ] && return 0
         done
     done < <( readelf -d "${file}"                                              \
               |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
-- 
1.9.1

[Buildroot] [RFC PATCH 9/9] support/scripts: check-host-rpath now uses patchelf to get the rpath

[Wolfgang Grandegger]

"patchelf --print-rpath" returns the rpath without using regex's.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 support/scripts/check-host-rpath | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
index 020c123..2847f40 100755
--- a/support/scripts/check-host-rpath
+++ b/support/scripts/check-host-rpath
@@ -5,7 +5,7 @@
 # there.
 
 # Override the user's locale so we are sure we can parse the output of
-# readelf(1) and file(1)
+# file(1)
 export LC_ALL=C
 
 main() {
@@ -41,10 +41,7 @@ elf_needs_rpath() {
 
     while read lib; do
         [ -e "${hostdir}/usr/lib/${lib}" ] && return 0
-    done < <( readelf -d "${file}"                                         \
-              |sed -r -e '/^.* \(NEEDED\) .*Shared library: \[(.+)\]$/!d;' \
-                     -e 's//\1/;'                                          \
-            )
+    done < <( patchelf --print-needed "${file}" )
 
     return 1
 }
@@ -60,10 +57,7 @@ check_elf_has_rpath() {
             dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
             [ "${dir}" = "${hostdir}/usr/lib" -o "${dir}" = "\$ORIGIN/../../usr/lib" ] && return 0
         done
-    done < <( readelf -d "${file}"                                              \
-              |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \
-                      -e 's//\3/;'                                              \
-            )
+    done < <( patchelf --print-rpath "${file}" )
 
     return 1
 }
-- 
1.9.1

[Buildroot] [RFC PATCH 8/9] support/scripts: check-host-rpath now handles $ORIGIN as well

[Thomas Petazzoni]

Hello,

On Fri,  3 Mar 2017 15:18:52 +0100, Wolfgang Grandegger wrote:
> $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?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Thomas Petazzoni]

Hello,

On Fri,  3 Mar 2017 15:18:46 +0100, Wolfgang Grandegger wrote:
> From: Samuel Martin <s.martin49@gmail.com>
> 
> 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 <s.martin49@gmail.com>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  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?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [RFC PATCH 8/9] support/scripts: check-host-rpath now handles $ORIGIN as well

[Wolfgang Grandegger]

Hello,

Am 03.03.2017 um 15:46 schrieb Thomas Petazzoni:
> Hello,
>
> On Fri,  3 Mar 2017 15:18:52 +0100, Wolfgang Grandegger wrote:
>> $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".

> 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...

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Hello,

Am 03.03.2017 um 15:48 schrieb Thomas Petazzoni:
> Hello,
>
> On Fri,  3 Mar 2017 15:18:46 +0100, Wolfgang Grandegger wrote:
>> From: Samuel Martin <s.martin49@gmail.com>
>>
>> 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 <s.martin49@gmail.com>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  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?

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

Wolfgang.

[Buildroot] [RFC PATCH 8/9] support/scripts: check-host-rpath now handles $ORIGIN as well

[Wolfgang Grandegger]

Hello,

Am 03.03.2017 um 15:56 schrieb Wolfgang Grandegger:
> Hello,
>
> Am 03.03.2017 um 15:46 schrieb Thomas Petazzoni:
>> Hello,
>>
>> On Fri,  3 Mar 2017 15:18:52 +0100, Wolfgang Grandegger wrote:
>>> $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".
>
>> 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...

That's not correct. At the *second* make I get:

*** ERROR: package host-libxml-parser-perl installs executables without 
proper RPATH:
***   /work/agco/output/dcu_host/host/usr/bin/gtester
***   /work/agco/output/dcu_host/host/usr/bin/xgettext
***   /work/agco/output/dcu_host/host/usr/bin/msgexec
***   /work/agco/output/dcu_host/host/usr/bin/msggrep
***   /work/agco/output/dcu_host/host/usr/bin/msgcat
***   /work/agco/output/dcu_host/host/usr/bin/toe
***   /work/agco/output/dcu_host/host/usr/bin/msgconv
***   /work/agco/output/dcu_host/host/usr/bin/infocmp

In my case, this is due to the sanitized rpath from the previous make. 
With my config there is no ELF file with an initial rpath 
"$ORIGIN/../../usr/lib". Well, this check is not necessary at all after 
rpath sanitation. I'm going to remove it in my patch series.

Wolfgang.

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Arnout Vandecappelle]

 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.

On 03-03-17 15:18, Wolfgang Grandegger wrote:
> The patch allows to use patchelf to sanitize the rpath of the buildroot
> libraries and binaries.
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  ...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
> 
> diff --git a/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
> new file mode 100644
> index 0000000..44b1742
> --- /dev/null
> +++ b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
> @@ -0,0 +1,313 @@
> +From cc9f15bb80cf36b4a88e10000c3b1c1989b51197 Mon Sep 17 00:00:00 2001
> +From: Wolfgang Grandegger <wg@grandegger.com>
> +Date: Mon, 20 Feb 2017 16:29:24 +0100
> +Subject: [PATCH] Add option to make the rpath relative within a specified root
> + directory
> +
> +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.

> +
> +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.

> +
> +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).


> +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).

> +
> +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.

> +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.

> +
> +[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;
> + 
> + 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.

> + 
> + #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);
> + 
> +-    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.

> + 
> +     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)) {
> ++		    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.

> + 
> + 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)
> + {
> +     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]
> + 
> ++    /* 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().

> ++            std::string path;

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

> ++
> ++            /* 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;
> ++                }
> ++            }

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

> ++
> ++            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.

> ++
> ++            /* 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.

> ++        }
> ++    }

 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]
> diff --git a/package/patchelf/patchelf.hash b/package/patchelf/patchelf.hash
> index 653eb46..7188b2e 100644
> --- a/package/patchelf/patchelf.hash
> +++ b/package/patchelf/patchelf.hash
> @@ -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.

 Regards,
 Arnout

> diff --git a/package/patchelf/patchelf.mk b/package/patchelf/patchelf.mk
> index cf2e43a..c880222 100644
> --- a/package/patchelf/patchelf.mk
> +++ b/package/patchelf/patchelf.mk
> @@ -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
>  
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Arnout Vandecappelle]

 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.

On 03-03-17 15:18, Wolfgang Grandegger wrote:
> From: Samuel Martin <s.martin49@gmail.com>
> 
> 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 <s.martin49@gmail.com>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---
>  support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100755 support/scripts/fix-rpath
> 
> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
> new file mode 100755
> index 0000000..bde2c17
> --- /dev/null
> +++ b/support/scripts/fix-rpath
> @@ -0,0 +1,106 @@
> +#!/usr/bin/env bash
> +
> +# Copyright (C) 2016 Samuel Martin <s.martin49@gmail.com>
> +#
> +# 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.
> +#
> +# 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.

> +
> +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
> +        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


> +
> +            # 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

> +
> +            ;;
> +        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 ${@}
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 3/9] core: sanitize HOST_DIR at the very end of the build

[Arnout Vandecappelle]

On 03-03-17 15:18, Wolfgang Grandegger wrote:
> From: Samuel Martin <s.martin49@gmail.com>
> 
> This patch adds host-patchelf to the list of package to build, and use
                                               packages              uses

> 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.

 I don't think it makes sense to put this in a separate SANITIZE_RPATH_HOST
variable; just put it directly in the rule.

 Also, I think it fits better as part of the target-post-image rule itself. I
like it that world is a dependency-only target, it feels cleaner :-) But that's
just bikeshedding of course.

> 
> 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
                                                   that

>   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.

 And shared libraries can have their own RPATH, no?

> 
> 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 <s.martin49@gmail.com>

 Wolfgang, you forgot to add your Sob.

 Regards,
 Arnout

> ---
>  Makefile | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 0308f9b..54c815b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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 \
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Wolfgang Grandegger]

Hello Arnout,

Am 04.03.2017 um 12:26 schrieb Arnout Vandecappelle:
>  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.

OK. The idea was to get the patch mainlined first. That's the reason
why I implemented it with the most recent version. If we think the
patch does what we want, I would start the mainlining process. I will split
and update the description.

> On 03-03-17 15:18, Wolfgang Grandegger wrote:
>> The patch allows to use patchelf to sanitize the rpath of the buildroot
>> libraries and binaries.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  ...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
>>
>> diff --git a/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
>> new file mode 100644
>> index 0000000..44b1742
>> --- /dev/null
>> +++ b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-within-a-speci.patch
>> @@ -0,0 +1,313 @@
>> +From cc9f15bb80cf36b4a88e10000c3b1c1989b51197 Mon Sep 17 00:00:00 2001
>> +From: Wolfgang Grandegger <wg@grandegger.com>
>> +Date: Mon, 20 Feb 2017 16:29:24 +0100
>> +Subject: [PATCH] Add option to make the rpath relative within a specified root
>> + directory
>> +
>> +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.

OK.

>> +
>> +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.

It does check if the path exists and if it's within the root tree. If
not; it get's dropped. I will update the description.

>> +
>> +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).

Yep.

>> +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).

No, for example pulseaudio uses the following target RPATH dirs:

ROOTDIR/usr/lib/pulseaudio
ROOTDIR/usr/lib/pulse-9.0/modules

>> +
>> +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.

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.

>> +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.

>> +
>> +[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;
>> + 
>> + 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.

For the moment yes, but I thought it may hold other flags, e.g. to
suppress checking the rpath dir against the needed libraries.

>> + 
>> + #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);
>> + 
>> +-    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.

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

>> + 
>> +     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)) {
>> ++		    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.

I agree.

>> + 
>> + 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)
>> + {
>> +     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.

OK.

> [snip]
>> + 
>> ++    /* 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.

> 
>> ++            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.

> 
>> ++
>> ++            /* 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;
>> ++                }
>> ++            }
> 
>  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.

> 
>> ++
>> ++            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.

I should remove all tabs, I know :(.

> 
>> ++
>> ++            /* 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.
> 
>> ++        }
>> ++    }
> 
>  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.

> 
>  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?

> 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.

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.

>  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?

> [snip]
>> diff --git a/package/patchelf/patchelf.hash b/package/patchelf/patchelf.hash
>> index 653eb46..7188b2e 100644
>> --- a/package/patchelf/patchelf.hash
>> +++ b/package/patchelf/patchelf.hash
>> @@ -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.

OK.

Thanks for reviewing.

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Hello Arnout,

Am 04.03.2017 um 12:39 schrieb Arnout Vandecappelle:
>  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.

If the logic is not too complex.

>
>  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.

I think the mailing-list is stripping the CC. On the mail I got, Samuel 
is on CC. I did use "git send-email" with an "-cc" to Samuel.

> On 03-03-17 15:18, Wolfgang Grandegger wrote:
>> From: Samuel Martin <s.martin49@gmail.com>
>>
>> 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 <s.martin49@gmail.com>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  support/scripts/fix-rpath | 106 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>>  create mode 100755 support/scripts/fix-rpath
>>
>> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
>> new file mode 100755
>> index 0000000..bde2c17
>> --- /dev/null
>> +++ b/support/scripts/fix-rpath
>> @@ -0,0 +1,106 @@
>> +#!/usr/bin/env bash
>> +
>> +# Copyright (C) 2016 Samuel Martin <s.martin49@gmail.com>
>> +#
>> +# 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.
>> +#
>> +# 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.

I agree!

>> +
>> +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
>> +        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

OK.

>
>> +
>> +            # 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.

OK.

>
>  Regards,
>  Arnout
>
>> +
>> +            ;;
>> +        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

This is just a hack because some elf files are not writable. To make all 
regular file writable is wrong. We may also want to reset the original 
permission. It needs more logic, unfortunately.

>> +        # 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

Simply ignoring errors here is fast, but it also does not allow to print 
warnings.

Wolfgang.

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Arnout Vandecappelle]

On 04-03-17 16:16, Wolfgang Grandegger wrote:
> Hello Arnout,
> 
> Am 04.03.2017 um 12:26 schrieb Arnout Vandecappelle:
[snip]
>>> +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.
> 
> 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.


>>> +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.

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

[snip]
>>> ++    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]
>>> ++    /* 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 /".

> 
>>
>>> ++            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.
> 
>>
>>> ++
>>> ++            /* 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;
>>> ++                }
>>> ++            }
>>
>>  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]
>>  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.

 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.


>>  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.


>> 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.

>>
>> 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.
> 
>>  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.


 Regards,
 Arnout

[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Wolfgang Grandegger]

Am 06.03.2017 um 00:13 schrieb Arnout Vandecappelle:
> 
> 
> On 04-03-17 16:16, Wolfgang Grandegger wrote:
>> Hello Arnout,
>>
>> Am 04.03.2017 um 12:26 schrieb Arnout Vandecappelle:
> [snip]
>>>> +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.
>>
>> 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.
> 
> 
>>>> +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.
> 
>  Even if both option are part of the --make-rpath-relative bit, it makes sense
> to treat them in two separate commits.
> 
> [snip]
>>>> ++    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.

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.

>  Note that forbiddenRpathPrefixes is actually wrong, since it shouldn't be
> prefixes, it should be full paths. So forbiddenRpaths.
> 
> [snip]
>>>> ++    /* 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 /".
> 
>>
>>>
>>>> ++            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.
>>
>>>
>>>> ++
>>>> ++            /* 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;
>>>> ++                }
>>>> ++            }
>>>
>>>  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]
>>>  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.
> 
>  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.
> 
> 
>>>  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.
> 
> 
>>> 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.
> 
>>>
>>> 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.
>>
>>>  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.

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...

Wolfgang. 

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Hello,

Am 03.03.2017 um 17:39 schrieb Wolfgang Grandegger:
> Hello,
> 
> Am 03.03.2017 um 15:48 schrieb Thomas Petazzoni:
>> Hello,
>>
>> On Fri,  3 Mar 2017 15:18:46 +0100, Wolfgang Grandegger wrote:
>>> From: Samuel Martin <s.martin49@gmail.com>
>>>
>>> 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 <s.martin49@gmail.com>
>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>>> ---
>>>  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?
> 
> Well, I think three magic "find" oneliners could do the job. I will try
> and see.

Here are my current oneliners in the Makefile. It mainly shows what needs
to be done. Unfortunately, the scanning for ELF files may take rather
long:

 >>>   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 {}; \
	/opt/bdo/x86_host/host/usr/bin/patchelf --make-rpath-relative /opt/bdo/x86_host/target \
	--no-standard-lib-dirs {}; fi"

  real	0m9.644s
  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 \
	"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

  >>>   Rendering the SDK relocatable
  cp /opt/bdo/x86_host/host/usr/bin/patchelf /opt/bdo/x86_host/host/usr/bin/patchelf.__copy__
  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


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.

Because some ELF files are not writeable, we need a chmod first. 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?
We could also introduce BR2_TOOLCHAIN_RELOCATABLE. Any other idea to
speed up the scripts above?

Wolfgang.

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Arnout Vandecappelle]

On 06-03-17 09:42, Wolfgang Grandegger wrote:
> Am 06.03.2017 um 00:13 schrieb Arnout Vandecappelle:

[snip]
>>  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 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]

> 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.

> 
> 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).


> 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?

> - 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...

 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.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 1/9] package/patchelf: use a recent version and add "--make-rpath-relative" patch

[Wolfgang Grandegger]

Am 06.03.2017 um 13:40 schrieb Arnout Vandecappelle:
>
>
> On 06-03-17 09:42, Wolfgang Grandegger wrote:
>> Am 06.03.2017 um 00:13 schrieb Arnout Vandecappelle:
>
> [snip]
>>>  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 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.

You are right. I was just wondering why we keep "$HOST_DIR/usr/lib/" as 
"$ORIGIN/../../../usr/lib but not a similar rpath to the directory above.

> [snip]
>
>> 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.
>
>>
>> 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).
>
>
>> 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?

Well... I thought it must match exactly but that's not the case:

     for (auto & i : allowedPrefixes)
         if (!s.compare(0, i.size(), i)) return true;

But we need the rootdir for other purposes as well.

>> - 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...
>
>  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.

OK, sounds like a plan.

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Am 07.03.2017 um 00:49 schrieb Arnout Vandecappelle:
> 
> 
> On 06-03-17 10:07, Wolfgang Grandegger wrote:
>> Hello,
>>
>> Am 03.03.2017 um 17:39 schrieb Wolfgang Grandegger:
>>> Hello,
>>>
>>> Am 03.03.2017 um 15:48 schrieb Thomas Petazzoni:
>>>> Hello,
>>>>
>>>> On Fri,  3 Mar 2017 15:18:46 +0100, Wolfgang Grandegger wrote:
>>>>> From: Samuel Martin <s.martin49@gmail.com>
>>>>>
>>>>> 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 <s.martin49@gmail.com>
>>>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>>>>> ---
>>>>>  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?
>>>
>>> Well, I think three magic "find" oneliners could do the job. I will try
>>> and see.
>>
>> 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.

+1

>> Unfortunately, the scanning for ELF files may take rather
>> long:
>>
>>  >>>   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 '{}' ';'

OK, it' two times faster, indeed

>> 	/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:

  $ 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
 
>  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.

>>   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 \)".


  $ 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".

>  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).

>> 	"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
>>
>>   >>>   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?

We cannot update the "patchelf" binary while it's in use. 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]

"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".
Running patchelf with "LD_DEBUG" tells me that it will take the libraries
from the host (/usr/lib). Just wondering if that's correct!?

> 
>>   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
>>
>>
>> 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.

> 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.

>>
>> 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.

>> 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.

Using -P8 on my i7 quad-core laptop is almost 8 times faster.

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Am 07.03.2017 um 18:40 schrieb Arnout Vandecappelle:
>
>
> On 07-03-17 10:13, Wolfgang Grandegger wrote:
>> Am 07.03.2017 um 00:49 schrieb Arnout Vandecappelle:
>>>
>>>
>>> On 06-03-17 10:07, Wolfgang Grandegger wrote:
>>>> Hello,
>>>>
> [snip]
>>>>  >>>   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 '{}' ';'
>>
>> 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.

"-P" is very effective... but that's fine tuning.

>>>> 	/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:
>>
>>   $ 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?

It's a config including QT5... also doubling the build time .

$ find sysroot/usr/lib/qt/ -type f| wc -l
3355
$ find sysroot/usr/include/qt5/ -type f| wc -l
4168

>>>  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.
>>
>>>>   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 \)".
>>
>>
>>   $ 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 :-)

Yep.

>>>  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).
>>
>>>> 	"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
>>>>
>>>>   >>>   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?
>>
>> We cannot update the "patchelf" binary while it's in use.
>
>  Yes, but that's avoided with the -name patchelf -prune bit.

Yep.

>> 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]
>>
>> "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/".

>> 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.

>  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:

Shouldn't "host/usr/x86_64-buildroot-linux-gnu/lib64" not treated in a 
similar way? Just wondering! I have attached the debug output of 
patchelf for the host tree.

> - 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.

OK.

>>>>   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
>>>>
>>>>
>>>> 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.
>>
>>> 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.

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.

>>>> 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.

Good idea!

>  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.

I think restoring the permission is the correct solution.

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Arnout Vandecappelle]

On 08-03-17 10:25, Wolfgang Grandegger wrote:
> Am 07.03.2017 um 18:40 schrieb Arnout Vandecappelle:
>>
>>
[snip]
>>  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]
>>> "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.


> 
>>> 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]
>>  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. 

 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%.

> 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?

 Regards,
 Arnout


[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Am 12.03.2017 um 21:53 schrieb Arnout Vandecappelle:
>
>
> On 08-03-17 10:25, Wolfgang Grandegger wrote:
>> Am 07.03.2017 um 18:40 schrieb Arnout Vandecappelle:
>>>
>>>
> [snip]
>>>  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]
>>>> "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.
>
>
>>
>>>> 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]
>>>  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.
>
>  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.

>
>> 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?

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.

Wolfgang.

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Arnout Vandecappelle]

On 12-03-17 22:10, Wolfgang Grandegger wrote:
> Am 12.03.2017 um 21:53 schrieb Arnout Vandecappelle:
>>
>>
[snip]
>>  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.

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

> 
>>
>>> 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?
> 
> 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.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 2/9] support/scripts: add fix-rpath script to sanitize the rpath

[Wolfgang Grandegger]

Hello Arnout,

Am 13.03.2017 um 18:08 schrieb Arnout Vandecappelle:
>
>
> On 12-03-17 22:10, Wolfgang Grandegger wrote:
>> Am 12.03.2017 um 21:53 schrieb Arnout Vandecappelle:
>>>
>>>
> [snip]
>>>  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.
>
>  I think I had a bigger difference, but anyway even 20% isn't really worth it,
> probably.

Well, I just learned that it's very difficult to do reproducible 
measurements with my laptop due to frequency scaling, boost, 
temperature, etc. :(.

>>
>>>
>>>> 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?
>>
>> 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.

OK, will do with a CC to this list. More soon...

Wolfgang.

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Arnout Vandecappelle]

On 03-03-17 15:18, Wolfgang Grandegger wrote:
> 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 <wg@grandegger.com>
> ---
>  support/scripts/create-relocation-script | 72 ++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100755 support/scripts/create-relocation-script
> 
> diff --git a/support/scripts/create-relocation-script b/support/scripts/create-relocation-script
> new file mode 100755
> index 0000000..0ceaeb2
> --- /dev/null
> +++ b/support/scripts/create-relocation-script
> @@ -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
> +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.

> +# 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.

 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 ?


> +#!/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
> +    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.

> +    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?
> +
> +# 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...

 Regards,
 Arnout

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

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Wolfgang Grandegger]

Hello,

Am 16.03.2017 um 18:51 schrieb Arnout Vandecappelle:
>
>
> On 03-03-17 15:18, Wolfgang Grandegger wrote:
>> 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 <wg@grandegger.com>
>> ---
>>  support/scripts/create-relocation-script | 72 ++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>  create mode 100755 support/scripts/create-relocation-script
>>
>> diff --git a/support/scripts/create-relocation-script b/support/scripts/create-relocation-script
>> new file mode 100755
>> index 0000000..0ceaeb2
>> --- /dev/null
>> +++ b/support/scripts/create-relocation-script
>> @@ -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
>> +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.

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

>> +# 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.

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?

>
>  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.

>> +#!/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
>> +    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.

Yes, these magic spaces in file names. I will try using "find ... -exec".

>
>> +    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?

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.

>> +
>> +# 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.

Wolfgang.

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Arnout Vandecappelle]

On 17-03-17 08:10, Wolfgang Grandegger wrote:
> Am 16.03.2017 um 18:51 schrieb Arnout Vandecappelle:
>> On 03-03-17 15:18, Wolfgang Grandegger wrote:
[snip]
>>> +# 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.

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

>>> +# 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.
> 
> 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.


> 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.

>>  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]
>>> +# 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.

> 
>>> +
>>> +# 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.

 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.

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Wolfgang Grandegger]

Am 17.03.2017 um 16:50 schrieb Arnout Vandecappelle:
> 
> 
> On 17-03-17 08:10, Wolfgang Grandegger wrote:
>> Am 16.03.2017 um 18:51 schrieb Arnout Vandecappelle:
>>> On 03-03-17 15:18, Wolfgang Grandegger wrote:
> [snip]
>>>> +# 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.
> 
>  Well, the root directory would be host, not host/usr, right?

Of course!

>>>> +# 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.
>>
>> 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.

No problem. At the very first moment, I thought I sent the old series.

>> 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.
> 
>>>  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]


  # 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?

>>>> +# 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.

OK.

>>
>>>> +
>>>> +# 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.
> 
>  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.

+1

>  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.

OK.

Wolfgang.

[Buildroot] [RFC PATCH 5/9] support/scripts: add create-relocation-script for toolchain relocation

[Arnout Vandecappelle]

On 17-03-17 18:15, Wolfgang Grandegger wrote:
> Am 17.03.2017 um 16:50 schrieb Arnout Vandecappelle:
>>
>>
>> On 17-03-17 08:10, Wolfgang Grandegger wrote:
[snip]
>   # 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?

 All characters except / and \0 are allowed in file names. Look at [1] for a
long rant (and a number of good approaches) about this subject. Note that our
scripts don't have to be portable, so we can use e.g. find -print0. But your
proposal is pretty good, just needs 'read -r' insteade.

 So OLDPATH or NEWPATH could indeed contain a comma. You can check for that at
the beginning and error out.

 Regards,
 Arnout

[1] https://www.dwheeler.com/essays/filenames-in-shell.html

[snip]

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF