config.mak.dev: enable -Wunreachable-code

Commit Message

Jeff King March 7, 2025, 10:54 p.m. UTC

On Fri, Mar 07, 2025 at 12:46:27PM -0800, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
> >
> >> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
> >>  					    update->refname,
> >>  					    oid_to_hex(&update->old_oid));
> >>  				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
> >> +
> >> +				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> >> +					strbuf_setlen(err, 0);
> >> +					ret = 0;
> >> +					continue;
> >> +				}
> >> +
> >>  				goto error;
> >>  			}
> >>  		}
> >
> > This new code isn't reachable, since we return in the lines shown in the
> > diff context.
> >
> > Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
> > I think the "goto error" was already unreachable, so possibly the error
> > is in an earlier patch. (I didn't look; Coverity flagged this in the
> > final state in 'jch').
> 
> Sorry about that.  It shows that I lack the bandwidth necessary to
> go through fine toothed comb on all the topics I queue.  Perhaps I
> should be more selective and queue only the ones I personally had
> enough bandwidth to look over (or have seen clear "I looked each and
> every line of this series with fine toothed comb, put reviewed-by:
> me" messages sent by trusted reviewers) while ignoring others?

Eh, I would not worry about it too much. Things get missed, and that is
why we have many layers of reviews, static analysis, and ultimately
users to help us find bugs. ;)

I was disappointed that the compiler didn't complain, though. Maybe we
should do this:

-- >8 --
Subject: [PATCH] config.mak.dev: enable -Wunreachable-code

Having the compiler point out unreachable code can help avoid bugs, like
the one discussed in:

  https://lore.kernel.org/git/[e-mail]/

In that case it was found by Coverity, but finding it earlier saves
everybody time and effort.

We can use -Wunreachable-code to get some help from the compiler here.
Interestingly, this is a noop in gcc. It was a real warning up until gcc
4.x, when it was removed for being too flaky, but they left the
command-line option to avoid breaking users. See:

  https://stackoverflow.com/questions/17249934/why-does-gcc-not-warn-for-unreachable-code

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.

We could enable it only for clang, but since gcc is happy to ignore it,
it's simpler to just turn it on for all developer builds.

Signed-off-by: Jeff King <[e-mail]>
---
You can see it in action (merged into 'jch') here:

  https://github.com/peff/git/actions/runs/13729842188

where all of the clang jobs fail.

 config.mak.dev | 1 +
 1 file changed, 1 insertion(+)

Comments

Junio C Hamano March 7, 2025, 11:28 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> I was disappointed that the compiler didn't complain, though. Maybe we
> should do this:

Indeed.  It would have helped us if it were already there in place.

> -- >8 --
> Subject: [PATCH] config.mak.dev: enable -Wunreachable-code
>
> Having the compiler point out unreachable code can help avoid bugs, like
> the one discussed in:
>
>   https://lore.kernel.org/git/[e-mail]/
>
> In that case it was found by Coverity, but finding it earlier saves
> everybody time and effort.
>
> We can use -Wunreachable-code to get some help from the compiler here.
> Interestingly, this is a noop in gcc. It was a real warning up until gcc
> 4.x, when it was removed for being too flaky, but they left the
> command-line option to avoid breaking users. See:
>
>   https://stackoverflow.com/questions/17249934/why-does-gcc-not-warn-for-unreachable-code

Wow, now they leave their users confused, making them wondering why
their command line option does not do anything useful ;-)

> 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.

Yes, this is great.

Thanks.

Jeff King March 8, 2025, 3:23 a.m. UTC | #2

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

Junio C Hamano March 10, 2025, 3:40 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> 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");
>                   ^~~~~~~~~
> ...
> 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.

Bah.  This is just as horrible as some other warnings that are not
enabled by default.  I guess we should just be more vigilant X-<.

Thanks.

Jeff King March 10, 2025, 4:04 p.m. UTC | #4

On Mon, Mar 10, 2025 at 08:40:46AM -0700, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > 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");
> >                   ^~~~~~~~~
> > ...
> > 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.
> 
> Bah.  This is just as horrible as some other warnings that are not
> enabled by default.  I guess we should just be more vigilant X-<.

Yeah. We could perhaps live with hacking around this one specific spot.
But there's an open question of how often these kinds of false positives
will come up.

Maybe not often, if there is only one instance in the current code base.
Or maybe a lot, but we wouldn't know because we haven't had the warning
enabled.

I guess another option is to enable it in _one_ CI job that uses clang
on Linux (maybe linux-sha256?) and see how often it is helpful or
harmful.

-Peff

Junio C Hamano March 10, 2025, 6:50 p.m. UTC | #5

Jeff King <[e-mail]> writes:

> Maybe not often, if there is only one instance in the current code base.
> Or maybe a lot, but we wouldn't know because we haven't had the warning
> enabled.
>
> I guess another option is to enable it in _one_ CI job that uses clang
> on Linux (maybe linux-sha256?) and see how often it is helpful or
> harmful.

The reason why you said Linux rather than macOS is because the
single instance we know about would not have to be worked around if
we did it that way?

I am OK with that.  

Thanks.

Patch

diff --git a/config.mak.dev b/config.mak.dev
index 0fd8cc4d35..95b7bc46ae 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -39,6 +39,7 @@  DEVELOPER_CFLAGS += -Wunused
 DEVELOPER_CFLAGS += -Wvla
 DEVELOPER_CFLAGS += -Wwrite-strings
 DEVELOPER_CFLAGS += -fno-common
+DEVELOPER_CFLAGS += -Wunreachable-code
 
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
 DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare