Re: [PATCH] config.mak.dev: enable -Wunreachable-code

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2025, 17:27 UTC
Message-ID: <xmqqv7sbh698.fsf@gitster.g>
In-Reply-To: <20250314161347.GA9440@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> -- >8 --
> Subject: [PATCH] run-command: use errno to check for sigfillset() error
>
> 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.

Would it work to instead do this here

	if (sigfillset(&all) || false_but_compiler_does_not_know_it)
		die_error("sigfillset");

with

	extern int false_but_compiler_does_not_know_it;

in <git-compat-util.h>? And a standalone .c file with its definition

	#include <git-compat-util.h>
	int false_but_compiler_does_not_know_it;

and nothing else, linked into libgit.a?

I am hoping that such a false-positive would come from conditionals that are known to be compiler to be always taken (or never taken), so eventually we can mark such an expression with a macro, e.g.

	if (CAN_BE_TAKEN(sigfilllset(&all))
		die_error("sigfillset");

Because in this particular case we _can_ rely on errno, so the patch we see here is perfectly fine by me, but a more generic approach like the above would make it unnecessary to

 - have a 4-line comment
 - come up with workaround

suitable for each such places we need to work around compiler smarta^hness.

Show 38 quoted lines

> 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 <peff@peff.net>
> ---
>  run-command.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> 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))