Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3

[Jeff King <peff@peff.net>]

On Tue, Nov 16, 2021 at 03:29:58PM -0500, Taylor Blau wrote:

> On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote:
> > So something like the patch at the end of this email works (compiles
> > with -O3 and passes the tests), but I think it is just making things
> > more confusing.
> 
> I can absolutely understand and am sympathetic to the reasons that
> your patch would be making things more brittle. In some sense, it makes
> spots like these a little easier to read:
> 
> > -			&update->new_oid, &update->old_oid,
> > +			update->flags & REF_HAVE_NEW ? &update->new_oid : NULL,
> > +			update->flags & REF_HAVE_OLD ? &update->old_oid : NULL,
> 
> But I think forcing that burden on every caller is what makes the
> overall approach worse.

Yeah, exactly.

> So it's too bad that we even have this problem in the first place, since
> GCC's warning is clearly a false positive. But I would be OK with the
> bandaid you propose here:
> 
> > I think an assertion similar to what you have above is a better bet,
> > but perhaps written more simply as:
> >
> >   if (flags & REF_HAVE_NEW) {
> > 	/* silence gcc 11's over-eager compile-time analysis */
> > 	if (!new_oid)
> > 		BUG("REF_HAVE_NEW set without passing new_oid");
> > 	oidcpy(&update->new_oid, new_oid);
> >   }

I'm still OK with that. Another one that works and may be more obvious
to somebody reading is:

diff --git a/refs.c b/refs.c
index d7cc0a23a3..da39e9fb35 100644
--- a/refs.c
+++ b/refs.c
@@ -1089,6 +1089,14 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
 		BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
 
+	/*
+	 * Should be a noop per the ALLOWED_FLAGS check above, but this
+	 * is necessary to work around a problem with some versions of
+	 * "gcc -O3 -Wnonnull", which otherwise thinks that you can have the
+	 * flag set with a NULL new_oid.
+	 */
+	flags &= ~REF_HAVE_OLD | REF_HAVE_NEW;
+
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
 	ref_transaction_add_update(transaction, refname, flags,

I do find it interesting that gcc really _is_ convinced that those flags
can be set coming in, since clearing them makes the problem go away.
That points to it being confused by the "ALLOWED_FLAGS" check above.
Which makes me wonder if there's some weird integer rule at work here.
But even if I write:

  if (flags & REF_HAVE_NEW)
	BUG(...);

it can't figure it out. Which almost implies that it is not accepting
that BUG() will never return, but it's clearly marked with the noreturn
attribute. So...I dunno.

I noticed that Debian is carrying gcc-12 in experimental as of today. It
reports the same problem, plus another one that I can't make heads or
tails of:

  dir.c: In function ‘git_url_basename’:
  dir.c:3131:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread]
   3131 |         if (memchr(start, '/', end - start) == NULL
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reading over the code, it all looks OK. And that size is...weirdly huge.

-Peff