diff mbox series

[2/2] toolchain: Get ld.so name if available

Message ID 1577937441-18703-2-git-send-email-han_mao@c-sky.com
State Changes Requested
Headers show
Series [1/2] package/toolchain-external: ensure ARCH_LIB_DIR exist | expand

Commit Message

=?UTF-8?B?5q+b5pmX?= Jan. 2, 2020, 3:57 a.m. UTC
RISC-V multilib toolchain(github.com/riscv/riscv-gnu-toolchain.git) put
multi ld.so with different ABI under sysroot/lib:
sysroot/lib/ld-linux-riscv32-ilp32d.so.1
sysroot/lib/ld-linux-riscv32-ilp32.so.1
sysroot/lib/ld-linux-riscv64-lp64d.so.1
sysroot/lib/ld-linux-riscv64-lp64.so.1
Current buildroot script can't handle multi ld.so and report:
>>> toolchain-external-custom  Copying external toolchain sysroot to staging...
/bin/bash: line 0: [: too many arguments
This patch try to get the exact name for ld.so and avoid multi ld.so check in
the script.

Signed-off-by: Qu Xianmiao <xianmiao_qu@c-sky.com>
Signed-off-by: Chen Hongdeng <hongdeng_chen@c-sky.com>
Signed-off-by: Guo Ren<ren_guo@c-sky.com>
Signed-off-by: Mao Han <han_mao@c-sky.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Mark Corbin <mark.corbin@embecosm.com>
---
 toolchain/helpers.mk                                   | 10 ++++++++--
 toolchain/toolchain-external/pkg-toolchain-external.mk |  3 ++-
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Yann E. MORIN Jan. 26, 2020, 3:31 p.m. UTC | #1
On 2020-01-02 11:57 +0800, Mao Han spake thusly:
> RISC-V multilib toolchain(github.com/riscv/riscv-gnu-toolchain.git) put
> multi ld.so with different ABI under sysroot/lib:
> sysroot/lib/ld-linux-riscv32-ilp32d.so.1
> sysroot/lib/ld-linux-riscv32-ilp32.so.1
> sysroot/lib/ld-linux-riscv64-lp64d.so.1
> sysroot/lib/ld-linux-riscv64-lp64.so.1
> Current buildroot script can't handle multi ld.so and report:
> >>> toolchain-external-custom  Copying external toolchain sysroot to staging...
> /bin/bash: line 0: [: too many arguments
> This patch try to get the exact name for ld.so and avoid multi ld.so check in
> the script.

I only had a cursory look at the rest of the thread, but if the last
comment from Thomas DS. is valid, then only this patch is required.

However, I have some feedback on it.

You need to provide a bit more explanations in the commit log. You
started by providing the problem you are facing, which is great, and you
explained why it happens, which is again very nice.

However, the fix you provided should be explained a bit more, because
this is touching the core infrastructure of Buildroot, and the code
touching the toolchain is always a bit hairy and fragile to manage...

> Signed-off-by: Qu Xianmiao <xianmiao_qu@c-sky.com>
> Signed-off-by: Chen Hongdeng <hongdeng_chen@c-sky.com>
> Signed-off-by: Guo Ren<ren_guo@c-sky.com>
> Signed-off-by: Mao Han <han_mao@c-sky.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Mark Corbin <mark.corbin@embecosm.com>
> ---
>  toolchain/helpers.mk                                   | 10 ++++++++--
>  toolchain/toolchain-external/pkg-toolchain-external.mk |  3 ++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 03355f5..cc581fc 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -104,6 +104,7 @@ copy_toolchain_sysroot = \
>  	ARCH_SUBDIR="$(strip $3)"; \
>  	ARCH_LIB_DIR="$(strip $4)" ; \
>  	SUPPORT_LIB_DIR="$(strip $5)" ; \
> +	SPECIFIC_LD_NAME="$(strip $6)" ; \
>  	for i in etc $${ARCH_LIB_DIR} sbin usr usr/$${ARCH_LIB_DIR}; do \
>  		if [ ! -d $${ARCH_SYSROOT_DIR}/$$i ] ; then \
>  			continue ; \
> @@ -136,8 +137,13 @@ copy_toolchain_sysroot = \
>  		done ; \
>  	fi ; \
>  	if [ ! -e $(STAGING_DIR)/lib/ld*.so.* ]; then \
> -		if [ -e $${ARCH_SYSROOT_DIR}/lib/ld*.so.* ]; then \
> -			cp -a $${ARCH_SYSROOT_DIR}/lib/ld*.so.* $(STAGING_DIR)/lib/ ; \
> +		if [ "$${SPECIFIC_LD_NAME}" != "" ]; then \
> +			LD_NAME=$${SPECIFIC_LD_NAME}; \
> +		else \
> +			LD_NAME="ld*.so.*"; \
> +		fi; \
> +		if [ -e $${ARCH_SYSROOT_DIR}/lib/$${LD_NAME} ]; then \
> +			cp -a $${ARCH_SYSROOT_DIR}/lib/$${LD_NAME} $(STAGING_DIR)/lib/ ; \

Since we now have the actual interpreter name, as extracted by the
caller, we should be able to greatly simplify this conditional block,
no?

But see below...

>  		fi ; \
>  	fi ; \
>  	if [ `readlink -f $${SYSROOT_DIR}` != `readlink -f $${ARCH_SYSROOT_DIR}` ] ; then \
> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> index 13b2468..19cf7d6 100644
> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> @@ -425,6 +425,7 @@ define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
>  	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
>  	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
>  	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> +	SPECIFIC_LD_NAME=`$(TOOLCHAIN_EXTERNAL_READELF) -d $${ARCH_SYSROOT_DIR}$${ARCH_LIB_DIR}/libc*.so|grep ld|grep so|awk '{print $$NF}'|sed "s/\[//g"|sed "s/\]//g"`;\

As far as I understand this, you are trying to extract the interpreter.
This is overly complex, and can be done with a single sed invocation:

    $(TOOLCHAIN_EXTERNAL_READELF) -l $${ARCH_SYSROOT_DIR}$${ARCH_LIB_DIR}/libc*.so \
    |sed -r -e '/Requesting program interpreter/!d; s:^.*/([^[:space:]]+)\]$$:\1:'

But then, we should probably do that for the internal toolchain, too?
This way we can actually cimplify the conditional block, above.

Regards,
Yann E. MORIN.

>  	SUPPORT_LIB_DIR="" ; \
>  	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
>  		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
> @@ -441,7 +442,7 @@ define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
>  		ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \
>  	fi ; \
>  	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
> -	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR})
> +	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR},$${SPECIFIC_LD_NAME})
>  endef
>  
>  # Create a symlink from (usr/)$(ARCH_LIB_DIR) to lib.
> -- 
> 2.7.4
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas De Schampheleire Feb. 5, 2020, 10:30 a.m. UTC | #2
Hi Mao Han,

El jue., 2 ene. 2020 a las 5:15, Mao Han (<han_mao@c-sky.com>) escribió:
>
> RISC-V multilib toolchain(github.com/riscv/riscv-gnu-toolchain.git) put
> multi ld.so with different ABI under sysroot/lib:
> sysroot/lib/ld-linux-riscv32-ilp32d.so.1
> sysroot/lib/ld-linux-riscv32-ilp32.so.1
> sysroot/lib/ld-linux-riscv64-lp64d.so.1
> sysroot/lib/ld-linux-riscv64-lp64.so.1
> Current buildroot script can't handle multi ld.so and report:
> >>> toolchain-external-custom  Copying external toolchain sysroot to staging...
> /bin/bash: line 0: [: too many arguments
> This patch try to get the exact name for ld.so and avoid multi ld.so check in
> the script.
>
> Signed-off-by: Qu Xianmiao <xianmiao_qu@c-sky.com>
> Signed-off-by: Chen Hongdeng <hongdeng_chen@c-sky.com>
> Signed-off-by: Guo Ren<ren_guo@c-sky.com>
> Signed-off-by: Mao Han <han_mao@c-sky.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Mark Corbin <mark.corbin@embecosm.com>
> ---
>  toolchain/helpers.mk                                   | 10 ++++++++--
>  toolchain/toolchain-external/pkg-toolchain-external.mk |  3 ++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 03355f5..cc581fc 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -104,6 +104,7 @@ copy_toolchain_sysroot = \
>         ARCH_SUBDIR="$(strip $3)"; \
>         ARCH_LIB_DIR="$(strip $4)" ; \
>         SUPPORT_LIB_DIR="$(strip $5)" ; \
> +       SPECIFIC_LD_NAME="$(strip $6)" ; \
>         for i in etc $${ARCH_LIB_DIR} sbin usr usr/$${ARCH_LIB_DIR}; do \
>                 if [ ! -d $${ARCH_SYSROOT_DIR}/$$i ] ; then \
>                         continue ; \
> @@ -136,8 +137,13 @@ copy_toolchain_sysroot = \
>                 done ; \
>         fi ; \
>         if [ ! -e $(STAGING_DIR)/lib/ld*.so.* ]; then \
> -               if [ -e $${ARCH_SYSROOT_DIR}/lib/ld*.so.* ]; then \
> -                       cp -a $${ARCH_SYSROOT_DIR}/lib/ld*.so.* $(STAGING_DIR)/lib/ ; \
> +               if [ "$${SPECIFIC_LD_NAME}" != "" ]; then \
> +                       LD_NAME=$${SPECIFIC_LD_NAME}; \
> +               else \
> +                       LD_NAME="ld*.so.*"; \
> +               fi; \

In addition to the comments posted previously by Yann E. Morin, I
wonder if there can actually be a case where SPECIFIC_LD_NAME is
empty. What I mean is: do we still need the wildcard-based approach if
we are using a more correct approach of actually checking the
interpreter via readelf?
Removing the wildcards would make the code a bit more clear. In this
case, the name 'SPECIFIC_LD_NAME' could perhaps be renamed to just
'LD_SO_FILENAME' or something.

Best regards,
Thomas
diff mbox series

Patch

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index 03355f5..cc581fc 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -104,6 +104,7 @@  copy_toolchain_sysroot = \
 	ARCH_SUBDIR="$(strip $3)"; \
 	ARCH_LIB_DIR="$(strip $4)" ; \
 	SUPPORT_LIB_DIR="$(strip $5)" ; \
+	SPECIFIC_LD_NAME="$(strip $6)" ; \
 	for i in etc $${ARCH_LIB_DIR} sbin usr usr/$${ARCH_LIB_DIR}; do \
 		if [ ! -d $${ARCH_SYSROOT_DIR}/$$i ] ; then \
 			continue ; \
@@ -136,8 +137,13 @@  copy_toolchain_sysroot = \
 		done ; \
 	fi ; \
 	if [ ! -e $(STAGING_DIR)/lib/ld*.so.* ]; then \
-		if [ -e $${ARCH_SYSROOT_DIR}/lib/ld*.so.* ]; then \
-			cp -a $${ARCH_SYSROOT_DIR}/lib/ld*.so.* $(STAGING_DIR)/lib/ ; \
+		if [ "$${SPECIFIC_LD_NAME}" != "" ]; then \
+			LD_NAME=$${SPECIFIC_LD_NAME}; \
+		else \
+			LD_NAME="ld*.so.*"; \
+		fi; \
+		if [ -e $${ARCH_SYSROOT_DIR}/lib/$${LD_NAME} ]; then \
+			cp -a $${ARCH_SYSROOT_DIR}/lib/$${LD_NAME} $(STAGING_DIR)/lib/ ; \
 		fi ; \
 	fi ; \
 	if [ `readlink -f $${SYSROOT_DIR}` != `readlink -f $${ARCH_SYSROOT_DIR}` ] ; then \
diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
index 13b2468..19cf7d6 100644
--- a/toolchain/toolchain-external/pkg-toolchain-external.mk
+++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
@@ -425,6 +425,7 @@  define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
 	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
 	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
 	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
+	SPECIFIC_LD_NAME=`$(TOOLCHAIN_EXTERNAL_READELF) -d $${ARCH_SYSROOT_DIR}$${ARCH_LIB_DIR}/libc*.so|grep ld|grep so|awk '{print $$NF}'|sed "s/\[//g"|sed "s/\]//g"`;\
 	SUPPORT_LIB_DIR="" ; \
 	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
 		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
@@ -441,7 +442,7 @@  define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
 		ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \
 	fi ; \
 	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
-	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR})
+	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR},$${SPECIFIC_LD_NAME})
 endef
 
 # Create a symlink from (usr/)$(ARCH_LIB_DIR) to lib.