Jeff King <peff@xxxxxxxx> writes:
> -- >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.
> 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@xxxxxxxx>
> ---
> 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))
