New-ish warning in refs.c with GCC (at least 11.2) under -O3
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-16 01:42:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
(Sent earlier as [off-list ref], 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; }