[3/5,v2] toolchain: check the SSP option is known
diff mbox series

Message ID 6137_1552392583_5C87A187_6137_470_1_3090e77d-fd75-49ec-8643-3a89ee5ea133@OPEXCLILM6F.corporate.adroot.infra.ftgroup
State New
Headers show
Series
  • [1/5,v2] toolchain: prepare to pass more additional CFLAGS via the wrapper
Related show

Commit Message

Yann E. MORIN March 12, 2019, 12:09 p.m. UTC
From: "Yann E. MORIN" <yann.morin@orange.com>

Some toolchain vendors may have backported those options to older gcc
versions, and we have no way to know, so we have to check that the
user's selection is acceptable.

Extend the macro that currently checks for SSP in the toolchain, with
a new test that the actual SSP option is recognised and accepted.

Note that the SSP option is either totaly empty, or an already-quoted
string, so we can safely and easily assign it to a shell variable to
test and use it.

Note that we do not introduce BR2_TOOLCHAIN_HAS_SSP_STRONG, because:

  - our internal toolchain infra only supports gcc >= 4.9, so it has
    SSP strong;

  - of the external pre-built toolchains, only the codesourcery-arm
    one has a gcc-4.8 which lacks SSP strong, all the others have a
    gcc >= 4.9;

  - we'd still have to do the actual check for custom external
    toolchains anyway.

So, we're not adding BR2_TOOLCHAIN_HAS_SSP_STRONG just for a single
case.

Signed-off-by: "Yann E. MORIN" <yann.morin@orange.com>
Cc: Matt Weber <matthew.weber@rockwellcollins.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
Changes v1 -> v2:
  - expand the commit log to explain why we're not adding
    BR2_TOOLCHAIN_HAS_SSP_STRONG  (Arnout)
---
 toolchain/helpers.mk                                   | 8 ++++++++
 toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Arnout Vandecappelle March 12, 2019, 11:25 p.m. UTC | #1
On 12/03/2019 13:09, yann.morin@orange.com wrote:
> From: "Yann E. MORIN" <yann.morin@orange.com>
> 
> Some toolchain vendors may have backported those options to older gcc
> versions, and we have no way to know, so we have to check that the
> user's selection is acceptable.
> 
> Extend the macro that currently checks for SSP in the toolchain, with
> a new test that the actual SSP option is recognised and accepted.
> 
> Note that the SSP option is either totaly empty, or an already-quoted
> string, so we can safely and easily assign it to a shell variable to
> test and use it.
> 
> Note that we do not introduce BR2_TOOLCHAIN_HAS_SSP_STRONG, because:
> 
>   - our internal toolchain infra only supports gcc >= 4.9, so it has
>     SSP strong;
> 
>   - of the external pre-built toolchains, only the codesourcery-arm
>     one has a gcc-4.8 which lacks SSP strong, all the others have a
>     gcc >= 4.9;
> 
>   - we'd still have to do the actual check for custom external
>     toolchains anyway.
> 
> So, we're not adding BR2_TOOLCHAIN_HAS_SSP_STRONG just for a single
> case.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin@orange.com>
> Cc: Matt Weber <matthew.weber@rockwellcollins.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> 
> ---
> Changes v1 -> v2:
>   - expand the commit log to explain why we're not adding
>     BR2_TOOLCHAIN_HAS_SSP_STRONG  (Arnout)
> ---
>  toolchain/helpers.mk                                   | 8 ++++++++
>  toolchain/toolchain-external/pkg-toolchain-external.mk | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
> index e5520c00c3..ba097e83cf 100644
> --- a/toolchain/helpers.mk
> +++ b/toolchain/helpers.mk
> @@ -415,6 +415,7 @@ check_unusable_toolchain = \
>  # Check if the toolchain has SSP (stack smashing protector) support
>  #
>  # $1: cross-gcc path
> +# $2: gcc ssp option
>  #
>  check_toolchain_ssp = \
>  	__CROSS_CC=$(strip $1) ; \
> @@ -427,6 +428,13 @@ check_toolchain_ssp = \
>  		echo "SSP support not available in this toolchain, please disable BR2_TOOLCHAIN_EXTERNAL_HAS_SSP" ; \
>  		exit 1 ; \
>  	fi ; \
> +	__SSP_OPTION=$(2); \

 Is there any reason to pass this as an argument rather than using
$(BR2_SSP_OPTION) directly?

 The same goes for $(TOOLCHAIN_EXTERNAL_CC), but that ship has sailed.

 Regards,
 Arnout

> +	if [ -n "$${__SSP_OPTION}" ] ; then \
> +		if ! echo 'void main(){}' | $${__CROSS_CC} -Werror $${__SSP_OPTION} -x c - -o $(BUILD_DIR)/.br-toolchain-test.tmp >/dev/null 2>&1 ; then \
> +			echo "SSP option $${__SSP_OPTION} not available in this toolchain, please select another SSP level" ; \
> +			exit 1 ; \
> +		fi; \
> +	fi; \
>  	rm -f $(BUILD_DIR)/.br-toolchain-test.tmp*
>  
>  #
> diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> index db3570d96f..00cbd7b17a 100644
> --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> @@ -549,7 +549,7 @@ define $(2)_CONFIGURE_CMDS
>  	else \
>  		$$(call check_glibc,$$$${SYSROOT_DIR}) ; \
>  	fi
> -	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC))
> +	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC),$(BR2_SSP_OPTION))
>  endef
>  
>  $(2)_TOOLCHAIN_WRAPPER_ARGS += $$(TOOLCHAIN_EXTERNAL_TOOLCHAIN_WRAPPER_ARGS)
>
Yann E. MORIN March 13, 2019, 7:27 a.m. UTC | #2
Arnout, All,

On 2019-03-13 00:25 +0100, Arnout Vandecappelle spake thusly:
> On 12/03/2019 13:09, yann.morin@orange.com wrote:
> > From: "Yann E. MORIN" <yann.morin@orange.com>
[--SNIP--]
> > Note that the SSP option is either totaly empty, or an already-quoted
> > string, so we can safely and easily assign it to a shell variable to
> > test and use it.

Please note this paragraph above, and...

[--SNIP--]
> > @@ -427,6 +428,13 @@ check_toolchain_ssp = \
> >  		echo "SSP support not available in this toolchain, please disable BR2_TOOLCHAIN_EXTERNAL_HAS_SSP" ; \
> >  		exit 1 ; \
> >  	fi ; \
> > +	__SSP_OPTION=$(2); \
> 
>  Is there any reason to pass this as an argument rather than using
> $(BR2_SSP_OPTION) directly?

... correlate it with your question here.

There are two cases:

  - no SSP      BR2_SSP_OPTION is not defined, so it is not even the
                empty quoted string ""

  - some SSP    BR2_SSP_OPTION is a non-empty quoted string.

So...

> > +	if [ -n "$${__SSP_OPTION}" ] ; then \

... this test would have to use the qstrip-then-quote-anyway dance.

And finally, I tried to mimick the existing code, which lead to the
current patch: pass the option as a parameter to the macro, and use an
intermediate shell variable.

Regards,
Yann E. MORIN.

> > +		if ! echo 'void main(){}' | $${__CROSS_CC} -Werror $${__SSP_OPTION} -x c - -o $(BUILD_DIR)/.br-toolchain-test.tmp >/dev/null 2>&1 ; then \
> > +			echo "SSP option $${__SSP_OPTION} not available in this toolchain, please select another SSP level" ; \
> > +			exit 1 ; \
> > +		fi; \
> > +	fi; \
> >  	rm -f $(BUILD_DIR)/.br-toolchain-test.tmp*
> >  
> >  #
> > diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > index db3570d96f..00cbd7b17a 100644
> > --- a/toolchain/toolchain-external/pkg-toolchain-external.mk
> > +++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
> > @@ -549,7 +549,7 @@ define $(2)_CONFIGURE_CMDS
> >  	else \
> >  		$$(call check_glibc,$$$${SYSROOT_DIR}) ; \
> >  	fi
> > -	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC))
> > +	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC),$(BR2_SSP_OPTION))
> >  endef
> >  
> >  $(2)_TOOLCHAIN_WRAPPER_ARGS += $$(TOOLCHAIN_EXTERNAL_TOOLCHAIN_WRAPPER_ARGS)
> >
Thomas Petazzoni March 26, 2019, 7:26 p.m. UTC | #3
Hello,

On Tue, 12 Mar 2019 13:09:34 +0100
<yann.morin@orange.com> wrote:

>  # $1: cross-gcc path
> +# $2: gcc ssp option
>  #
>  check_toolchain_ssp = \
>  	__CROSS_CC=$(strip $1) ; \
> @@ -427,6 +428,13 @@ check_toolchain_ssp = \
>  		echo "SSP support not available in this toolchain, please disable BR2_TOOLCHAIN_EXTERNAL_HAS_SSP" ; \
>  		exit 1 ; \
>  	fi ; \
> +	__SSP_OPTION=$(2); \
> +	if [ -n "$${__SSP_OPTION}" ] ; then \
> +		if ! echo 'void main(){}' | $${__CROSS_CC} -Werror $${__SSP_OPTION} -x c - -o $(BUILD_DIR)/.br-toolchain-test.tmp >/dev/null 2>&1 ; then \
> +			echo "SSP option $${__SSP_OPTION} not available in this toolchain, please select another SSP level" ; \
> +			exit 1 ; \
> +		fi; \
> +	fi; \

Does it really make sense to check for both -fstack-protector (in the
code that already exists) *and* BR2_SSP_OPTION ?

Wouldn't it make more sense to just check for BR2_SSP_OPTION ?

Thomas
Yann E. MORIN March 27, 2019, 6:35 a.m. UTC | #4
Thomas, All,

On 2019-03-26 20:26 +0100, Thomas Petazzoni spake thusly:
> On Tue, 12 Mar 2019 13:09:34 +0100
> <yann.morin@orange.com> wrote:
> 
> >  # $1: cross-gcc path
> > +# $2: gcc ssp option
> >  #
> >  check_toolchain_ssp = \
> >  	__CROSS_CC=$(strip $1) ; \
> > @@ -427,6 +428,13 @@ check_toolchain_ssp = \
> >  		echo "SSP support not available in this toolchain, please disable BR2_TOOLCHAIN_EXTERNAL_HAS_SSP" ; \
> >  		exit 1 ; \
> >  	fi ; \
> > +	__SSP_OPTION=$(2); \
> > +	if [ -n "$${__SSP_OPTION}" ] ; then \
> > +		if ! echo 'void main(){}' | $${__CROSS_CC} -Werror $${__SSP_OPTION} -x c - -o $(BUILD_DIR)/.br-toolchain-test.tmp >/dev/null 2>&1 ; then \
> > +			echo "SSP option $${__SSP_OPTION} not available in this toolchain, please select another SSP level" ; \
> > +			exit 1 ; \
> > +		fi; \
> > +	fi; \
> 
> Does it really make sense to check for both -fstack-protector (in the
> code that already exists) *and* BR2_SSP_OPTION ?
> 
> Wouldn't it make more sense to just check for BR2_SSP_OPTION ?

My reasoning behind this additional test, is that there are two things
we need to test:

  * does the toolchain have SSP support (or not) as claimed by the user?

  * does the toolchain actually support the SSP level requested by the
    user?

There are packages that, wrongly or rightfully so, are not concerned
with te SSP level, but just the fact that there is SSP or not.

So, I decided to keep the existing test, and expand it. It means that we
may indeed check for -fstack-protector twice.

Regards,
Yann E. MORIN.

Patch
diff mbox series

diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
index e5520c00c3..ba097e83cf 100644
--- a/toolchain/helpers.mk
+++ b/toolchain/helpers.mk
@@ -415,6 +415,7 @@  check_unusable_toolchain = \
 # Check if the toolchain has SSP (stack smashing protector) support
 #
 # $1: cross-gcc path
+# $2: gcc ssp option
 #
 check_toolchain_ssp = \
 	__CROSS_CC=$(strip $1) ; \
@@ -427,6 +428,13 @@  check_toolchain_ssp = \
 		echo "SSP support not available in this toolchain, please disable BR2_TOOLCHAIN_EXTERNAL_HAS_SSP" ; \
 		exit 1 ; \
 	fi ; \
+	__SSP_OPTION=$(2); \
+	if [ -n "$${__SSP_OPTION}" ] ; then \
+		if ! echo 'void main(){}' | $${__CROSS_CC} -Werror $${__SSP_OPTION} -x c - -o $(BUILD_DIR)/.br-toolchain-test.tmp >/dev/null 2>&1 ; then \
+			echo "SSP option $${__SSP_OPTION} not available in this toolchain, please select another SSP level" ; \
+			exit 1 ; \
+		fi; \
+	fi; \
 	rm -f $(BUILD_DIR)/.br-toolchain-test.tmp*
 
 #
diff --git a/toolchain/toolchain-external/pkg-toolchain-external.mk b/toolchain/toolchain-external/pkg-toolchain-external.mk
index db3570d96f..00cbd7b17a 100644
--- a/toolchain/toolchain-external/pkg-toolchain-external.mk
+++ b/toolchain/toolchain-external/pkg-toolchain-external.mk
@@ -549,7 +549,7 @@  define $(2)_CONFIGURE_CMDS
 	else \
 		$$(call check_glibc,$$$${SYSROOT_DIR}) ; \
 	fi
-	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC))
+	$$(Q)$$(call check_toolchain_ssp,$$(TOOLCHAIN_EXTERNAL_CC),$(BR2_SSP_OPTION))
 endef
 
 $(2)_TOOLCHAIN_WRAPPER_ARGS += $$(TOOLCHAIN_EXTERNAL_TOOLCHAIN_WRAPPER_ARGS)