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

From: Jeff King <peff@peff.net>
Date: Mar 8, 2025, 03:23 UTC
Message-ID: <20250308032309.GA584028@coredump.intra.peff.net>
In-Reply-To: <20250307225444.GA42758@coredump.intra.peff.net>

On Fri, Mar 07, 2025 at 05:54:45PM -0500, Jeff King wrote:

> However, clang does implement this option, and it finds the case
> mentioned above (and no other cases within the code base). And since we
> run clang in several of our CI jobs, that's enough to get an early
> warning of breakage.

Hmph, this might be more trouble than it is worth.

After correcting the problem in the refs code, the osx CI builds (and only those) now fail with:

  run-command.c:519:3: error: code will never be executed [-Werror,-Wunreachable-code]
                  die_errno("sigfillset");
                  ^~~~~~~~~

The code in question is just:

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

So I have to imagine that the issue is that sigfillset() on that platform is an inline or macro that will never return an error, and the compiler can see that. But since POSIX says this can fail (though I'd imagine it's unlikely on most platforms), we should check in the general case.

So I don't see how to solve it short of:

#ifdef SIGFILLSET_CANNOT_FAIL
	sigfillset(&all);
#else
	if (sigfillset(&all))
		die_errno("sigfillset");
#endif

which is rather ugly. It's only used in one spot, so the damage doesn't go too far, but I don't love the idea of getting surprised by the compiler over-analyzing system functions (and having to add Makefile knobs to support it).

I guess a knob-less version is:

  errno = 0;
  sigfillset(&all); /* don't check return value! only errno */
  if (errno)
	die_errno("sigfillset");

which is subtle, to say the least.

-Peff