archive mirror
 help / color / mirror / Atom feed
From: Jeff King <>
To: Taylor Blau <>
Cc: "Ævar Arnfjörð Bjarmason" <>,, "Carlo Arenas" <>,
	"brian m. carlson" <>
Subject: Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3
Date: Tue, 16 Nov 2021 16:22:54 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <YZQUxkYI3TES3vDo@nand.local>

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,
 		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)

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.


  reply	other threads:[~2021-11-16 21:22 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-15 17:41 New-ish warning in refs.c with GCC (at least 11.2) under -O3 Ævar Arnfjörð Bjarmason
2021-11-15 22:39 ` Jeff King
2021-11-16 20:29   ` Taylor Blau
2021-11-16 21:22     ` Jeff King [this message]
2021-11-18 23:23       ` Junio C Hamano
2021-11-19 21:28         ` [PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).