diff mbox series

[v2] kbuild: fix SIGPIPE error message for AR=gcc-ar and AR=llvm-ar

Message ID 20221027162839.410720-1-masahiroy@kernel.org
State New
Headers
Series [v2] kbuild: fix SIGPIPE error message for AR=gcc-ar and AR=llvm-ar |

Commit Message

Masahiro Yamada Oct. 27, 2022, 4:28 p.m. UTC 
  Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
  /usr/bin/ar terminated with signal 13 [Broken pipe]

Nathan Chancellor reported the latest AR=llvm-ar shows
  error: write on a pipe with no reader

The latter occurs since LLVM commit 51b557adc131 ("Add an error message
to the default SIGPIPE handler").

The resulting vmlinux is correct, but it is better to silence it.

'head -n1' exits after reading the first line, so the pipe is closed.

Use 'sed -n 1p' to eat the stream till the end.

Fixes: 321648455061 ("kbuild: use obj-y instead extra-y for objects placed at the head")
Link: https://github.com/ClangBuiltLinux/linux/issues/1651
Reported-by: Jiri Slaby <jirislaby@kernel.org>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---

Changes in v2:
  - Update commit description to mention llvm-ar

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nick Desaulniers Oct. 27, 2022, 4:36 p.m. UTC | #1 
On Thu, Oct 27, 2022 at 9:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
>   /usr/bin/ar terminated with signal 13 [Broken pipe]
>
> Nathan Chancellor reported the latest AR=llvm-ar shows
>   error: write on a pipe with no reader
>
> The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> to the default SIGPIPE handler").
>
> The resulting vmlinux is correct, but it is better to silence it.
>
> 'head -n1' exits after reading the first line, so the pipe is closed.
>
> Use 'sed -n 1p' to eat the stream till the end.
>
> Fixes: 321648455061 ("kbuild: use obj-y instead extra-y for objects placed at the head")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1651
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Looks great! Thanks all.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>   - Update commit description to mention llvm-ar
>
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index e90bb2b38607..e9e7eff906a5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1218,7 +1218,7 @@ quiet_cmd_ar_vmlinux.a = AR      $@
>        cmd_ar_vmlinux.a = \
>         rm -f $@; \
>         $(AR) cDPrST $@ $(KBUILD_VMLINUX_OBJS); \
> -       $(AR) mPiT $$($(AR) t $@ | head -n1) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
> +       $(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
>
>  targets += vmlinux.a
>  vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
> --
> 2.34.1
>
Nathan Chancellor Oct. 27, 2022, 9:24 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
> Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
>   /usr/bin/ar terminated with signal 13 [Broken pipe]
> 
> Nathan Chancellor reported the latest AR=llvm-ar shows
>   error: write on a pipe with no reader
> 
> The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> to the default SIGPIPE handler").
> 
> The resulting vmlinux is correct, but it is better to silence it.
> 
> 'head -n1' exits after reading the first line, so the pipe is closed.
> 
> Use 'sed -n 1p' to eat the stream till the end.
> 
> Fixes: 321648455061 ("kbuild: use obj-y instead extra-y for objects placed at the head")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1651
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
> 
> Changes in v2:
>   - Update commit description to mention llvm-ar
> 
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index e90bb2b38607..e9e7eff906a5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1218,7 +1218,7 @@ quiet_cmd_ar_vmlinux.a = AR      $@
>        cmd_ar_vmlinux.a = \
>  	rm -f $@; \
>  	$(AR) cDPrST $@ $(KBUILD_VMLINUX_OBJS); \
> -	$(AR) mPiT $$($(AR) t $@ | head -n1) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
> +	$(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
>  
>  targets += vmlinux.a
>  vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
> -- 
> 2.34.1
>
Kees Cook Nov. 16, 2022, 7 p.m. UTC | #3 
On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
> Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
>   /usr/bin/ar terminated with signal 13 [Broken pipe]
> 
> Nathan Chancellor reported the latest AR=llvm-ar shows
>   error: write on a pipe with no reader
> 
> The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> to the default SIGPIPE handler").
> 
> The resulting vmlinux is correct, but it is better to silence it.
> 
> 'head -n1' exits after reading the first line, so the pipe is closed.
> 
> Use 'sed -n 1p' to eat the stream till the end.

I think this is wrong because it needlessly consumes CPU time. SIGPIPE
is _needed_ to stop a process after we found what we needed, but it's up
to the caller (the shell here) to determine what to do about it.

Similarly, that LLVM commit is wrong -- tools should _not_ catch their
own SIGPIPEs. They should be caught by their callers.

For example, see:

$ seq 10000 | head -n1
1

^^^ no warnings from the shell (caller of "seq")
And you can see it _is_ being killed by SIGPIPE:

$ strace seq 1000 | head -n1
...
write(1, "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14"..., 8192) = 8192
1
write(1, "\n1861\n1862\n1863\n1864\n1865\n1866\n1"..., 4096) = -1 EPIPE (Broken pipe)
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503448, si_uid=1000} ---
+++ killed by SIGPIPE +++

If we use "sed -n 1p" seq will continue to run, consuming needless time
and CPU resources.

So, I strongly think this is the wrong solution. SIGPIPE should be
ignored for ar, and LLVM should _not_ catch its own SIGPIPE.

-Kees

> 
> Fixes: 321648455061 ("kbuild: use obj-y instead extra-y for objects placed at the head")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1651
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> 
> Changes in v2:
>   - Update commit description to mention llvm-ar
> 
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index e90bb2b38607..e9e7eff906a5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1218,7 +1218,7 @@ quiet_cmd_ar_vmlinux.a = AR      $@
>        cmd_ar_vmlinux.a = \
>  	rm -f $@; \
>  	$(AR) cDPrST $@ $(KBUILD_VMLINUX_OBJS); \
> -	$(AR) mPiT $$($(AR) t $@ | head -n1) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
> +	$(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
>  
>  targets += vmlinux.a
>  vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE
> -- 
> 2.34.1
>
Masahiro Yamada Nov. 16, 2022, 8:37 p.m. UTC | #4 
On Thu, Nov 17, 2022 at 4:01 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
> > Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
> >   /usr/bin/ar terminated with signal 13 [Broken pipe]
> >
> > Nathan Chancellor reported the latest AR=llvm-ar shows
> >   error: write on a pipe with no reader
> >
> > The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> > to the default SIGPIPE handler").
> >
> > The resulting vmlinux is correct, but it is better to silence it.
> >
> > 'head -n1' exits after reading the first line, so the pipe is closed.
> >
> > Use 'sed -n 1p' to eat the stream till the end.
>
> I think this is wrong because it needlessly consumes CPU time. SIGPIPE
> is _needed_ to stop a process after we found what we needed, but it's up
> to the caller (the shell here) to determine what to do about it.
>
> Similarly, that LLVM commit is wrong -- tools should _not_ catch their
> own SIGPIPEs. They should be caught by their callers.
>
> For example, see:
>
> $ seq 10000 | head -n1
> 1
>
> ^^^ no warnings from the shell (caller of "seq")
> And you can see it _is_ being killed by SIGPIPE:
>
> $ strace seq 1000 | head -n1
> ...
> write(1, "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14"..., 8192) = 8192
> 1
> write(1, "\n1861\n1862\n1863\n1864\n1865\n1866\n1"..., 4096) = -1 EPIPE (Broken pipe)
> --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503448, si_uid=1000} ---
> +++ killed by SIGPIPE +++
>
> If we use "sed -n 1p" seq will continue to run, consuming needless time
> and CPU resources.
>
> So, I strongly think this is the wrong solution. SIGPIPE should be
> ignored for ar, and LLVM should _not_ catch its own SIGPIPE.
>
> -Kees


I thought of this - it is just wasting CPU time,
but I did not come up with a better idea on the kbuild side.

I do not want to use 2>/dev/null because it may hide
non-SIGPIPE (i.e. real) errors.


I think you guys will be keen on fixing llvm.
I hope gcc as well?
Kees Cook Nov. 16, 2022, 10:07 p.m. UTC | #5 
On Thu, Nov 17, 2022 at 05:37:31AM +0900, Masahiro Yamada wrote:
> On Thu, Nov 17, 2022 at 4:01 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
> > > Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
> > >   /usr/bin/ar terminated with signal 13 [Broken pipe]
> > >
> > > Nathan Chancellor reported the latest AR=llvm-ar shows
> > >   error: write on a pipe with no reader
> > >
> > > The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> > > to the default SIGPIPE handler").
> > >
> > > The resulting vmlinux is correct, but it is better to silence it.
> > >
> > > 'head -n1' exits after reading the first line, so the pipe is closed.
> > >
> > > Use 'sed -n 1p' to eat the stream till the end.
> >
> > I think this is wrong because it needlessly consumes CPU time. SIGPIPE
> > is _needed_ to stop a process after we found what we needed, but it's up
> > to the caller (the shell here) to determine what to do about it.
> >
> > Similarly, that LLVM commit is wrong -- tools should _not_ catch their
> > own SIGPIPEs. They should be caught by their callers.
> >
> > For example, see:
> >
> > $ seq 10000 | head -n1
> > 1
> >
> > ^^^ no warnings from the shell (caller of "seq")
> > And you can see it _is_ being killed by SIGPIPE:
> >
> > $ strace seq 1000 | head -n1
> > ...
> > write(1, "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14"..., 8192) = 8192
> > 1
> > write(1, "\n1861\n1862\n1863\n1864\n1865\n1866\n1"..., 4096) = -1 EPIPE (Broken pipe)
> > --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503448, si_uid=1000} ---
> > +++ killed by SIGPIPE +++
> >
> > If we use "sed -n 1p" seq will continue to run, consuming needless time
> > and CPU resources.
> >
> > So, I strongly think this is the wrong solution. SIGPIPE should be
> > ignored for ar, and LLVM should _not_ catch its own SIGPIPE.
> >
> > -Kees
> 
> 
> I thought of this - it is just wasting CPU time,
> but I did not come up with a better idea on the kbuild side.
> 
> I do not want to use 2>/dev/null because it may hide
> non-SIGPIPE (i.e. real) errors.

Yes, I've opened an upstream LLVM bug for this:
https://github.com/llvm/llvm-project/issues/59037
Masahiro Yamada Dec. 6, 2022, 4:24 a.m. UTC | #6 
On Thu, Nov 17, 2022 at 7:07 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Nov 17, 2022 at 05:37:31AM +0900, Masahiro Yamada wrote:
> > On Thu, Nov 17, 2022 at 4:01 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
> > > > Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
> > > >   /usr/bin/ar terminated with signal 13 [Broken pipe]
> > > >
> > > > Nathan Chancellor reported the latest AR=llvm-ar shows
> > > >   error: write on a pipe with no reader
> > > >
> > > > The latter occurs since LLVM commit 51b557adc131 ("Add an error message
> > > > to the default SIGPIPE handler").
> > > >
> > > > The resulting vmlinux is correct, but it is better to silence it.
> > > >
> > > > 'head -n1' exits after reading the first line, so the pipe is closed.
> > > >
> > > > Use 'sed -n 1p' to eat the stream till the end.
> > >
> > > I think this is wrong because it needlessly consumes CPU time. SIGPIPE
> > > is _needed_ to stop a process after we found what we needed, but it's up
> > > to the caller (the shell here) to determine what to do about it.
> > >
> > > Similarly, that LLVM commit is wrong -- tools should _not_ catch their
> > > own SIGPIPEs. They should be caught by their callers.
> > >
> > > For example, see:
> > >
> > > $ seq 10000 | head -n1
> > > 1
> > >
> > > ^^^ no warnings from the shell (caller of "seq")
> > > And you can see it _is_ being killed by SIGPIPE:
> > >
> > > $ strace seq 1000 | head -n1
> > > ...
> > > write(1, "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14"..., 8192) = 8192
> > > 1
> > > write(1, "\n1861\n1862\n1863\n1864\n1865\n1866\n1"..., 4096) = -1 EPIPE (Broken pipe)
> > > --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503448, si_uid=1000} ---
> > > +++ killed by SIGPIPE +++
> > >
> > > If we use "sed -n 1p" seq will continue to run, consuming needless time
> > > and CPU resources.
> > >
> > > So, I strongly think this is the wrong solution. SIGPIPE should be
> > > ignored for ar, and LLVM should _not_ catch its own SIGPIPE.
> > >
> > > -Kees
> >
> >
> > I thought of this - it is just wasting CPU time,
> > but I did not come up with a better idea on the kbuild side.
> >
> > I do not want to use 2>/dev/null because it may hide
> > non-SIGPIPE (i.e. real) errors.
>
> Yes, I've opened an upstream LLVM bug for this:
> https://github.com/llvm/llvm-project/issues/59037
>
> --
> Kees Cook



BTW, Python does something similar by default.
(noisy back-trace for SIGPIPE)





masahiro@zoe:/tmp$ cat test.py
#!/usr/bin/python3
for i in range(4000):
    print(i)

masahiro@zoe:/tmp$ ./test.py  |  head -n1
0
Traceback (most recent call last):
  File "/tmp/./test.py", line 3, in <module>
    print(i)
BrokenPipeError: [Errno 32] Broken pipe






This page
https://www.geeksforgeeks.org/broken-pipe-error-in-python/

suggests some workarounds.





Python scripts potentially have this issue.






$ ./scripts/diffconfig  .config.old  .config  | head -n1
-104_QUAD_8 m
Traceback (most recent call last):
  File "/home/masahiro/ref/linux/./scripts/diffconfig", line 132, in <module>
    main()
  File "/home/masahiro/ref/linux/./scripts/diffconfig", line 111, in main
    print_config("-", config, a[config], None)
  File "/home/masahiro/ref/linux/./scripts/diffconfig", line 62, in print_config
    print("-%s %s" % (config, value))
BrokenPipeError: [Errno 32] Broken pipe







What would you suggest for python scripts?
Kees Cook Dec. 6, 2022, 5:26 a.m. UTC | #7 
On December 5, 2022 8:24:41 PM PST, Masahiro Yamada <masahiroy@kernel.org> wrote:
>On Thu, Nov 17, 2022 at 7:07 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Thu, Nov 17, 2022 at 05:37:31AM +0900, Masahiro Yamada wrote:
>> > On Thu, Nov 17, 2022 at 4:01 AM Kees Cook <keescook@chromium.org> wrote:
>> > >
>> > > On Fri, Oct 28, 2022 at 01:28:39AM +0900, Masahiro Yamada wrote:
>> > > > Jiri Slaby reported that building the kernel with AR=gcc-ar shows:
>> > > >   /usr/bin/ar terminated with signal 13 [Broken pipe]
>> > > >
>> > > > Nathan Chancellor reported the latest AR=llvm-ar shows
>> > > >   error: write on a pipe with no reader
>> > > >
>> > > > The latter occurs since LLVM commit 51b557adc131 ("Add an error message
>> > > > to the default SIGPIPE handler").
>> > > >
>> > > > The resulting vmlinux is correct, but it is better to silence it.
>> > > >
>> > > > 'head -n1' exits after reading the first line, so the pipe is closed.
>> > > >
>> > > > Use 'sed -n 1p' to eat the stream till the end.
>> > >
>> > > I think this is wrong because it needlessly consumes CPU time. SIGPIPE
>> > > is _needed_ to stop a process after we found what we needed, but it's up
>> > > to the caller (the shell here) to determine what to do about it.
>> > >
>> > > Similarly, that LLVM commit is wrong -- tools should _not_ catch their
>> > > own SIGPIPEs. They should be caught by their callers.
>> > >
>> > > For example, see:
>> > >
>> > > $ seq 10000 | head -n1
>> > > 1
>> > >
>> > > ^^^ no warnings from the shell (caller of "seq")
>> > > And you can see it _is_ being killed by SIGPIPE:
>> > >
>> > > $ strace seq 1000 | head -n1
>> > > ...
>> > > write(1, "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14"..., 8192) = 8192
>> > > 1
>> > > write(1, "\n1861\n1862\n1863\n1864\n1865\n1866\n1"..., 4096) = -1 EPIPE (Broken pipe)
>> > > --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503448, si_uid=1000} ---
>> > > +++ killed by SIGPIPE +++
>> > >
>> > > If we use "sed -n 1p" seq will continue to run, consuming needless time
>> > > and CPU resources.
>> > >
>> > > So, I strongly think this is the wrong solution. SIGPIPE should be
>> > > ignored for ar, and LLVM should _not_ catch its own SIGPIPE.
>> > >
>> > > -Kees
>> >
>> >
>> > I thought of this - it is just wasting CPU time,
>> > but I did not come up with a better idea on the kbuild side.
>> >
>> > I do not want to use 2>/dev/null because it may hide
>> > non-SIGPIPE (i.e. real) errors.
>>
>> Yes, I've opened an upstream LLVM bug for this:
>> https://github.com/llvm/llvm-project/issues/59037
>>
>> --
>> Kees Cook
>
>
>
>BTW, Python does something similar by default.
>(noisy back-trace for SIGPIPE)
>
>
>
>
>
>masahiro@zoe:/tmp$ cat test.py
>#!/usr/bin/python3
>for i in range(4000):
>    print(i)
>
>masahiro@zoe:/tmp$ ./test.py  |  head -n1
>0
>Traceback (most recent call last):
>  File "/tmp/./test.py", line 3, in <module>
>    print(i)
>BrokenPipeError: [Errno 32] Broken pipe

Eww. Well, same problem, IMO. For any Python scripts that are going to have potentially truncated output, they need to do:

from signal import signal, SIGPIPE, SIG_DFL
signal(SIGPIPE,SIG_DFL)

>This page
>https://www.geeksforgeeks.org/broken-pipe-error-in-python/
>
>suggests some workarounds.

(As suggested in this page.)

>What would you suggest for python scripts?

They need to be fixed. A command line tool internally catching SIGPIPE is wrong. :)

-Kees
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index e90bb2b38607..e9e7eff906a5 100644
--- a/Makefile
+++ b/Makefile
@@ -1218,7 +1218,7 @@  quiet_cmd_ar_vmlinux.a = AR      $@
       cmd_ar_vmlinux.a = \
 	rm -f $@; \
 	$(AR) cDPrST $@ $(KBUILD_VMLINUX_OBJS); \
-	$(AR) mPiT $$($(AR) t $@ | head -n1) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
+	$(AR) mPiT $$($(AR) t $@ | sed -n 1p) $@ $$($(AR) t $@ | grep -F -f $(srctree)/scripts/head-object-list.txt)
 
 targets += vmlinux.a
 vmlinux.a: $(KBUILD_VMLINUX_OBJS) scripts/head-object-list.txt autoksyms_recursive FORCE