| Message ID | 6137_1552392583_5C87A187_6137_470_1_3090e77d-fd75-49ec-8643-3a89ee5ea133@OPEXCLILM6F.corporate.adroot.infra.ftgroup |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/5,v2] toolchain: prepare to pass more additional CFLAGS via the wrapper | expand |
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) >
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) > >
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
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.
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> You replied to all concerns, so I've applied as-is. Regards, Arnout
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)