[v2,1/2] Strip binaries in the rootfs creation instead of in target-finalize
diff mbox series

Message ID 20191001013654.22298-2-unixmania@gmail.com
State New
Headers show
Series
  • Make remote debugging easier
Related show

Commit Message

Carlos Santos Oct. 1, 2019, 1:36 a.m. UTC
From: Carlos Santos <unixmania@gmail.com>

Since commit 118534fe54 the root filesystem image is generated from a
temporary copy of TARGET_DIR, so we can strip the binaries in the copy,
only.

This allows us to easily find the non-stripped executables to debug with
gdbserver, as they are at the same relative path in TARGET_DIR as in the
target device, rather than searching inside the build directory.

Fixes: https://bugs.busybox.net/show_bug.cgi?id=10386

Signed-off-by: Carlos Santos <unixmania@gmail.com>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
CC: Ciro Santilli <ciro.santilli@gmail.com>
---
Changes v1->v2:
- Strip before running the fakeroot script, as suggested by Arnout
  Vandecappelle
- Change commit message accordingly. Removed paragraph about setting
  sysroot to TARGET_DIR in gdb, which is done in the next commit.
---
 Makefile     | 34 ----------------------------------
 fs/common.mk | 36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 35 deletions(-)

Comments

Thomas Petazzoni Oct. 1, 2019, 6:52 a.m. UTC | #1
On Mon, 30 Sep 2019 22:36:53 -0300
unixmania@gmail.com wrote:

> From: Carlos Santos <unixmania@gmail.com>
> 
> Since commit 118534fe54 the root filesystem image is generated from a
> temporary copy of TARGET_DIR, so we can strip the binaries in the copy,
> only.
> 
> This allows us to easily find the non-stripped executables to debug with
> gdbserver, as they are at the same relative path in TARGET_DIR as in the
> target device, rather than searching inside the build directory.
> 
> Fixes: https://bugs.busybox.net/show_bug.cgi?id=10386
> 
> Signed-off-by: Carlos Santos <unixmania@gmail.com>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

I see Arnout reviewed-by this, and while I understand the goal, I am
not sure this is the real direction/solution we want for this problem.
I believe the real solution we have been talking about for a long time
is to install everything in STAGING_DIR (not just libraries). This way,
unstripped binaries with debugging symbols with all be in STAGING_DIR.

To me, the change being proposed here makes TARGET_DIR really different
from what ends up in the final rootfs image. While we admittely already
have a few differences, I personally like to keep the idea of
TARGET_DIR being really what's on the target root filesystem, with as
few differences as possible.

Thomas
Carlos Santos Oct. 1, 2019, 11:26 a.m. UTC | #2
On Tue, Oct 1, 2019 at 3:52 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 30 Sep 2019 22:36:53 -0300
> unixmania@gmail.com wrote:
>
> > From: Carlos Santos <unixmania@gmail.com>
> >
> > Since commit 118534fe54 the root filesystem image is generated from a
> > temporary copy of TARGET_DIR, so we can strip the binaries in the copy,
> > only.
> >
> > This allows us to easily find the non-stripped executables to debug with
> > gdbserver, as they are at the same relative path in TARGET_DIR as in the
> > target device, rather than searching inside the build directory.
> >
> > Fixes: https://bugs.busybox.net/show_bug.cgi?id=10386
> >
> > Signed-off-by: Carlos Santos <unixmania@gmail.com>
> > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>
> I see Arnout reviewed-by this, and while I understand the goal, I am
> not sure this is the real direction/solution we want for this problem.
> I believe the real solution we have been talking about for a long time
> is to install everything in STAGING_DIR (not just libraries). This way,
> unstripped binaries with debugging symbols with all be in STAGING_DIR.
>
> To me, the change being proposed here makes TARGET_DIR really different
> from what ends up in the final rootfs image. While we admittely already
> have a few differences, I personally like to keep the idea of
> TARGET_DIR being really what's on the target root filesystem, with as
> few differences as possible.

We could make it optional, keeping the the current behavior by default.
Yann E. MORIN Oct. 1, 2019, 8:15 p.m. UTC | #3
Thomas, Carlos, Arnout, All,

On 2019-10-01 08:52 +0200, Thomas Petazzoni spake thusly:
> On Mon, 30 Sep 2019 22:36:53 -0300
> unixmania@gmail.com wrote:
> 
> > From: Carlos Santos <unixmania@gmail.com>
> > 
> > Since commit 118534fe54 the root filesystem image is generated from a
> > temporary copy of TARGET_DIR, so we can strip the binaries in the copy,
> > only.
> > 
> > This allows us to easily find the non-stripped executables to debug with
> > gdbserver, as they are at the same relative path in TARGET_DIR as in the
> > target device, rather than searching inside the build directory.
> > 
> > Fixes: https://bugs.busybox.net/show_bug.cgi?id=10386
> > 
> > Signed-off-by: Carlos Santos <unixmania@gmail.com>
> > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> 
> I see Arnout reviewed-by this, and while I understand the goal, I am
> not sure this is the real direction/solution we want for this problem.
> I believe the real solution we have been talking about for a long time
> is to install everything in STAGING_DIR (not just libraries). This way,
> unstripped binaries with debugging symbols with all be in STAGING_DIR.

I agree with Thomas here.

The dichotomy between staging/ and target/ has no raison-d'ĂȘtre nowadays.

The idea would be to ensure that all packages install in staging/,
always. Then, identify packages that have different commands for target/
and staging/ and make them identical. Finally, remove the target install
commands, and genreate it as a copy of staging at the beginning of the
target-finalize step.

Yes, the real patches will be a bit more gory than the ideal view above.
But except for some special snowflakes (qt5, I'm looking at you), it
should be fairly straghtforward...

> To me, the change being proposed here makes TARGET_DIR really different
> from what ends up in the final rootfs image. While we admittely already
> have a few differences, I personally like to keep the idea of
> TARGET_DIR being really what's on the target root filesystem, with as
> few differences as possible.

I beg to slightly disagree here, and with a much more controversial view
on the topic: we should just get rid of target/ altogether. The only
valid artefacts we generates are the images in images/

But that one is definitely not on the table. ;-)

Regards,
Yann E. MORIN.

> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index ecaae13846..57314e5f1d 100644
--- a/Makefile
+++ b/Makefile
@@ -614,38 +614,6 @@  RSYNC_VCS_EXCLUSIONS = \
 	--exclude .svn --exclude .git --exclude .hg --exclude .bzr \
 	--exclude CVS
 
-# When stripping, obey to BR2_STRIP_EXCLUDE_DIRS and
-# BR2_STRIP_EXCLUDE_FILES
-STRIP_FIND_COMMON_CMD = \
-	find $(TARGET_DIR) \
-	$(if $(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS)), \
-		\( $(call finddirclauses,$(TARGET_DIR),$(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS))) \) \
-		-prune -o \
-	) \
-	$(if $(call qstrip,$(BR2_STRIP_EXCLUDE_FILES)), \
-		-not \( $(call findfileclauses,$(call qstrip,$(BR2_STRIP_EXCLUDE_FILES))) \) )
-
-# Regular stripping for everything, except libpthread, ld-*.so and
-# kernel modules:
-# - libpthread.so: a non-stripped libpthread shared library is needed for
-#   proper debugging of pthread programs using gdb.
-# - ld.so: a non-stripped dynamic linker library is needed for valgrind
-# - kernel modules (*.ko): do not function properly when stripped like normal
-#   applications and libraries. Normally kernel modules are already excluded
-#   by the executable permission check, so the explicit exclusion is only
-#   done for kernel modules with incorrect permissions.
-STRIP_FIND_CMD = \
-	$(STRIP_FIND_COMMON_CMD) \
-	-type f \( -perm /111 -o -name '*.so*' \) \
-	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
-	-print0
-
-# Special stripping (only debugging symbols) for libpthread and ld-*.so.
-STRIP_FIND_SPECIAL_LIBS_CMD = \
-	$(STRIP_FIND_COMMON_CMD) \
-	\( -name 'ld-*.so*' -o -name 'libpthread*.so*' \) \
-	-print0
-
 ifeq ($(BR2_ECLIPSE_REGISTER),y)
 define TOOLCHAIN_ECLIPSE_REGISTER
 	./support/scripts/eclipse-register-toolchain `readlink -f $(O)` \
@@ -761,8 +729,6 @@  endif
 	rm -rf $(TARGET_DIR)/usr/doc $(TARGET_DIR)/usr/share/doc
 	rm -rf $(TARGET_DIR)/usr/share/gtk-doc
 	rmdir $(TARGET_DIR)/usr/share 2>/dev/null || true
-	$(STRIP_FIND_CMD) | xargs -0 $(STRIPCMD) 2>/dev/null || true
-	$(STRIP_FIND_SPECIAL_LIBS_CMD) | xargs -0 -r $(STRIPCMD) $(STRIP_STRIP_DEBUG) 2>/dev/null || true
 
 	test -f $(TARGET_DIR)/etc/ld.so.conf && \
 		{ echo "ERROR: we shouldn't have a /etc/ld.so.conf file"; exit 1; } || true
diff --git a/fs/common.mk b/fs/common.mk
index 842ea924a5..caa7825cbb 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -61,6 +61,38 @@  ROOTFS_COMMON_FINAL_RECURSIVE_DEPENDENCIES = $(sort \
 	) \
 	$(ROOTFS_COMMON_FINAL_RECURSIVE_DEPENDENCIES__X))
 
+# When stripping, obey to BR2_STRIP_EXCLUDE_DIRS and
+# BR2_STRIP_EXCLUDE_FILES
+STRIP_FIND_COMMON_CMD = \
+	find $(TARGET_DIR) \
+	$(if $(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS)), \
+		\( $(call finddirclauses,$(TARGET_DIR),$(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS))) \) \
+		-prune -o \
+	) \
+	$(if $(call qstrip,$(BR2_STRIP_EXCLUDE_FILES)), \
+		-not \( $(call findfileclauses,$(call qstrip,$(BR2_STRIP_EXCLUDE_FILES))) \) )
+
+# Regular stripping for everything, except libpthread, ld-*.so and
+# kernel modules:
+# - libpthread.so: a non-stripped libpthread shared library is needed for
+#   proper debugging of pthread programs using gdb.
+# - ld.so: a non-stripped dynamic linker library is needed for valgrind
+# - kernel modules (*.ko): do not function properly when stripped like normal
+#   applications and libraries. Normally kernel modules are already excluded
+#   by the executable permission check, so the explicit exclusion is only
+#   done for kernel modules with incorrect permissions.
+STRIP_FIND_CMD = \
+	$(STRIP_FIND_COMMON_CMD) \
+	-type f \( -perm /111 -o -name '*.so*' \) \
+	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
+	-print0
+
+# Special stripping (only debugging symbols) for libpthread and ld-*.so.
+STRIP_FIND_SPECIAL_LIBS_CMD = \
+	$(STRIP_FIND_COMMON_CMD) \
+	\( -name 'ld-*.so*' -o -name 'libpthread*.so*' \) \
+	-print0
+
 .PHONY: rootfs-common
 rootfs-common: $(ROOTFS_COMMON_DEPENDENCIES) target-finalize
 	@$(call MESSAGE,"Generating root filesystems common tables")
@@ -157,9 +189,11 @@  $$(BINARIES_DIR)/$$(ROOTFS_$(2)_FINAL_IMAGE_NAME): $$(ROOTFS_$(2)_DEPENDENCIES)
 		$$(BASE_TARGET_DIR)/ \
 		$$(TARGET_DIR)
 
+	$$(STRIP_FIND_CMD) | xargs -0 $$(STRIPCMD) 2>/dev/null || true
+	$$(STRIP_FIND_SPECIAL_LIBS_CMD) | xargs -0 -r $$(STRIPCMD) $$(STRIP_STRIP_DEBUG) 2>/dev/null || true
+
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
 	echo "set -e" >> $$(FAKEROOT_SCRIPT)
-
 	echo "chown -h -R 0:0 $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 	PATH=$$(BR_PATH) $$(TOPDIR)/support/scripts/mkusers $$(ROOTFS_FULL_USERS_TABLE) $$(TARGET_DIR) >> $$(FAKEROOT_SCRIPT)
 	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_FULL_DEVICES_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)