diff mbox

toochain/wrapper: fix potential bug in foreach loop

Message ID 1445462467-26873-1-git-send-email-yann.morin.1998@free.fr
State Accepted
Commit 32e2636c51e409f108f24174257ea4867ec6d74d
Headers show

Commit Message

Yann E. MORIN Oct. 21, 2015, 9:21 p.m. UTC
In Makefile, the comma ',' is used to separate the arguments passed to
functions, so we should not be allowed to use straight commas in strings
we want to expand.

For the toolchain wrapper, we need to transform a list:
    -mfoo -mbar -mbuz

into something acceptable for a C array assignment:
    "-mfoo", "-mbar", "-mbuz",

So, we use a $(foreach ...) loop for that. However, we do have a
straight comma in there.

It does not cause any issue in practice, since $(foreach) is a make
builtin function that accepts three and only three parameters.

However, this is not sane.

Change the straight comma to the usual $(comma) expansion, like we sould
do for a call to any other function.

At the same time, make the code a bit easier to read, by first creating
the transformed list, and then creating the define.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 toolchain/toolchain-wrapper.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Arnout Vandecappelle Oct. 22, 2015, 8:27 p.m. UTC | #1
On 21-10-15 23:21, Yann E. MORIN wrote:
> In Makefile, the comma ',' is used to separate the arguments passed to
> functions, so we should not be allowed to use straight commas in strings
> we want to expand.
> 
> For the toolchain wrapper, we need to transform a list:
>     -mfoo -mbar -mbuz
> 
> into something acceptable for a C array assignment:
>     "-mfoo", "-mbar", "-mbuz",
> 
> So, we use a $(foreach ...) loop for that. However, we do have a
> straight comma in there.
> 
> It does not cause any issue in practice, since $(foreach) is a make
> builtin function that accepts three and only three parameters.
> 
> However, this is not sane.
> 
> Change the straight comma to the usual $(comma) expansion, like we sould
> do for a call to any other function.
> 
> At the same time, make the code a bit easier to read, by first creating
> the transformed list, and then creating the define.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Arnout Vandecappelle <arnout@mind.be>

Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Tested-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
 (Build test with BR2_TARGET_OPTIMIZATION containing two elements)


 Regards,
 Arnout


> ---
>  toolchain/toolchain-wrapper.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/toolchain/toolchain-wrapper.mk b/toolchain/toolchain-wrapper.mk
> index eba2b38..af39071 100644
> --- a/toolchain/toolchain-wrapper.mk
> +++ b/toolchain/toolchain-wrapper.mk
> @@ -14,7 +14,9 @@ TOOLCHAIN_WRAPPER_ARGS += -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
>  
>  # We create a list like '"-mfoo", "-mbar", "-mbarfoo"' so that each flag is a
>  # separate argument when used in execv() by the toolchain wrapper.
> -TOOLCHAIN_WRAPPER_ARGS += -DBR_ADDITIONAL_CFLAGS='$(foreach f,$(call qstrip,$(BR2_TARGET_OPTIMIZATION)),"$(f)",)'
> +TOOLCHAIN_WRAPPER_OPTS = \
> +	$(foreach f,$(call qstrip,$(BR2_TARGET_OPTIMIZATION)),"$(f)"$(comma))
> +TOOLCHAIN_WRAPPER_ARGS += -DBR_ADDITIONAL_CFLAGS='$(TOOLCHAIN_WRAPPER_OPTS)'
>  
>  ifeq ($(BR2_CCACHE),y)
>  TOOLCHAIN_WRAPPER_ARGS += -DBR_CCACHE
>
Peter Korsgaard Oct. 25, 2015, 10:03 p.m. UTC | #2
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > In Makefile, the comma ',' is used to separate the arguments passed to
 > functions, so we should not be allowed to use straight commas in strings
 > we want to expand.

 > For the toolchain wrapper, we need to transform a list:
 >     -mfoo -mbar -mbuz

 > into something acceptable for a C array assignment:
 >     "-mfoo", "-mbar", "-mbuz",

 > So, we use a $(foreach ...) loop for that. However, we do have a
 > straight comma in there.

 > It does not cause any issue in practice, since $(foreach) is a make
 > builtin function that accepts three and only three parameters.

 > However, this is not sane.

 > Change the straight comma to the usual $(comma) expansion, like we sould
 > do for a call to any other function.

 > At the same time, make the code a bit easier to read, by first creating
 > the transformed list, and then creating the define.

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Arnout Vandecappelle <arnout@mind.be>

Committed, thanks.
diff mbox

Patch

diff --git a/toolchain/toolchain-wrapper.mk b/toolchain/toolchain-wrapper.mk
index eba2b38..af39071 100644
--- a/toolchain/toolchain-wrapper.mk
+++ b/toolchain/toolchain-wrapper.mk
@@ -14,7 +14,9 @@  TOOLCHAIN_WRAPPER_ARGS += -DBR_SYSROOT='"$(STAGING_SUBDIR)"'
 
 # We create a list like '"-mfoo", "-mbar", "-mbarfoo"' so that each flag is a
 # separate argument when used in execv() by the toolchain wrapper.
-TOOLCHAIN_WRAPPER_ARGS += -DBR_ADDITIONAL_CFLAGS='$(foreach f,$(call qstrip,$(BR2_TARGET_OPTIMIZATION)),"$(f)",)'
+TOOLCHAIN_WRAPPER_OPTS = \
+	$(foreach f,$(call qstrip,$(BR2_TARGET_OPTIMIZATION)),"$(f)"$(comma))
+TOOLCHAIN_WRAPPER_ARGS += -DBR_ADDITIONAL_CFLAGS='$(TOOLCHAIN_WRAPPER_OPTS)'
 
 ifeq ($(BR2_CCACHE),y)
 TOOLCHAIN_WRAPPER_ARGS += -DBR_CCACHE