Tools should _not_ catch their own SIGPIPE

[kees]

Since commit 51b557a, tools are catching their own SIGPIPE signals. This is wrong: the caller needs to be the one handling these. This is the long-standing behavior for *nix utilities. For example:

$ seq 10000 | head -n1
1

No error generated, but seq is being killed by SIGPIPE:

$ strace seq 10000 | 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 +++

It is the caller (the shell, here) that is filtering out SIGPIPE. The tool itself should not catch the signal, and caller are expecting tools to be killed early by SIGPIPE so that needless CPU resources (and time) are not wasted. (For example, when using head, or grep -mN, etc.)

The SIGPIPE handler should be left explicitly untouched, i.e. set to SIG_DFL. If the error is wanted, callers (shells) can be configured to report it:

$ set -o pipefail
$ seq 10000 | head -n1
1
$ echo $?
141

The LLVM tool behavior is incorrect:

$ llvm-objdump -d /bin/true | head -n1

error: write on a pipe with no reader

And we can see that the signal is being caught internally:

$ strace llvm-objdump -d /bin/true | head -n1
...
write(1, "\n/bin/true:\tfile format elf64-x8"..., 95) = 95
write(1, "    1000: f3 0f 1e fa           "..., 48
) = -1 EPIPE (Broken pipe)
--- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=3503786, si_uid=1000} ---
...
rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fa15add7520}, NULL, 8) = 0
...
lseek(2, 0, SEEK_CUR)                   = -1 ESPIPE (Illegal seek)
newfstatat(2, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x19), ...}, AT_EMPTY_PATH) = 0
write(2, "error: write on a pipe with no r"..., 38error: write on a pipe with no reader
) = 38
exit_group(74)                          = ?
+++ exited with 74 +++

And errno 74 is also wrong here (EBADMSG). Again, setting pipefail can expose this wrong error too:

$ set -o pipefail
$ llvm-objdump -d /bin/true | head -n1
...
$ echo $?
74

But this errno is not correct. :(

[kees]

cc @nickdesaulniers @nathanchance @TNorthover

[nathanchance]

This appears to be the differential review but I am not sure the author is on GitHub: https://reviews.llvm.org/D124607

cc @jh7370

[nickdesaulniers]

What a mess. @TNorthover committed this without retaining authorship details... (make sure to use git commit --amend --author="Actual author <email>" when committing others' changes. Or better yet, use arcanist to fetch reviewed patches from phabricator including reviewed by tags. I'm also not happy that @TNorthover provided no response since my messages in June and again in October. Even if @TNorthover isn't on github, he is on phabricator...

cc @isanbard @bwendling @dmcyk

See also #25723 where we had these annoying warnings with clang; annoying for them to pop up again for the binutils substitutes.

51b557a landed in clang-15; cc @tstellar in case we want to consider a revert for a clang 15.0.6 release.
https://discourse.llvm.org/t/llvm-15-0-5-release/66616

[bwendling]

There's at least one situation in scripts/Makefile.build where Sami piped the error message from nm to /dev/null, I assume to avoid this (or similar) issue. It would be nice to have a generic way to do this, but would it be more trouble than it's worth?

[jh7370]

I don't know anything about Unix 03 conformance really - I approved on the basis that the referenced commit just prints a message to the error stream, and on the claim by the author that this is required.

A slight correction on words being used in this ticket though: strictly, LLVM already had a SIGPIPE handler before this patch, so technically, this patch didn't change the fact that LLVM tools "handle" SIGPIPEs (what they do as part of that handling is another matter of course).

I took a look through the Unix 03 standadds docs (as best I could - they aren't easy to navigate), and I couldn't find a reference to the thing being talked about in the original patch description. I'd therefore support this being reverted, if the original author and/or committer don't come back with the full standard citation. I wouldn't be surprised if there was a misinterpretation of the standards somewhere.

[dmcyk]

Thanks for bringing this up, the error message is indeed redundant and I'm sorry this is causing problems.
I checked internally with folks who work on UNIX03 conformance and my patch was overgeneralized, it's not necessary to print the error message on SIGPIPE, but specifically when there's an error on stdout stream in a normal program flow, e.g. when SIGPIPE trap is disabled. I don't currently have the full standard citation, but the behavior is verified in certification vendor's test suite and I can ask for the standard reference.

We specifically care about llvm-nm in this case, so I'm going to make a patch to handle this requirement only there too. I can remove the error message from the signal handler in the same patch, or if you want to already revert the change that's fine for me too, I have no commit access to do that myself.
I'd like to clarify the commit history though, 51b557a was reverted in 4badd4d and then a final patch was commited in b89bcef.

Like @jh7370 already noticed, the patch only added the error message though, the exit code was already there and I think it might be correct, the code (74) comes from sysexits(3).

[nathanchance]

@bwendling pushed a revert for review already. It would probably be easiest to go with a revert and new patch so that the revert can be cherry-picked to LLVM 15.0.6 easily, unless people think the new patch deserves to go too (perhaps that is up to @tstellar and co.).