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

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

[Ævar Arnfjörð Bjarmason]

(Sent earlier as <211115.86ee7hsa58.gmgdl@evledraar.gmail.com>, but I
typo'd the list address)

This happens on "master", but is old, going back to at least v2.25.0 (I
didn't bother to test older versions):
    
    $ gcc --version
    gcc (Debian 11.2.0-10) 11.2.0
    [...]
    $ make refs.o CC=gcc CFLAGS=-O2
        * new build flags
        CC refs.o
    $ make refs.o CC=gcc CFLAGS=-O3
        * new build flags
        CC refs.o
    In file included from hashmap.h:4,
                     from cache.h:6,
                     from refs.c:5:
    In function ‘oidcpy’,
        inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,
        inlined from ‘ref_transaction_update’ at refs.c:1094:2,
        inlined from ‘ref_transaction_verify’ at refs.c:1132:9:
    hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull]
      262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    In file included from git-compat-util.h:177,
                     from cache.h:4,
                     from refs.c:5:
    refs.c: In function ‘ref_transaction_verify’:
    /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’
       43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
          |              ^~~~~~
    cc1: all warnings being treated as errors
    make: *** [Makefile:2500: refs.o] Error 1

I don't have time to dig into it right now, but if anyone's
interested...

It seems like the whole business of passing REF_HAVE_{NEW,OLD} is at
least partially redundant to simply checking the relevant
variables. Maybe.

I.e. our entire test suite passes with the patch below. We "could"
unconditionally check the variable name itself, but I haven't dug enough
to see if that's introducing a subtle bug we're not testing for, or just
a lot of redundant work.

diff --git a/refs.c b/refs.c
index d7cc0a23a3b..335244f756f 100644
--- a/refs.c
+++ b/refs.c
@@ -1061,10 +1061,22 @@ struct ref_update *ref_transaction_add_update(
 
 	update->flags = flags;
 
-	if (flags & REF_HAVE_NEW)
+	if (new_oid && flags & REF_HAVE_NEW)
 		oidcpy(&update->new_oid, new_oid);
-	if (flags & REF_HAVE_OLD)
+	else if (!new_oid && flags & REF_HAVE_NEW)
+		BUG("would have passed NULL to memcpy() with REF_HAVE_NEW");
+	else if (new_oid && !(flags & REF_HAVE_NEW))
+		oidcpy(&update->new_oid, new_oid);
+		//BUG("missed a memcpy() new_oid due to no REF_HAVE_NEW");
+
+	if (old_oid && flags & REF_HAVE_OLD)
 		oidcpy(&update->old_oid, old_oid);
+	else if (!old_oid && flags & REF_HAVE_OLD)
+		BUG("would have passed NULL to memcpy() with REF_HAVE_OLD");
+	else if (old_oid && !(flags & REF_HAVE_OLD))
+		oidcpy(&update->old_oid, old_oid);
+		//BUG("missed a memcpy() old_oid due to no REF_HAVE_OLD");
+
 	update->msg = normalize_reflog_message(msg);
 	return update;
 }

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

[Jeff King]

On Mon, Nov 15, 2021 at 06:41:00PM +0100, Ævar Arnfjörð Bjarmason wrote:

> This happens on "master", but is old, going back to at least v2.25.0 (I
> didn't bother to test older versions):
>     
>     $ gcc --version
>     gcc (Debian 11.2.0-10) 11.2.0
>     [...]
>     $ make refs.o CC=gcc CFLAGS=-O2
>         * new build flags
>         CC refs.o
>     $ make refs.o CC=gcc CFLAGS=-O3
>         * new build flags
>         CC refs.o
>     In file included from hashmap.h:4,
>                      from cache.h:6,
>                      from refs.c:5:
>     In function ‘oidcpy’,
>         inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,
>         inlined from ‘ref_transaction_update’ at refs.c:1094:2,
>         inlined from ‘ref_transaction_verify’ at refs.c:1132:9:
>     hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull]
>       262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);
>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This seems like a false positive. In ref_transaction_update() we do:

          if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
                  BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
  
          flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);

So we make sure that flags cannot have REF_HAVE_* in it (the allowed
refs are only bits 0 and 1, whereas the HAVE flags are bits 2 and 3).
And then we set them based on the non-NULLness of the parameters. So no
matter what we got passed in, the two are in sync.

And then we call add_update():

          ref_transaction_add_update(transaction, refname, flags,
				     new_oid, old_oid, msg);

which copies only if the flag is set:

        if (flags & REF_HAVE_NEW)
                oidcpy(&update->new_oid, new_oid);

So I don't see how this could go wrong. And the compiler should be able
to figure that out, but only if it does enough analysis to understand
that the flags will always be cleared going into that "|=" line.

We can verify that the ALLOWED_FLAGS check works correctly by doing
something stupid like:

diff --git a/refs.c b/refs.c
index d7cc0a23a3..340d807066 100644
--- a/refs.c
+++ b/refs.c
@@ -1131,7 +1131,7 @@ int ref_transaction_verify(struct ref_transaction *transaction,
 		BUG("verify called with old_oid set to NULL");
 	return ref_transaction_update(transaction, refname,
 				      NULL, old_oid,
-				      flags, NULL, err);
+				      flags | REF_HAVE_NEW, NULL, err);
 }
 
 int refs_update_ref(struct ref_store *refs, const char *msg,

which is caught by the BUG() rather than segfaulting.

So I guess the compiler just isn't smart enough to consider the possible
flag values. Which is not too surprising. But what's surprising to me is
that it insists that we hit the bogus memcpy(), even though it's gated
behind a conditional that it can't know whether it will be true or not.

Unless I'm missing something?

> It seems like the whole business of passing REF_HAVE_{NEW,OLD} is at
> least partially redundant to simply checking the relevant
> variables. Maybe.

There's definitely some Chesterton's Fence going on here. Once upon a
time we had a lot of bugs caused by confusing NULL and all-zero oids
(where one meant "I don't care what the old value was" and the other was
"the old value must be that there was no ref at all"). I think the flags
are necessary within the "struct ref_update" because there it is no
longer holding pointers, but actual embedded structs.

Now it could be that these days add_update() could check the pointers
and decide whether to set the flags there itself. But it looks like
there are a couple of callers which rely on the flags being the source
of truth. E.g.:

  - in files_initial_transaction_commit(), we pass in old_oid but clear
    the HAVE_OLD flag. I think this is just a minor bug-let that doesn't
    affect anything currently, but we probably ought to be passing NULL.

  - in split_head_update(), we propagate the flags from one struct to
    another, but pass new_oid and old_oid unconditionally. If
    add_update() starts caring about NULL-ness, then we'd have to decide
    what to pass here.

  - ditto in split_symref_update()

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.e. our entire test suite passes with the patch below. We "could"
> unconditionally check the variable name itself, but I haven't dug enough
> to see if that's introducing a subtle bug we're not testing for, or just
> a lot of redundant work.

It's not surprising that your patch doesn't trigger any test failures.
Any case it would catch would have segfaulted already. And the cases I
mentioned above are why you had to comment out the BUG in the final
branch here:

> +	if (new_oid && flags & REF_HAVE_NEW)
>  		oidcpy(&update->new_oid, new_oid);
> +	else if (!new_oid && flags & REF_HAVE_NEW)
> +		BUG("would have passed NULL to memcpy() with REF_HAVE_NEW");
> +	else if (new_oid && !(flags & REF_HAVE_NEW))
> +		oidcpy(&update->new_oid, new_oid);
> +		//BUG("missed a memcpy() new_oid due to no REF_HAVE_NEW");

My patch is below, but like I said, I think it makes things even more
brittle. 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);
  }

---
diff --git a/refs.c b/refs.c
index d7cc0a23a3..90c17118e5 100644
--- a/refs.c
+++ b/refs.c
@@ -1061,10 +1061,14 @@ struct ref_update *ref_transaction_add_update(
 
 	update->flags = flags;
 
-	if (flags & REF_HAVE_NEW)
+	if (new_oid) {
 		oidcpy(&update->new_oid, new_oid);
-	if (flags & REF_HAVE_OLD)
+		update->flags |= REF_HAVE_NEW;
+	}
+	if (old_oid) {
 		oidcpy(&update->old_oid, old_oid);
+		update->flags |= REF_HAVE_OLD;
+	}
 	update->msg = normalize_reflog_message(msg);
 	return update;
 }
@@ -1089,8 +1093,6 @@ 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);
 
-	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
-
 	ref_transaction_add_update(transaction, refname, flags,
 				   new_oid, old_oid, msg);
 	return 0;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 151b0056fe..39a57f6b5e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2281,7 +2281,8 @@ static int split_head_update(struct ref_update *update,
 	new_update = ref_transaction_add_update(
 			transaction, "HEAD",
 			update->flags | REF_LOG_ONLY | REF_NO_DEREF,
-			&update->new_oid, &update->old_oid,
+			update->flags & REF_HAVE_NEW ? &update->new_oid : NULL,
+			update->flags & REF_HAVE_OLD ? &update->old_oid : NULL,
 			update->msg);
 
 	/*
@@ -2344,7 +2345,8 @@ static int split_symref_update(struct ref_update *update,
 
 	new_update = ref_transaction_add_update(
 			transaction, referent, new_flags,
-			&update->new_oid, &update->old_oid,
+			new_flags & REF_HAVE_NEW ? &update->new_oid : NULL,
+			new_flags & REF_HAVE_OLD ? &update->old_oid : NULL,
 			update->msg);
 
 	new_update->parent_update = update;
@@ -3016,7 +3018,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 		 */
 		ref_transaction_add_update(packed_transaction, update->refname,
 					   update->flags & ~REF_HAVE_OLD,
-					   &update->new_oid, &update->old_oid,
+					   &update->new_oid, NULL,
 					   NULL);
 	}
 

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

[Taylor Blau]

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.

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);
>   }

Thanks,
Taylor

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	/*
> +	 * 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;

Are you missing parentheses around ~(OLD|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.
> ...
> Reading over the code, it all looks OK. And that size is...weirdly huge.

The original bug is really annoying and this looks even worse.
Hopefully it won't come down from experimental to more stable tracks
before they are corrected.

[PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW

[Jeff King]

On Thu, Nov 18, 2021 at 03:23:55PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +	/*
> > +	 * 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;
> 
> Are you missing parentheses around ~(OLD|NEW)?

Whoops, yes. Interesting that the compiler is still happy enough with it
to prevent the warning (it does clear OLD, but not NEW). I also expected
it to be a bug the tests would catch, but because of the OR it clears
nothing except REF_HAVE_OLD.

It probably should just be spelled using the ALLOWED_FLAGS constant,
though. See the patch below.

> > Reading over the code, it all looks OK. And that size is...weirdly huge.
> 
> The original bug is really annoying and this looks even worse.
> Hopefully it won't come down from experimental to more stable tracks
> before they are corrected.

Yeah, I'm willing to ignore that one for now. But we probably should
deal with the gcc-11 one. Here's a cleaned-up version with a commit
message.

-- >8 --
Subject: [PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW

Using gcc-11 (or 12) to compile refs.o with -O3 results in:

  In file included from hashmap.h:4,
                   from cache.h:6,
                   from refs.c:5:
  In function ‘oidcpy’,
      inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,
      inlined from ‘ref_transaction_update’ at refs.c:1094:2,
      inlined from ‘ref_transaction_verify’ at refs.c:1132:9:
  hash.h:262:9: warning: argument 2 null where non-null expected [-Wnonnull]
    262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from git-compat-util.h:177,
                   from cache.h:4,
                   from refs.c:5:
  refs.c: In function ‘ref_transaction_verify’:
  /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’
     43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
        |              ^~~~~~

That call to memcpy() is in a conditional block that requires
REF_HAVE_NEW to be set. But in ref_transaction_update(), we make sure it
isn't set coming in:

  if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
          BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);

and then only set it if the variable isn't NULL:

  flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);

So it should be impossible to reach that memcpy() with a NULL oid. But
for whatever reason, gcc doesn't accept that hitting the BUG() means we
won't go any further, even though it's marked with the noreturn
attribute. And the conditional is correct; ALLOWED_FLAGS doesn't contain
HAVE_NEW or HAVE_OLD, and you can even simplify it to check for those
flags explicitly and the compiler still complains.

We can work around this by just clearing the disallowed flags
explicitly. This should be a noop because of the BUG() check, but it
makes the compiler happy.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/refs.c b/refs.c
index d7cc0a23a3..33e8867a9c 100644
--- a/refs.c
+++ b/refs.c
@@ -1089,6 +1089,13 @@ 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);
 
+	/*
+	 * Clear flags outside the allowed set; this should be a noop because
+	 * of the BUG() check above, but it works around a -Wnonnull warning
+	 * with some versions of "gcc -O3".
+	 */
+	flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
+
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
 	ref_transaction_add_update(transaction, refname, flags,
-- 
2.34.0.635.gde47f84164