[v2,2/3] run-command: use errno to check for sigfillset() error

Commit Message

Junio C Hamano March 14, 2025, 9:09 p.m. UTC

From: Jeff King <[e-mail]>

Since enabling -Wunreachable-code, builds with clang on macOS now fail,
complaining that the die_errno() call in:

  if (sigfillset(&all))
	die_errno("sigfillset");

is unreachable. On that platform the manpage documents that sigfillset()
always returns success, and presumably the implementation is a macro or
inline function that does so in a way that is transparent to the
compiler.

But we should continue to check on other platforms, since POSIX says it
may return an error.

We could solve this with a compile-time knob to split the two cases
(assuming success on macOS and checking for the error elsewhere). But we
can also work around it more directly by relying on errno to check the
outcome (since POSIX dictates that errno will be set on error). And that
works around the compiler's cleverness, since it doesn't know the
semantics of errno (though I suppose if sigfillset() is simple enough,
it could perhaps realize that no writes to errno are possible; however
this does seem to work in practice).

Signed-off-by: Jeff King <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 run-command.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Taylor Blau March 17, 2025, 9:30 p.m. UTC | #1

On Fri, Mar 14, 2025 at 02:09:08PM -0700, Junio C Hamano wrote:
> From: Jeff King <[e-mail]>
>
> Since enabling -Wunreachable-code, builds with clang on macOS now fail,
> complaining that the die_errno() call in:
>
>   if (sigfillset(&all))
> 	die_errno("sigfillset");

Hmm. Would it have made sense to swap the order of this and the first
patch so we don't have a DEVELOPER=1 breakage (for macOS with Clang) in
history?

I think it's too late now since this topic is already on 'next', but it
occurred to me idly while reading this patch.

Thanks,
Taylor

Junio C Hamano March 17, 2025, 11:12 p.m. UTC | #2

Taylor Blau <[e-mail]> writes:

> On Fri, Mar 14, 2025 at 02:09:08PM -0700, Junio C Hamano wrote:
>> From: Jeff King <[e-mail]>
>>
>> Since enabling -Wunreachable-code, builds with clang on macOS now fail,
>> complaining that the die_errno() call in:
>>
>>   if (sigfillset(&all))
>> 	die_errno("sigfillset");
>
> Hmm. Would it have made sense to swap the order of this and the first
> patch so we don't have a DEVELOPER=1 breakage (for macOS with Clang) in
> history?
>
> I think it's too late now since this topic is already on 'next', but it
> occurred to me idly while reading this patch.

I thought db1d1f5d (config.mak.dev: enable -Wunreachable-code,
2025-03-14) aka jk/use-wunreachable-code-for-devs~2 is still out of
'next'?

Junio C Hamano March 18, 2025, 12:36 a.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> Taylor Blau <[e-mail]> writes:
>
>> On Fri, Mar 14, 2025 at 02:09:08PM -0700, Junio C Hamano wrote:
>>> From: Jeff King <[e-mail]>
>>>
>>> Since enabling -Wunreachable-code, builds with clang on macOS now fail,
>>> complaining that the die_errno() call in:
>>>
>>>   if (sigfillset(&all))
>>> 	die_errno("sigfillset");
>>
>> Hmm. Would it have made sense to swap the order of this and the first
>> patch so we don't have a DEVELOPER=1 breakage (for macOS with Clang) in
>> history?
>>
>> I think it's too late now since this topic is already on 'next', but it
>> occurred to me idly while reading this patch.
>
> I thought db1d1f5d (config.mak.dev: enable -Wunreachable-code,
> 2025-03-14) aka jk/use-wunreachable-code-for-devs~2 is still out of
> 'next'?

Ah, I did revert an earlier one-commit topic out of 'next'.  Perhaps
I didn't tell What's cooking about it.

Patch

diff --git a/run-command.c b/run-command.c
index 402138b8b5..d527c46175 100644
--- a/run-command.c
+++ b/run-command.c
@@ -515,7 +515,15 @@  static void atfork_prepare(struct atfork_state *as)
 {
 	sigset_t all;
 
-	if (sigfillset(&all))
+	/*
+	 * Do not use the return value of sigfillset(). It is transparently 0
+	 * on some platforms, meaning a clever compiler may complain that
+	 * the conditional body is dead code. Instead, check for error via
+	 * errno, which outsmarts the compiler.
+	 */
+	errno = 0;
+	sigfillset(&all);
+	if (errno)
 		die_errno("sigfillset");
 #ifdef NO_PTHREADS
 	if (sigprocmask(SIG_SETMASK, &all, &as->old))