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

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:

quoted

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:

quoted

-			&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:

quoted

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