[Buildroot] [PATCHv2 0/4] EABI vs EABIhf check

[Buildroot] [PATCHv2 0/4] EABI vs EABIhf check

[Thomas Petazzoni]

Hello,

Second version of the EABI vs. EABIhf check, using a different
technique, as discussed with Yann E. Morin, and also additional
cleanups.

I've also included a somewhat unrelated fix to the arch/Config.in.arm
file.

Thomas

Thomas Petazzoni (4):
  toolchain/helpers: don't use the x$(...) = x"value" syntax
  toolchain: check ARM EABI vs. EABIhf for external toolchains
  toolchain/toolchain-external: don't use x$(...) construct or ==
  arch/arm: update VFPv2 comment to mention ARMv5

 arch/Config.in.arm                       |  4 ++--
 toolchain/helpers.mk                     | 21 ++++++++++++++++++---
 toolchain/toolchain-external/ext-tool.mk | 13 ++++++++-----
 3 files changed, 28 insertions(+), 10 deletions(-)

-- 
1.8.1.2

[Buildroot] [PATCHv2 1/4] toolchain/helpers: don't use the x$(...) = x"value" syntax

[Thomas Petazzoni]

As noted by Yann E. Morin, the x$(...) = x"value" syntax is old and
ugly, and the easier to read "$(...)" = "value" can now be used
without problems.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/helpers.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index 95217e7..94fe015 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -181,7 +181,7 @@ create_lib64_symlinks = \
 # $2: feature description
 #
 check_glibc_feature = \
-	if [ x$($(1)) != x"y" ] ; then \
+	if [ "$($(1))" != "y" ] ; then \
 		echo "$(2) available in C library, please enable $(1)" ; \
 		exit 1 ; \
 	fi
@@ -236,11 +236,11 @@ check_glibc = \
 #
 check_uclibc_feature = \
 	IS_IN_LIBC=`grep -q "\#define $(1) 1" $(3) && echo y` ; \
-	if [ x$($(2)) != x"y" -a x$${IS_IN_LIBC} = x"y" ] ; then \
+	if [ "$($(2))" != "y" -a $${IS_IN_LIBC} = "y" ] ; then \
 		echo "$(4) available in C library, please enable $(2)" ; \
 		exit 1 ; \
 	fi ; \
-	if [ x$($(2)) = x"y" -a x$${IS_IN_LIBC} != x"y" ] ; then \
+	if [ "$($(2))" = "y" -a $${IS_IN_LIBC} != "y" ] ; then \
 		echo "$(4) not available in C library, please disable $(2)" ; \
 		exit 1 ; \
 	fi
-- 
1.8.1.2

[Buildroot] [PATCHv2 1/4] toolchain/helpers: don't use the x$(...) = x"value" syntax

[Yann E. MORIN]

Thomas, All,

On 2013-07-17 10:13 +0200, Thomas Petazzoni spake thusly:
> As noted by Yann E. Morin, the x$(...) = x"value" syntax is old and
> ugly, and the easier to read "$(...)" = "value" can now be used
> without problems.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  toolchain/helpers.mk | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 95217e7..94fe015 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -181,7 +181,7 @@ create_lib64_symlinks = \
>  # $2: feature description
>  #
>  check_glibc_feature = \
> -	if [ x$($(1)) != x"y" ] ; then \
> +	if [ "$($(1))" != "y" ] ; then \
>  		echo "$(2) available in C library, please enable $(1)" ; \
>  		exit 1 ; \
>  	fi
> @@ -236,11 +236,11 @@ check_glibc = \
>  #
>  check_uclibc_feature = \
>  	IS_IN_LIBC=`grep -q "\#define $(1) 1" $(3) && echo y` ; \
> -	if [ x$($(2)) != x"y" -a x$${IS_IN_LIBC} = x"y" ] ; then \
> +	if [ "$($(2))" != "y" -a $${IS_IN_LIBC} = "y" ] ; then \
				 ^^^^^^^^^^^^^^
You'd have to quote this, too, since it can be the empty string.

>  		echo "$(4) available in C library, please enable $(2)" ; \
>  		exit 1 ; \
>  	fi ; \
> -	if [ x$($(2)) = x"y" -a x$${IS_IN_LIBC} != x"y" ] ; then \
> +	if [ "$($(2))" = "y" -a $${IS_IN_LIBC} != "y" ] ; then \
				^^^^^^^^^^^^^^
Ditto.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCHv2 2/4] toolchain: check ARM EABI vs. EABIhf for external toolchains

[Thomas Petazzoni]

Following the introduction of the support of EABIhf as a second ARM
ABI, it is important to check whether the external toolchain provided
by the user actually uses the ABI that has been selected in the
Buildroot configuration. This commit introduces such a check by
looking at the 'Tag_ABI_VFP_args' tag of the architecture-specific
section of the ELF headers. This assumes that ELF is the binary format
used on ARM, which may not be the case on ARM noMMU systems (they use
the FLAT binary format), but Buildroot doesn't have support for such
systems at the moment.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/helpers.mk                     | 15 +++++++++++++++
 toolchain/toolchain-external/ext-tool.mk |  5 ++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index 94fe015..a8dab90 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -279,10 +279,25 @@ check_uclibc = \
 #
 check_arm_abi = \
 	__CROSS_CC=$(strip $1) ; \
+	__CROSS_READELF=$(strip $2) ; \
 	EXT_TOOLCHAIN_TARGET=`LANG=C $${__CROSS_CC} -v 2>&1 | grep ^Target | cut -f2 -d ' '` ; \
 	if ! echo $${EXT_TOOLCHAIN_TARGET} | grep -qE 'eabi(hf)?$$' ; then \
 		echo "External toolchain uses the unsuported OABI" ; \
 		exit 1 ; \
+	fi ; \
+	EXT_TOOLCHAIN_CRT1=`LANG=C $${__CROSS_CC} -print-file-name=crt1.o` ; \
+	if $${__CROSS_READELF} -A $${EXT_TOOLCHAIN_CRT1} | grep -q "Tag_ABI_VFP_args:" ; then \
+		EXT_TOOLCHAIN_ABI="eabihf" ; \
+	else \
+		EXT_TOOLCHAIN_ABI="eabi" ; \
+	fi ; \
+	if [ "$(BR2_ARM_EABI)" = "y" -a $${EXT_TOOLCHAIN_ABI} = "eabihf" ] ; then \
+		echo "Incorrect ABI setting: EABI selected, but toolchain uses EABIhf" ; \
+		exit 1 ; \
+	fi ; \
+	if [ "$(BR2_ARM_EABIHF)" = "y" -a $${EXT_TOOLCHAIN_ABI} = "eabi" ] ; then \
+		echo "Incorrect ABI setting: EABIhf selected, but toolchain uses EABI" ; \
+		exit 1 ; \
 	fi
 
 #
diff --git a/toolchain/toolchain-external/ext-tool.mk b/toolchain/toolchain-external/ext-tool.mk
index b9ae68f..80e03b9 100644
--- a/toolchain/toolchain-external/ext-tool.mk
+++ b/toolchain/toolchain-external/ext-tool.mk
@@ -125,6 +125,7 @@ endif
 TOOLCHAIN_EXTERNAL_CROSS=$(TOOLCHAIN_EXTERNAL_BIN)/$(TOOLCHAIN_EXTERNAL_PREFIX)-
 TOOLCHAIN_EXTERNAL_CC=$(TOOLCHAIN_EXTERNAL_CROSS)gcc
 TOOLCHAIN_EXTERNAL_CXX=$(TOOLCHAIN_EXTERNAL_CROSS)g++
+TOOLCHAIN_EXTERNAL_READELF=$(TOOLCHAIN_EXTERNAL_CROSS)readelf
 TOOLCHAIN_EXTERNAL_WRAPPER_ARGS = -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
 
 ifeq ($(filter $(HOST_DIR)/%,$(TOOLCHAIN_EXTERNAL_BIN)),)
@@ -368,7 +369,9 @@ $(STAMP_DIR)/ext-toolchain-checked: $(TOOLCHAIN_EXTERNAL_DEPENDENCIES)
 		exit 1 ; \
 	fi ; \
 	if test x$(BR2_arm) == x"y" ; then \
-		$(call check_arm_abi,$(TOOLCHAIN_EXTERNAL_CC)) ; \
+		$(call check_arm_abi,\
+			"$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS)",\
+			$(TOOLCHAIN_EXTERNAL_READELF)) ; \
 	fi ; \
 	if test x$(BR2_INSTALL_LIBSTDCPP) == x"y" ; then \
 		$(call check_cplusplus,$(TOOLCHAIN_EXTERNAL_CXX)) ; \
-- 
1.8.1.2

[Buildroot] [PATCHv2 2/4] toolchain: check ARM EABI vs. EABIhf for external toolchains

[Yann E. MORIN]

Thomas, All,

On 2013-07-17 10:13 +0200, Thomas Petazzoni spake thusly:
> Following the introduction of the support of EABIhf as a second ARM
> ABI, it is important to check whether the external toolchain provided
> by the user actually uses the ABI that has been selected in the
> Buildroot configuration. This commit introduces such a check by
> looking at the 'Tag_ABI_VFP_args' tag of the architecture-specific
> section of the ELF headers. This assumes that ELF is the binary format
> used on ARM, which may not be the case on ARM noMMU systems (they use
> the FLAT binary format), but Buildroot doesn't have support for such
> systems at the moment.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  toolchain/helpers.mk                     | 15 +++++++++++++++
>  toolchain/toolchain-external/ext-tool.mk |  5 ++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index 94fe015..a8dab90 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -279,10 +279,25 @@ check_uclibc = \
>  #
>  check_arm_abi = \
>  	__CROSS_CC=$(strip $1) ; \
> +	__CROSS_READELF=$(strip $2) ; \
>  	EXT_TOOLCHAIN_TARGET=`LANG=C $${__CROSS_CC} -v 2>&1 | grep ^Target | cut -f2 -d ' '` ; \
>  	if ! echo $${EXT_TOOLCHAIN_TARGET} | grep -qE 'eabi(hf)?$$' ; then \
>  		echo "External toolchain uses the unsuported OABI" ; \
>  		exit 1 ; \
> +	fi ; \
> +	EXT_TOOLCHAIN_CRT1=`LANG=C $${__CROSS_CC} -print-file-name=crt1.o` ; \
> +	if $${__CROSS_READELF} -A $${EXT_TOOLCHAIN_CRT1} | grep -q "Tag_ABI_VFP_args:" ; then \
> +		EXT_TOOLCHAIN_ABI="eabihf" ; \
> +	else \
> +		EXT_TOOLCHAIN_ABI="eabi" ; \
> +	fi ; \
> +	if [ "$(BR2_ARM_EABI)" = "y" -a $${EXT_TOOLCHAIN_ABI} = "eabihf" ] ; then \
	                                ^^^^^^^^^^^^^^^^^^^^^
Obviously, EXT_TOOLCHAIN_ABI can't be empty, but for consistecy, it
would be better to quote it, too.

> +		echo "Incorrect ABI setting: EABI selected, but toolchain uses EABIhf" ; \
> +		exit 1 ; \
> +	fi ; \
> +	if [ "$(BR2_ARM_EABIHF)" = "y" -a $${EXT_TOOLCHAIN_ABI} = "eabi" ] ; then \
	                                  ^^^^^^^^^^^^^^^^^^^^^
Ditto.

Otherwise:
Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCHv2 3/4] toolchain/toolchain-external: don't use x$(...) construct or ==

[Thomas Petazzoni]

With modern shells, we can simply do test using the "$(...)" = "value"
form. This commit gets rid of the x$(...) = x"value" constructs and
replaces == by =, which is the correct operator to test the equality
of two strings with the test program.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/toolchain-external/ext-tool.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/toolchain/toolchain-external/ext-tool.mk b/toolchain/toolchain-external/ext-tool.mk
index 80e03b9..8aac50e 100644
--- a/toolchain/toolchain-external/ext-tool.mk
+++ b/toolchain/toolchain-external/ext-tool.mk
@@ -368,15 +368,15 @@ $(STAMP_DIR)/ext-toolchain-checked: $(TOOLCHAIN_EXTERNAL_DEPENDENCIES)
 		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
 		exit 1 ; \
 	fi ; \
-	if test x$(BR2_arm) == x"y" ; then \
+	if test "$(BR2_arm)" = "y" ; then \
 		$(call check_arm_abi,\
 			"$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS)",\
 			$(TOOLCHAIN_EXTERNAL_READELF)) ; \
 	fi ; \
-	if test x$(BR2_INSTALL_LIBSTDCPP) == x"y" ; then \
+	if test "$(BR2_INSTALL_LIBSTDCPP)" = "y" ; then \
 		$(call check_cplusplus,$(TOOLCHAIN_EXTERNAL_CXX)) ; \
 	fi ; \
-	if test x$(BR2_TOOLCHAIN_EXTERNAL_UCLIBC) == x"y" ; then \
+	if test "$(BR2_TOOLCHAIN_EXTERNAL_UCLIBC)" = "y" ; then \
 		$(call check_uclibc,$${SYSROOT_DIR}) ; \
 	else \
 		$(call check_glibc,$${SYSROOT_DIR}) ; \
@@ -464,7 +464,7 @@ $(STAMP_DIR)/ext-toolchain-installed: $(STAMP_DIR)/ext-toolchain-checked
 	if [ -L $${ARCH_SYSROOT_DIR}/lib64 -o -d $${ARCH_SYSROOT_DIR}/lib64 ] ; then \
 		$(call create_lib64_symlinks) ; \
 	fi ; \
-	if test x"$(BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY)" == x"y"; then \
+	if test "$(BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY)" = "y"; then \
 		$(call MESSAGE,"Copying gdbserver") ; \
 		gdbserver_found=0 ; \
 		for d in $${ARCH_SYSROOT_DIR}/usr $${ARCH_SYSROOT_DIR}/../debug-root/usr $${ARCH_SYSROOT_DIR}/usr/$${ARCH_LIB_DIR} ; do \
-- 
1.8.1.2

[Buildroot] [PATCHv2 3/4] toolchain/toolchain-external: don't use x$(...) construct or ==

[Yann E. MORIN]

Thomas, All,

On 2013-07-17 10:13 +0200, Thomas Petazzoni spake thusly:
> With modern shells, we can simply do test using the "$(...)" = "value"
> form. This commit gets rid of the x$(...) = x"value" constructs and
> replaces == by =, which is the correct operator to test the equality
> of two strings with the test program.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCHv2 4/4] arch/arm: update VFPv2 comment to mention ARMv5

[Thomas Petazzoni]

Commit 6b3a0417c4 ('arch/arm: arm926 may have VFP') forgot to update
the help text of the VFPv2 option to mention ARMv5. This commit fixes
that.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/Config.in.arm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/Config.in.arm b/arch/Config.in.arm
index b2fe257..096fc48 100644
--- a/arch/Config.in.arm
+++ b/arch/Config.in.arm
@@ -193,8 +193,8 @@ config BR2_ARM_FPU_VFPV2
 	depends on BR2_ARM_CPU_HAS_VFPV2 || BR2_ARM_CPU_MAYBE_HAS_VFPV2
 	help
 	  This option allows to use the VFPv2 floating point unit, as
-	  available in some ARMv6 processors (ARM1136JF-S,
-	  ARM1176JZF-S and ARM11 MPCore).
+	  available in some ARMv5 processors (ARM926EJ-S) ARMv6
+	  processors (ARM1136JF-S, ARM1176JZF-S and ARM11 MPCore).
 
 	  Note that this option is also safe to use for newer cores
 	  such as Cortex-A, because the VFPv3 and VFPv4 units are
-- 
1.8.1.2

[Buildroot] [PATCHv2 4/4] arch/arm: update VFPv2 comment to mention ARMv5

[Yann E. MORIN]

Thomas, All,

On 2013-07-17 10:13 +0200, Thomas Petazzoni spake thusly:
> Commit 6b3a0417c4 ('arch/arm: arm926 may have VFP') forgot to update
> the help text of the VFPv2 option to mention ARMv5. This commit fixes
> that.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/Config.in.arm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/Config.in.arm b/arch/Config.in.arm
> index b2fe257..096fc48 100644
> --- a/arch/Config.in.arm
> +++ b/arch/Config.in.arm
> @@ -193,8 +193,8 @@ config BR2_ARM_FPU_VFPV2
>  	depends on BR2_ARM_CPU_HAS_VFPV2 || BR2_ARM_CPU_MAYBE_HAS_VFPV2
>  	help
>  	  This option allows to use the VFPv2 floating point unit, as
> -	  available in some ARMv6 processors (ARM1136JF-S,
> -	  ARM1176JZF-S and ARM11 MPCore).
> +	  available in some ARMv5 processors (ARM926EJ-S) ARMv6
                                                        ^^^
Weird sentence construction. What about:
    [...] in some ARMv5 processors (ARM926EJ-S), and in some ARMv6 [...]
                                               ^^^^^^^^^^^^^

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'