All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.