[PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[alexandre.belloni]

From: Alexandre Belloni <alexandre.belloni@bootlin.com>

When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
with a syntax error which is not the one we are looking for:

<stdin>: In function ‘foo’:
<stdin>:1:29: warning: missing terminating " character
<stdin>:1:29: error: missing terminating " character
<stdin>:2:5: error: expected ‘:’ before ‘+’ token
<stdin>:2:7: warning: missing terminating " character
<stdin>:2:7: error: missing terminating " character
<stdin>:2:5: error: expected declaration or statement at end of input

Move all the CC_HAS_ASM_GOTO tests to scripts/gcc-goto.sh to solve the
escaping issues.

Fixes: 1aa0e8b144b6 ("Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug")
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 init/Kconfig        |  6 +++---
 scripts/gcc-goto.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index c984afc489de..9903a11cfe7d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -71,16 +71,16 @@ config CC_CAN_LINK_STATIC
 	default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
 
 config CC_HAS_ASM_GOTO
-	def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
+	def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto $(CC))
 
 config CC_HAS_ASM_GOTO_OUTPUT
 	depends on CC_HAS_ASM_GOTO
-	def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
+	def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_output $(CC))
 
 config CC_HAS_ASM_GOTO_TIED_OUTPUT
 	depends on CC_HAS_ASM_GOTO_OUTPUT
 	# Detect buggy gcc and clang, fixed in gcc-11 clang-14.
-	def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o /dev/null)
+	def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_tied_output $(CC))
 
 config TOOLS_SUPPORT_RELR
 	def_bool $(success,env "CC=$(CC)" "LD=$(LD)" "NM=$(NM)" "OBJCOPY=$(OBJCOPY)" $(srctree)/scripts/tools-support-relr.sh)
diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
index 8b980fb2270a..aa9498b74df8 100755
--- a/scripts/gcc-goto.sh
+++ b/scripts/gcc-goto.sh
@@ -3,6 +3,11 @@
 # Test for gcc 'asm goto' support
 # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
 
+TEST=$1
+shift
+
+case $TEST in
+    "goto")
 cat << "END" | $@ -x c - -fno-PIE -c -o /dev/null
 int main(void)
 {
@@ -20,3 +25,29 @@ entry:
 	return 0;
 }
 END
+    ;;
+
+    "goto_output")
+cat << "END" | $@ -x c - -c -o /dev/null
+int foo(int x) {
+	asm goto ("": "=r"(x) ::: bar);
+	return x;
+	bar: return 0;
+}
+END
+    ;;
+
+    "goto_tied_output")
+cat << "END" | $@ -x c - -c -o /dev/null
+int foo(int *x) {
+	asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar);
+	return *x;
+	bar: return 0;
+}
+END
+    ;;
+
+    *)
+	exit -1
+    ;;
+esac
-- 
2.37.1

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Nick Desaulniers]

On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
>
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> with a syntax error which is not the one we are looking for:

Thanks for the patch, though I think I'd rather see `/bin/bash`
hardcoded. Bash is a non-optional requirement as of
commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
process substitution")
scripts/ is kind of a mess...

>
> <stdin>: In function ‘foo’:
> <stdin>:1:29: warning: missing terminating " character
> <stdin>:1:29: error: missing terminating " character
> <stdin>:2:5: error: expected ‘:’ before ‘+’ token
> <stdin>:2:7: warning: missing terminating " character
> <stdin>:2:7: error: missing terminating " character
> <stdin>:2:5: error: expected declaration or statement at end of input
>
> Move all the CC_HAS_ASM_GOTO tests to scripts/gcc-goto.sh to solve the
> escaping issues.
>
> Fixes: 1aa0e8b144b6 ("Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug")
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  init/Kconfig        |  6 +++---
>  scripts/gcc-goto.sh | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index c984afc489de..9903a11cfe7d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -71,16 +71,16 @@ config CC_CAN_LINK_STATIC
>         default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
>
>  config CC_HAS_ASM_GOTO
> -       def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto $(CC))
>
>  config CC_HAS_ASM_GOTO_OUTPUT
>         depends on CC_HAS_ASM_GOTO
> -       def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_output $(CC))
>
>  config CC_HAS_ASM_GOTO_TIED_OUTPUT
>         depends on CC_HAS_ASM_GOTO_OUTPUT
>         # Detect buggy gcc and clang, fixed in gcc-11 clang-14.
> -       def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o /dev/null)
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_tied_output $(CC))
>
>  config TOOLS_SUPPORT_RELR
>         def_bool $(success,env "CC=$(CC)" "LD=$(LD)" "NM=$(NM)" "OBJCOPY=$(OBJCOPY)" $(srctree)/scripts/tools-support-relr.sh)
> diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
> index 8b980fb2270a..aa9498b74df8 100755
> --- a/scripts/gcc-goto.sh
> +++ b/scripts/gcc-goto.sh
> @@ -3,6 +3,11 @@
>  # Test for gcc 'asm goto' support
>  # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
>
> +TEST=$1
> +shift
> +
> +case $TEST in
> +    "goto")
>  cat << "END" | $@ -x c - -fno-PIE -c -o /dev/null
>  int main(void)
>  {
> @@ -20,3 +25,29 @@ entry:
>         return 0;
>  }
>  END
> +    ;;
> +
> +    "goto_output")
> +cat << "END" | $@ -x c - -c -o /dev/null
> +int foo(int x) {
> +       asm goto ("": "=r"(x) ::: bar);
> +       return x;
> +       bar: return 0;
> +}
> +END
> +    ;;
> +
> +    "goto_tied_output")
> +cat << "END" | $@ -x c - -c -o /dev/null
> +int foo(int *x) {
> +       asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar);
> +       return *x;
> +       bar: return 0;
> +}
> +END
> +    ;;
> +
> +    *)
> +       exit -1
> +    ;;
> +esac
> --
> 2.37.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Randy Dunlap]

On 8/17/22 16:46, Nick Desaulniers wrote:
> On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
>>
>> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
>> with a syntax error which is not the one we are looking for:
> 
> Thanks for the patch, though I think I'd rather see `/bin/bash`
> hardcoded. Bash is a non-optional requirement as of
> commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
> process substitution")
> scripts/ is kind of a mess...
> 

Well, once upon a time, we took patches to remove bash-isms (convert to
standard shell)...
No longer, AFAICT.

>>
>> <stdin>: In function ‘foo’:
>> <stdin>:1:29: warning: missing terminating " character
>> <stdin>:1:29: error: missing terminating " character
>> <stdin>:2:5: error: expected ‘:’ before ‘+’ token
>> <stdin>:2:7: warning: missing terminating " character
>> <stdin>:2:7: error: missing terminating " character
>> <stdin>:2:5: error: expected declaration or statement at end of input
>>
>> Move all the CC_HAS_ASM_GOTO tests to scripts/gcc-goto.sh to solve the
>> escaping issues.
>>
>> Fixes: 1aa0e8b144b6 ("Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug")
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> ---
>>  init/Kconfig        |  6 +++---
>>  scripts/gcc-goto.sh | 31 +++++++++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index c984afc489de..9903a11cfe7d 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -71,16 +71,16 @@ config CC_CAN_LINK_STATIC
>>         default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
>>
>>  config CC_HAS_ASM_GOTO
>> -       def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
>> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto $(CC))
>>
>>  config CC_HAS_ASM_GOTO_OUTPUT
>>         depends on CC_HAS_ASM_GOTO
>> -       def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
>> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_output $(CC))
>>
>>  config CC_HAS_ASM_GOTO_TIED_OUTPUT
>>         depends on CC_HAS_ASM_GOTO_OUTPUT
>>         # Detect buggy gcc and clang, fixed in gcc-11 clang-14.
>> -       def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o /dev/null)
>> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_tied_output $(CC))
>>
>>  config TOOLS_SUPPORT_RELR
>>         def_bool $(success,env "CC=$(CC)" "LD=$(LD)" "NM=$(NM)" "OBJCOPY=$(OBJCOPY)" $(srctree)/scripts/tools-support-relr.sh)
>> diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
>> index 8b980fb2270a..aa9498b74df8 100755
>> --- a/scripts/gcc-goto.sh
>> +++ b/scripts/gcc-goto.sh
>> @@ -3,6 +3,11 @@
>>  # Test for gcc 'asm goto' support
>>  # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
>>
>> +TEST=$1
>> +shift
>> +
>> +case $TEST in
>> +    "goto")
>>  cat << "END" | $@ -x c - -fno-PIE -c -o /dev/null
>>  int main(void)
>>  {
>> @@ -20,3 +25,29 @@ entry:
>>         return 0;
>>  }
>>  END
>> +    ;;
>> +
>> +    "goto_output")
>> +cat << "END" | $@ -x c - -c -o /dev/null
>> +int foo(int x) {
>> +       asm goto ("": "=r"(x) ::: bar);
>> +       return x;
>> +       bar: return 0;
>> +}
>> +END
>> +    ;;
>> +
>> +    "goto_tied_output")
>> +cat << "END" | $@ -x c - -c -o /dev/null
>> +int foo(int *x) {
>> +       asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar);
>> +       return *x;
>> +       bar: return 0;
>> +}
>> +END
>> +    ;;
>> +
>> +    *)
>> +       exit -1
>> +    ;;
>> +esac
>> --
>> 2.37.1
>>
> 
> 

-- 
~Randy

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Richard Purdie]

On Wed, 2022-08-17 at 16:52 -0700, Randy Dunlap wrote:
> 
> On 8/17/22 16:46, Nick Desaulniers wrote:
> > On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
> > > 
> > > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > 
> > > When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> > > with a syntax error which is not the one we are looking for:
> > 
> > Thanks for the patch, though I think I'd rather see `/bin/bash`
> > hardcoded. Bash is a non-optional requirement as of
> > commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
> > process substitution")
> > scripts/ is kind of a mess...
> > 
> 
> Well, once upon a time, we took patches to remove bash-isms (convert to
> standard shell)...
> No longer, AFAICT.

This problem is a little more subtle. 

As far as I could work out, exec() is used on entries like this in
kConfig. exec() falls back to /bin/sh so it is hard to see where this
would be changed to be /bin/bash.

I have no issue with bash being required and used and if someone can
work out how to make that happen for the exec() calls, I'm fine with
that. It would probably require some parsing of the "code" being passed
to kConfig to decide how to call exec().

What worries me is seeing the kernel behaviour changing depending on
whether /bin/sh is dash or bash and I think that should be fixed as a
more urgent matter.

I'd hope Alexandre's patch could be taken in the meantime as it doesn't
really hurt anything and does fix a very unexpected behaviour change
depending on the host system setup.

Cheers,

Richard

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Masahiro Yamada]

On Thu, Aug 18, 2022 at 6:14 PM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Wed, 2022-08-17 at 16:52 -0700, Randy Dunlap wrote:
> >
> > On 8/17/22 16:46, Nick Desaulniers wrote:
> > > On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
> > > >
> > > > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > >
> > > > When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> > > > with a syntax error which is not the one we are looking for:
> > >
> > > Thanks for the patch, though I think I'd rather see `/bin/bash`
> > > hardcoded. Bash is a non-optional requirement as of
> > > commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
> > > process substitution")
> > > scripts/ is kind of a mess...
> > >
> >
> > Well, once upon a time, we took patches to remove bash-isms (convert to
> > standard shell)...
> > No longer, AFAICT.
>
> This problem is a little more subtle.
>
> As far as I could work out, exec() is used on entries like this in
> kConfig. exec() falls back to /bin/sh so it is hard to see where this
> would be changed to be /bin/bash.



Kconfig uses popen() to execute a shell command.

See do_shell() in scripts/kconfig/preprocess.c



popen(3) says that
"the command is passed to /bin/sh using the -c flag.
interpretation, if any, is performed by the shell."



GNU Make is the same.
Make uses /bin/sh to execute commands in
recipe lines and $(shell ...) functions.
You can change the default shell via 'SHELL' variable.





>
> I have no issue with bash being required and used and if someone can
> work out how to make that happen for the exec() calls, I'm fine with
> that. It would probably require some parsing of the "code" being passed
> to kConfig to decide how to call exec().
>
> What worries me is seeing the kernel behaviour changing depending on
> whether /bin/sh is dash or bash and I think that should be fixed as a
> more urgent matter.
>
> I'd hope Alexandre's patch could be taken in the meantime as it doesn't
> really hurt anything and does fix a very unexpected behaviour change
> depending on the host system setup.



Agree.

We should apply a simple patch to fix this particular case
(I suggest to drop '\n' unless there is a reason to have it)
but discussion is needed to avoid portability issues like this.






One way is to encourage writing really portable code.
We used to strive for this (so we avoided bashism where possible)
because we believed sticking to POSIX was always good.

Some people make an effort in this direction [1], stating that
bash may not be installed, and bash is slower than dash.

In shell commands, we can use only commands/options defined in POSIX.
This is fragile because we do not have real /bin/sh,
and it is difficult to know what is POSIX-compliant.

People submit a script with #!/bin/sh but only tested on
environments where /bin/sh is a symlink to bash.








Another (and the opposite) way is to force users to use a particular
program like bash.

Actually, Googlers suggest this way.

Shell Style Guide [2] says:
"Bash is the only shell scripting language permitted for executables."


If we are forced to use bash, it should work in the same way for everyone.
We do not need to know what is POSIX-defined, and what is bashism.

I was thinking this way for a long time.
I wrote some patches to switch the default shell for Kbuild and Kconfig to bash
some time ago, but I did not submit them after some consideration.
I have patches in my local repository, I can share them.







BTW, Richard is here, so let me ask about BitBake.

The manual [3] clearly says:

"When you create these types of functions in your recipe or class files,
you need to follow the shell programming rules. The scripts are
executed by /bin/sh,
which may not be a bash shell but might be something such as dash.
You should not use Bash-specific script (bashisms)"

I just thought BitBake ran shell code in bash before,
but I might have misunderstood.
Do OE/Yocto allow only POSIX shell code?




[1]: https://lore.kernel.org/linux-kbuild/CAK7LNAT+4fOkJ5WDb9t5qXCqS+GhnbnG8wBffxNa1ZJ3=4Ps3Q@mail.gmail.com/T/#m74e382837a8a47a2278d892bc5d7f8bdbb86dba4
[2]: https://google.github.io/styleguide/shellguide.html
[3]: https://docs.yoctoproject.org/1.6/bitbake-user-manual/bitbake-user-manual.html




--
Best Regards
Masahiro Yamada

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Richard Purdie]

On Fri, 2022-08-19 at 04:31 +0900, Masahiro Yamada wrote:
> On Thu, Aug 18, 2022 at 6:14 PM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > This problem is a little more subtle.
> > 
> > As far as I could work out, exec() is used on entries like this in
> > kConfig. exec() falls back to /bin/sh so it is hard to see where this
> > would be changed to be /bin/bash.
> 
> 
> 
> Kconfig uses popen() to execute a shell command.
> 
> See do_shell() in scripts/kconfig/preprocess.c
> 
> popen(3) says that
> "the command is passed to /bin/sh using the -c flag.
> interpretation, if any, is performed by the shell."
> 
> GNU Make is the same.
> Make uses /bin/sh to execute commands in
> recipe lines and $(shell ...) functions.
> You can change the default shell via 'SHELL' variable.

That makes sense. I don't think we can easily change the shell popen()
uses.

> 
> BTW, Richard is here, so let me ask about BitBake.
> 
> The manual [3] clearly says:
> 
> "When you create these types of functions in your recipe or class files,
> you need to follow the shell programming rules. The scripts are
> executed by /bin/sh,
> which may not be a bash shell but might be something such as dash.
> You should not use Bash-specific script (bashisms)"
> 
> I just thought BitBake ran shell code in bash before,
> but I might have misunderstood.
> Do OE/Yocto allow only POSIX shell code?

Bitbake runs shell code with /bin/sh so we don't allow bashisms and
that has always been the case.

Like this case in the kernel, we do get people submitting changes which
were only tested with bash which can be frustrating but the manual and
our policy is quite clear. We just fix any that do creep through and
have test systems that have dash to try and catch them too.

Cheers,

Richard

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Masahiro Yamada]

On Fri, Aug 19, 2022 at 6:46 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Fri, 2022-08-19 at 04:31 +0900, Masahiro Yamada wrote:
> > On Thu, Aug 18, 2022 at 6:14 PM Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > >
> > > This problem is a little more subtle.
> > >
> > > As far as I could work out, exec() is used on entries like this in
> > > kConfig. exec() falls back to /bin/sh so it is hard to see where this
> > > would be changed to be /bin/bash.
> >
> >
> >
> > Kconfig uses popen() to execute a shell command.
> >
> > See do_shell() in scripts/kconfig/preprocess.c
> >
> > popen(3) says that
> > "the command is passed to /bin/sh using the -c flag.
> > interpretation, if any, is performed by the shell."
> >
> > GNU Make is the same.
> > Make uses /bin/sh to execute commands in
> > recipe lines and $(shell ...) functions.
> > You can change the default shell via 'SHELL' variable.
>
> That makes sense. I don't think we can easily change the shell popen()
> uses.
>
> >
> > BTW, Richard is here, so let me ask about BitBake.
> >
> > The manual [3] clearly says:
> >
> > "When you create these types of functions in your recipe or class files,
> > you need to follow the shell programming rules. The scripts are
> > executed by /bin/sh,
> > which may not be a bash shell but might be something such as dash.
> > You should not use Bash-specific script (bashisms)"
> >
> > I just thought BitBake ran shell code in bash before,
> > but I might have misunderstood.
> > Do OE/Yocto allow only POSIX shell code?
>
> Bitbake runs shell code with /bin/sh so we don't allow bashisms and
> that has always been the case.
>
> Like this case in the kernel, we do get people submitting changes which
> were only tested with bash which can be frustrating but the manual and
> our policy is quite clear. We just fix any that do creep through and
> have test systems that have dash to try and catch them too.
>
> Cheers,
>
> Richard


Thanks.
So, Bitbake is the same approach as the kernel.




This is a patch set to use bash forcibly. FWIW.

https://lore.kernel.org/lkml/20220819065604.295572-1-masahiroy@kernel.org/





-- 
Best Regards
Masahiro Yamada

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Richard Purdie]

On Fri, 2022-08-19 at 17:40 +0900, Masahiro Yamada wrote:
> On Fri, Aug 19, 2022 at 6:46 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > On Fri, 2022-08-19 at 04:31 +0900, Masahiro Yamada wrote:
> > > On Thu, Aug 18, 2022 at 6:14 PM Richard Purdie
> > > <richard.purdie@linuxfoundation.org> wrote:
> > > > 
> > > > This problem is a little more subtle.
> > > > 
> > > > As far as I could work out, exec() is used on entries like this in
> > > > kConfig. exec() falls back to /bin/sh so it is hard to see where this
> > > > would be changed to be /bin/bash.
> > > 
> > > 
> > > 
> > > Kconfig uses popen() to execute a shell command.
> > > 
> > > See do_shell() in scripts/kconfig/preprocess.c
> > > 
> > > popen(3) says that
> > > "the command is passed to /bin/sh using the -c flag.
> > > interpretation, if any, is performed by the shell."
> > > 
> > > GNU Make is the same.
> > > Make uses /bin/sh to execute commands in
> > > recipe lines and $(shell ...) functions.
> > > You can change the default shell via 'SHELL' variable.
> > 
> > That makes sense. I don't think we can easily change the shell popen()
> > uses.
> > 
> > > 
> > > BTW, Richard is here, so let me ask about BitBake.
> > > 
> > > The manual [3] clearly says:
> > > 
> > > "When you create these types of functions in your recipe or class files,
> > > you need to follow the shell programming rules. The scripts are
> > > executed by /bin/sh,
> > > which may not be a bash shell but might be something such as dash.
> > > You should not use Bash-specific script (bashisms)"
> > > 
> > > I just thought BitBake ran shell code in bash before,
> > > but I might have misunderstood.
> > > Do OE/Yocto allow only POSIX shell code?
> > 
> > Bitbake runs shell code with /bin/sh so we don't allow bashisms and
> > that has always been the case.
> > 
> > Like this case in the kernel, we do get people submitting changes which
> > were only tested with bash which can be frustrating but the manual and
> > our policy is quite clear. We just fix any that do creep through and
> > have test systems that have dash to try and catch them too.
> > 
> > Cheers,
> > 
> > Richard
> 
> Thanks.
> So, Bitbake is the same approach as the kernel.

Yes, effectively.
> 
> This is a patch set to use bash forcibly. FWIW.
> 
> https://lore.kernel.org/lkml/20220819065604.295572-1-masahiroy@kernel.org/

Thanks, I'm watching with interest to see what happens.

The original patch causing issues was backported into several stable
releases and this won't be so we have a bit of a challenge there but we
have also started carrying patches to fix that too so as long as things
get fixed in master we should be ok in the long run and I'm happy.

Cheers,

Richard

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Masahiro Yamada]

On Thu, Aug 18, 2022 at 8:47 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
> >
> > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >
> > When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> > with a syntax error which is not the one we are looking for:
>
> Thanks for the patch, though I think I'd rather see `/bin/bash`
> hardcoded. Bash is a non-optional requirement as of
> commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
> process substitution")
> scripts/ is kind of a mess...



As for scripts/ mess,
perhaps we can remove scripts/gcc-goto.sh?



http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637
seems like a bug for GCC 4.x at least...


If we can revert the following two commits:
f3c003f72dfb2497056bcbb864885837a1968ed5
a9468f30b5eac6957c86aea97954553bfb7c1f18

... the asm-goto test will become simple enough
to fit into init/Kconfig.


If we know all GCC 5.1+ and Clang 11+ support asm-goto,
we can remove CC_HAS_ASM_GOTO, but I am not so sure.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Nick Desaulniers]

On Thu, Aug 18, 2022 at 7:29 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Aug 18, 2022 at 8:47 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Thu, Aug 4, 2022 at 12:03 PM <alexandre.belloni@bootlin.com> wrote:
> > >
> > > From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > >
> > > When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> > > with a syntax error which is not the one we are looking for:
> >
> > Thanks for the patch, though I think I'd rather see `/bin/bash`
> > hardcoded. Bash is a non-optional requirement as of
> > commit da4288b95baa ("scripts/check-local-export: avoid 'wait $!' for
> > process substitution")
> > scripts/ is kind of a mess...
>
>
>
> As for scripts/ mess,
> perhaps we can remove scripts/gcc-goto.sh?

Good idea, I think we can.

>
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48637
> seems like a bug for GCC 4.x at least...
>
>
> If we can revert the following two commits:
> f3c003f72dfb2497056bcbb864885837a1968ed5
> a9468f30b5eac6957c86aea97954553bfb7c1f18
>
> ... the asm-goto test will become simple enough
> to fit into init/Kconfig.
>
>
> If we know all GCC 5.1+ and Clang 11+ support asm-goto,
> we can remove CC_HAS_ASM_GOTO, but I am not so sure.

Yeah, asm goto has been supported for many releases of either compiler
at this point; here's versions we don't support anymore for kbuild yet
demonstrate asm goto support: https://godbolt.org/z/dr3zzn98e.

I'll send a patch with your suggested-by tag.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] init/Kconfig: fix CC_HAS_ASM_GOTO_TIED_OUTPUT test with dash

[Masahiro Yamada]

On Fri, Aug 5, 2022 at 4:03 AM <alexandre.belloni@bootlin.com> wrote:
>
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> When using dash as /bin/sh, the CC_HAS_ASM_GOTO_TIED_OUTPUT test fails
> with a syntax error which is not the one we are looking for:
>
> <stdin>: In function ‘foo’:
> <stdin>:1:29: warning: missing terminating " character
> <stdin>:1:29: error: missing terminating " character
> <stdin>:2:5: error: expected ‘:’ before ‘+’ token
> <stdin>:2:7: warning: missing terminating " character
> <stdin>:2:7: error: missing terminating " character
> <stdin>:2:5: error: expected declaration or statement at end of input
>
> Move all the CC_HAS_ASM_GOTO tests to scripts/gcc-goto.sh to solve the
> escaping issues.
>
> Fixes: 1aa0e8b144b6 ("Kconfig: Add option for asm goto w/ tied outputs to workaround clang-13 bug")
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  init/Kconfig        |  6 +++---
>  scripts/gcc-goto.sh | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index c984afc489de..9903a11cfe7d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -71,16 +71,16 @@ config CC_CAN_LINK_STATIC
>         default $(success,$(srctree)/scripts/cc-can-link.sh $(CC) $(CLANG_FLAGS) $(USERCFLAGS) $(USERLDFLAGS) $(m32-flag) -static)
>
>  config CC_HAS_ASM_GOTO
> -       def_bool $(success,$(srctree)/scripts/gcc-goto.sh $(CC))
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto $(CC))
>
>  config CC_HAS_ASM_GOTO_OUTPUT
>         depends on CC_HAS_ASM_GOTO
> -       def_bool $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_output $(CC))
>
>  config CC_HAS_ASM_GOTO_TIED_OUTPUT
>         depends on CC_HAS_ASM_GOTO_OUTPUT
>         # Detect buggy gcc and clang, fixed in gcc-11 clang-14.
> -       def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o /dev/null)
> +       def_bool $(success,$(srctree)/scripts/gcc-goto.sh goto_tied_output $(CC))
>
>  config TOOLS_SUPPORT_RELR
>         def_bool $(success,env "CC=$(CC)" "LD=$(LD)" "NM=$(NM)" "OBJCOPY=$(OBJCOPY)" $(srctree)/scripts/tools-support-relr.sh)
> diff --git a/scripts/gcc-goto.sh b/scripts/gcc-goto.sh
> index 8b980fb2270a..aa9498b74df8 100755
> --- a/scripts/gcc-goto.sh
> +++ b/scripts/gcc-goto.sh
> @@ -3,6 +3,11 @@
>  # Test for gcc 'asm goto' support
>  # Copyright (C) 2010, Jason Baron <jbaron@redhat.com>
>
> +TEST=$1
> +shift
> +
> +case $TEST in
> +    "goto")
>  cat << "END" | $@ -x c - -fno-PIE -c -o /dev/null
>  int main(void)
>  {
> @@ -20,3 +25,29 @@ entry:
>         return 0;
>  }
>  END
> +    ;;
> +
> +    "goto_output")
> +cat << "END" | $@ -x c - -c -o /dev/null
> +int foo(int x) {
> +       asm goto ("": "=r"(x) ::: bar);
> +       return x;
> +       bar: return 0;
> +}
> +END
> +    ;;
> +
> +    "goto_tied_output")
> +cat << "END" | $@ -x c - -c -o /dev/null
> +int foo(int *x) {
> +       asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar);
> +       return *x;
> +       bar: return 0;
> +}
> +END
> +    ;;
> +
> +    *)
> +       exit -1
> +    ;;
> +esac
> --
> 2.37.1
>







It is well known that 'echo' command is not portable
when used with escape sequences.
'printf' will do in such situations.


The POSIX spec [1] also mentions this:

  "If the first operand is -n, or if any of the operands contain a
<backslash> character,
   the results are implementation-defined."


  "It is not possible to use echo portably across all POSIX systems
  unless both -n (as the first argument) and escape sequences are omitted."


[1] : https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html






So, the issue for this case is '\n'.
Do we need it?


Even with '\n' dropped, I still can detect the bug of clang-13.



[For clang 13]


$ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .": "+m"(*x)
::: bar); return *x; bar: return 0; }' | clang-13 -x c - -c -o
/dev/null
<stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
int foo(int *x) { asm goto (".long (%l[bar]) - .": "+m"(*x) ::: bar);
return *x; bar: return 0; }
                            ^
<stdin>:1:29: error: unknown token in expression
<inline asm>:1:9: note: instantiated into assembly here
        .long () - .
               ^
2 errors generated.




[For clang 14]

$ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .": "+m"(*x)
::: bar); return *x; bar: return 0; }' | clang-14 -x c - -c -o
/dev/null
$ echo $?
0





So, please drop '\n' and check if it is OK.

That will be simpler for back-porting.






-- 
Best Regards
Masahiro Yamada