git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: Carlo Arenas <carenas@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Jeff King <peff@peff.net>
Subject: New-ish warning in refs.c with GCC (at least 11.2) under -O3
Date: Mon, 15 Nov 2021 18:41:00 +0100	[thread overview]
Message-ID: <211115.86a6i5s4bn.gmgdl@evledraar.gmail.com> (raw)

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

             reply	other threads:[~2021-11-16  1:42 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-15 17:41 Ævar Arnfjörð Bjarmason [this message]
2021-11-15 22:39 ` New-ish warning in refs.c with GCC (at least 11.2) under -O3 Jeff King
2021-11-16 20:29   ` Taylor Blau
2021-11-16 21:22     ` Jeff King
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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=211115.86a6i5s4bn.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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).