{"thread":{"id":"56911","subject":"Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3","startedAt":"2021-11-15T22:52:30Z","lastAt":"2021-11-19T21:28:33Z","messageCount":6,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"441169","messageId":"YZLhrSoTzrC7wcQo@coredump.intra.peff.net","threadId":"56911","inReplyTo":"211115.86a6i5s4bn.gmgdl@evledraar.gmail.com","subject":"Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-15T22:39:41Z","receivedAt":"2021-11-15T22:52:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 15, 2021 at 06:41:00PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This happens on \"master\", but is old, going back to at least v2.25.0 (I\n> didn't bother to test older versions):\n>     \n>     $ gcc --version\n>     gcc (Debian 11.2.0-10) 11.2.0\n>     [...]\n>     $ make refs.o CC=gcc CFLAGS=-O2\n>         * new build flags\n>         CC refs.o\n>     $ make refs.o CC=gcc CFLAGS=-O3\n>         * new build flags\n>         CC refs.o\n>     In file included from hashmap.h:4,\n>                      from cache.h:6,\n>                      from refs.c:5:\n>     In function ‘oidcpy’,\n>         inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,\n>         inlined from ‘ref_transaction_update’ at refs.c:1094:2,\n>         inlined from ‘ref_transaction_verify’ at refs.c:1132:9:\n>     hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull]\n>       262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);\n>           |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nThis seems like a false positive. In ref_transaction_update() we do:\n\n          if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n                  BUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n  \n          flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n\nSo we make sure that flags cannot have REF_HAVE_* in it (the allowed\nrefs are only bits 0 and 1, whereas the HAVE flags are bits 2 and 3).\nAnd then we set them based on the non-NULLness of the parameters. So no\nmatter what we got passed in, the two are in sync.\n\nAnd then we call add_update():\n\n          ref_transaction_add_update(transaction, refname, flags,\n\t\t\t\t     new_oid, old_oid, msg);\n\nwhich copies only if the flag is set:\n\n        if (flags & REF_HAVE_NEW)\n                oidcpy(&update->new_oid, new_oid);\n\nSo I don't see how this could go wrong. And the compiler should be able\nto figure that out, but only if it does enough analysis to understand\nthat the flags will always be cleared going into that \"|=\" line.\n\nWe can verify that the ALLOWED_FLAGS check works correctly by doing\nsomething stupid like:\n\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3..340d807066 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1131,7 +1131,7 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n \t\tBUG(\"verify called with old_oid set to NULL\");\n \treturn ref_transaction_update(transaction, refname,\n \t\t\t\t      NULL, old_oid,\n-\t\t\t\t      flags, NULL, err);\n+\t\t\t\t      flags | REF_HAVE_NEW, NULL, err);\n }\n \n int refs_update_ref(struct ref_store *refs, const char *msg,\n\nwhich is caught by the BUG() rather than segfaulting.\n\nSo I guess the compiler just isn't smart enough to consider the possible\nflag values. Which is not too surprising. But what's surprising to me is\nthat it insists that we hit the bogus memcpy(), even though it's gated\nbehind a conditional that it can't know whether it will be true or not.\n\nUnless I'm missing something?\n\n> It seems like the whole business of passing REF_HAVE_{NEW,OLD} is at\n> least partially redundant to simply checking the relevant\n> variables. Maybe.\n\nThere's definitely some Chesterton's Fence going on here. Once upon a\ntime we had a lot of bugs caused by confusing NULL and all-zero oids\n(where one meant \"I don't care what the old value was\" and the other was\n\"the old value must be that there was no ref at all\"). I think the flags\nare necessary within the \"struct ref_update\" because there it is no\nlonger holding pointers, but actual embedded structs.\n\nNow it could be that these days add_update() could check the pointers\nand decide whether to set the flags there itself. But it looks like\nthere are a couple of callers which rely on the flags being the source\nof truth. E.g.:\n\n  - in files_initial_transaction_commit(), we pass in old_oid but clear\n    the HAVE_OLD flag. I think this is just a minor bug-let that doesn't\n    affect anything currently, but we probably ought to be passing NULL.\n\n  - in split_head_update(), we propagate the flags from one struct to\n    another, but pass new_oid and old_oid unconditionally. If\n    add_update() starts caring about NULL-ness, then we'd have to decide\n    what to pass here.\n\n  - ditto in split_symref_update()\n\nSo something like the patch at the end of this email works (compiles\nwith -O3 and passes the tests), but I think it is just making things\nmore confusing.\n\n> I.e. our entire test suite passes with the patch below. We \"could\"\n> unconditionally check the variable name itself, but I haven't dug enough\n> to see if that's introducing a subtle bug we're not testing for, or just\n> a lot of redundant work.\n\nIt's not surprising that your patch doesn't trigger any test failures.\nAny case it would catch would have segfaulted already. And the cases I\nmentioned above are why you had to comment out the BUG in the final\nbranch here:\n\n> +\tif (new_oid && flags & REF_HAVE_NEW)\n>  \t\toidcpy(&update->new_oid, new_oid);\n> +\telse if (!new_oid && flags & REF_HAVE_NEW)\n> +\t\tBUG(\"would have passed NULL to memcpy() with REF_HAVE_NEW\");\n> +\telse if (new_oid && !(flags & REF_HAVE_NEW))\n> +\t\toidcpy(&update->new_oid, new_oid);\n> +\t\t//BUG(\"missed a memcpy() new_oid due to no REF_HAVE_NEW\");\n\nMy patch is below, but like I said, I think it makes things even more\nbrittle. I think an assertion similar to what you have above is a better\nbet, but perhaps written more simply as:\n\n  if (flags & REF_HAVE_NEW) {\n\t/* silence gcc 11's over-eager compile-time analysis */\n\tif (!new_oid)\n\t\tBUG(\"REF_HAVE_NEW set without passing new_oid\");\n\toidcpy(&update->new_oid, new_oid);\n  }\n\n---\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3..90c17118e5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1061,10 +1061,14 @@ struct ref_update *ref_transaction_add_update(\n \n \tupdate->flags = flags;\n \n-\tif (flags & REF_HAVE_NEW)\n+\tif (new_oid) {\n \t\toidcpy(&update->new_oid, new_oid);\n-\tif (flags & REF_HAVE_OLD)\n+\t\tupdate->flags |= REF_HAVE_NEW;\n+\t}\n+\tif (old_oid) {\n \t\toidcpy(&update->old_oid, old_oid);\n+\t\tupdate->flags |= REF_HAVE_OLD;\n+\t}\n \tupdate->msg = normalize_reflog_message(msg);\n \treturn update;\n }\n@@ -1089,8 +1093,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n \t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n \n-\tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n-\n \tref_transaction_add_update(transaction, refname, flags,\n \t\t\t\t   new_oid, old_oid, msg);\n \treturn 0;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 151b0056fe..39a57f6b5e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2281,7 +2281,8 @@ static int split_head_update(struct ref_update *update,\n \tnew_update = ref_transaction_add_update(\n \t\t\ttransaction, \"HEAD\",\n \t\t\tupdate->flags | REF_LOG_ONLY | REF_NO_DEREF,\n-\t\t\t&update->new_oid, &update->old_oid,\n+\t\t\tupdate->flags & REF_HAVE_NEW ? &update->new_oid : NULL,\n+\t\t\tupdate->flags & REF_HAVE_OLD ? &update->old_oid : NULL,\n \t\t\tupdate->msg);\n \n \t/*\n@@ -2344,7 +2345,8 @@ static int split_symref_update(struct ref_update *update,\n \n \tnew_update = ref_transaction_add_update(\n \t\t\ttransaction, referent, new_flags,\n-\t\t\t&update->new_oid, &update->old_oid,\n+\t\t\tnew_flags & REF_HAVE_NEW ? &update->new_oid : NULL,\n+\t\t\tnew_flags & REF_HAVE_OLD ? &update->old_oid : NULL,\n \t\t\tupdate->msg);\n \n \tnew_update->parent_update = update;\n@@ -3016,7 +3018,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \t\t */\n \t\tref_transaction_add_update(packed_transaction, update->refname,\n \t\t\t\t\t   update->flags & ~REF_HAVE_OLD,\n-\t\t\t\t\t   &update->new_oid, &update->old_oid,\n+\t\t\t\t\t   &update->new_oid, NULL,\n \t\t\t\t\t   NULL);\n \t}\n \n"},{"id":"441222","messageId":"211115.86a6i5s4bn.gmgdl@evledraar.gmail.com","threadId":"56911","inReplyTo":null,"subject":"New-ish warning in refs.c with GCC (at least 11.2) under -O3","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-15T17:41:00Z","receivedAt":"2021-11-16T01:42:42Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"(Sent earlier as <211115.86ee7hsa58.gmgdl@evledraar.gmail.com>, but I\ntypo'd the list address)\n\nThis happens on \"master\", but is old, going back to at least v2.25.0 (I\ndidn't bother to test older versions):\n    \n    $ gcc --version\n    gcc (Debian 11.2.0-10) 11.2.0\n    [...]\n    $ make refs.o CC=gcc CFLAGS=-O2\n        * new build flags\n        CC refs.o\n    $ make refs.o CC=gcc CFLAGS=-O3\n        * new build flags\n        CC refs.o\n    In file included from hashmap.h:4,\n                     from cache.h:6,\n                     from refs.c:5:\n    In function ‘oidcpy’,\n        inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,\n        inlined from ‘ref_transaction_update’ at refs.c:1094:2,\n        inlined from ‘ref_transaction_verify’ at refs.c:1132:9:\n    hash.h:262:9: error: argument 2 null where non-null expected [-Werror=nonnull]\n      262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);\n          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n    In file included from git-compat-util.h:177,\n                     from cache.h:4,\n                     from refs.c:5:\n    refs.c: In function ‘ref_transaction_verify’:\n    /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’\n       43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,\n          |              ^~~~~~\n    cc1: all warnings being treated as errors\n    make: *** [Makefile:2500: refs.o] Error 1\n\nI don't have time to dig into it right now, but if anyone's\ninterested...\n\nIt seems like the whole business of passing REF_HAVE_{NEW,OLD} is at\nleast partially redundant to simply checking the relevant\nvariables. Maybe.\n\nI.e. our entire test suite passes with the patch below. We \"could\"\nunconditionally check the variable name itself, but I haven't dug enough\nto see if that's introducing a subtle bug we're not testing for, or just\na lot of redundant work.\n\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3b..335244f756f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1061,10 +1061,22 @@ struct ref_update *ref_transaction_add_update(\n \n \tupdate->flags = flags;\n \n-\tif (flags & REF_HAVE_NEW)\n+\tif (new_oid && flags & REF_HAVE_NEW)\n \t\toidcpy(&update->new_oid, new_oid);\n-\tif (flags & REF_HAVE_OLD)\n+\telse if (!new_oid && flags & REF_HAVE_NEW)\n+\t\tBUG(\"would have passed NULL to memcpy() with REF_HAVE_NEW\");\n+\telse if (new_oid && !(flags & REF_HAVE_NEW))\n+\t\toidcpy(&update->new_oid, new_oid);\n+\t\t//BUG(\"missed a memcpy() new_oid due to no REF_HAVE_NEW\");\n+\n+\tif (old_oid && flags & REF_HAVE_OLD)\n \t\toidcpy(&update->old_oid, old_oid);\n+\telse if (!old_oid && flags & REF_HAVE_OLD)\n+\t\tBUG(\"would have passed NULL to memcpy() with REF_HAVE_OLD\");\n+\telse if (old_oid && !(flags & REF_HAVE_OLD))\n+\t\toidcpy(&update->old_oid, old_oid);\n+\t\t//BUG(\"missed a memcpy() old_oid due to no REF_HAVE_OLD\");\n+\n \tupdate->msg = normalize_reflog_message(msg);\n \treturn update;\n }\n"},{"id":"441359","messageId":"YZQUxkYI3TES3vDo@nand.local","threadId":"56911","inReplyTo":"YZLhrSoTzrC7wcQo@coredump.intra.peff.net","subject":"Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T20:29:58Z","receivedAt":"2021-11-16T20:30:01Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote:\n> So something like the patch at the end of this email works (compiles\n> with -O3 and passes the tests), but I think it is just making things\n> more confusing.\n\nI can absolutely understand and am sympathetic to the reasons that\nyour patch would be making things more brittle. In some sense, it makes\nspots like these a little easier to read:\n\n> -\t\t\t&update->new_oid, &update->old_oid,\n> +\t\t\tupdate->flags & REF_HAVE_NEW ? &update->new_oid : NULL,\n> +\t\t\tupdate->flags & REF_HAVE_OLD ? &update->old_oid : NULL,\n\nBut I think forcing that burden on every caller is what makes the\noverall approach worse.\n\nSo it's too bad that we even have this problem in the first place, since\nGCC's warning is clearly a false positive. But I would be OK with the\nbandaid you propose here:\n\n> I think an assertion similar to what you have above is a better bet,\n> but perhaps written more simply as:\n>\n>   if (flags & REF_HAVE_NEW) {\n> \t/* silence gcc 11's over-eager compile-time analysis */\n> \tif (!new_oid)\n> \t\tBUG(\"REF_HAVE_NEW set without passing new_oid\");\n> \toidcpy(&update->new_oid, new_oid);\n>   }\n\nThanks,\nTaylor\n"},{"id":"441365","messageId":"YZQhLh2BU5Hquhpo@coredump.intra.peff.net","threadId":"56911","inReplyTo":"YZQUxkYI3TES3vDo@nand.local","subject":"Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T21:22:54Z","receivedAt":"2021-11-16T21:22:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 03:29:58PM -0500, Taylor Blau wrote:\n\n> On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote:\n> > So something like the patch at the end of this email works (compiles\n> > with -O3 and passes the tests), but I think it is just making things\n> > more confusing.\n> \n> I can absolutely understand and am sympathetic to the reasons that\n> your patch would be making things more brittle. In some sense, it makes\n> spots like these a little easier to read:\n> \n> > -\t\t\t&update->new_oid, &update->old_oid,\n> > +\t\t\tupdate->flags & REF_HAVE_NEW ? &update->new_oid : NULL,\n> > +\t\t\tupdate->flags & REF_HAVE_OLD ? &update->old_oid : NULL,\n> \n> But I think forcing that burden on every caller is what makes the\n> overall approach worse.\n\nYeah, exactly.\n\n> So it's too bad that we even have this problem in the first place, since\n> GCC's warning is clearly a false positive. But I would be OK with the\n> bandaid you propose here:\n> \n> > I think an assertion similar to what you have above is a better bet,\n> > but perhaps written more simply as:\n> >\n> >   if (flags & REF_HAVE_NEW) {\n> > \t/* silence gcc 11's over-eager compile-time analysis */\n> > \tif (!new_oid)\n> > \t\tBUG(\"REF_HAVE_NEW set without passing new_oid\");\n> > \toidcpy(&update->new_oid, new_oid);\n> >   }\n\nI'm still OK with that. Another one that works and may be more obvious\nto somebody reading is:\n\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3..da39e9fb35 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1089,6 +1089,14 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n \t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n \n+\t/*\n+\t * Should be a noop per the ALLOWED_FLAGS check above, but this\n+\t * is necessary to work around a problem with some versions of\n+\t * \"gcc -O3 -Wnonnull\", which otherwise thinks that you can have the\n+\t * flag set with a NULL new_oid.\n+\t */\n+\tflags &= ~REF_HAVE_OLD | REF_HAVE_NEW;\n+\n \tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n \n \tref_transaction_add_update(transaction, refname, flags,\n\nI do find it interesting that gcc really _is_ convinced that those flags\ncan be set coming in, since clearing them makes the problem go away.\nThat points to it being confused by the \"ALLOWED_FLAGS\" check above.\nWhich makes me wonder if there's some weird integer rule at work here.\nBut even if I write:\n\n  if (flags & REF_HAVE_NEW)\n\tBUG(...);\n\nit can't figure it out. Which almost implies that it is not accepting\nthat BUG() will never return, but it's clearly marked with the noreturn\nattribute. So...I dunno.\n\nI noticed that Debian is carrying gcc-12 in experimental as of today. It\nreports the same problem, plus another one that I can't make heads or\ntails of:\n\n  dir.c: In function ‘git_url_basename’:\n  dir.c:3131:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread]\n   3131 |         if (memchr(start, '/', end - start) == NULL\n        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nReading over the code, it all looks OK. And that size is...weirdly huge.\n\n-Peff\n"},{"id":"441672","messageId":"xmqqwnl5ujxw.fsf@gitster.g","threadId":"56911","inReplyTo":"YZQhLh2BU5Hquhpo@coredump.intra.peff.net","subject":"Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T23:23:55Z","receivedAt":"2021-11-18T23:24:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\t/*\n> +\t * Should be a noop per the ALLOWED_FLAGS check above, but this\n> +\t * is necessary to work around a problem with some versions of\n> +\t * \"gcc -O3 -Wnonnull\", which otherwise thinks that you can have the\n> +\t * flag set with a NULL new_oid.\n> +\t */\n> +\tflags &= ~REF_HAVE_OLD | REF_HAVE_NEW;\n\nAre you missing parentheses around ~(OLD|NEW)?\n\n>  \tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n>  \n>  \tref_transaction_add_update(transaction, refname, flags,\n>\n> I do find it interesting that gcc really _is_ convinced that those flags\n> can be set coming in, since clearing them makes the problem go away.\n> ...\n> Reading over the code, it all looks OK. And that size is...weirdly huge.\n\nThe original bug is really annoying and this looks even worse.\nHopefully it won't come down from experimental to more stable tracks\nbefore they are corrected.\n\n"},{"id":"441793","messageId":"YZgW/pz6CbpaywDa@coredump.intra.peff.net","threadId":"56911","inReplyTo":"xmqqwnl5ujxw.fsf@gitster.g","subject":"[PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-19T21:28:30Z","receivedAt":"2021-11-19T21:28:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 18, 2021 at 03:23:55PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\t/*\n> > +\t * Should be a noop per the ALLOWED_FLAGS check above, but this\n> > +\t * is necessary to work around a problem with some versions of\n> > +\t * \"gcc -O3 -Wnonnull\", which otherwise thinks that you can have the\n> > +\t * flag set with a NULL new_oid.\n> > +\t */\n> > +\tflags &= ~REF_HAVE_OLD | REF_HAVE_NEW;\n> \n> Are you missing parentheses around ~(OLD|NEW)?\n\nWhoops, yes. Interesting that the compiler is still happy enough with it\nto prevent the warning (it does clear OLD, but not NEW). I also expected\nit to be a bug the tests would catch, but because of the OR it clears\nnothing except REF_HAVE_OLD.\n\nIt probably should just be spelled using the ALLOWED_FLAGS constant,\nthough. See the patch below.\n\n> > Reading over the code, it all looks OK. And that size is...weirdly huge.\n> \n> The original bug is really annoying and this looks even worse.\n> Hopefully it won't come down from experimental to more stable tracks\n> before they are corrected.\n\nYeah, I'm willing to ignore that one for now. But we probably should\ndeal with the gcc-11 one. Here's a cleaned-up version with a commit\nmessage.\n\n-- >8 --\nSubject: [PATCH] refs: work around gcc-11 warning with REF_HAVE_NEW\n\nUsing gcc-11 (or 12) to compile refs.o with -O3 results in:\n\n  In file included from hashmap.h:4,\n                   from cache.h:6,\n                   from refs.c:5:\n  In function ‘oidcpy’,\n      inlined from ‘ref_transaction_add_update’ at refs.c:1065:3,\n      inlined from ‘ref_transaction_update’ at refs.c:1094:2,\n      inlined from ‘ref_transaction_verify’ at refs.c:1132:9:\n  hash.h:262:9: warning: argument 2 null where non-null expected [-Wnonnull]\n    262 |         memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);\n        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n  In file included from git-compat-util.h:177,\n                   from cache.h:4,\n                   from refs.c:5:\n  refs.c: In function ‘ref_transaction_verify’:\n  /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared ‘nonnull’\n     43 | extern void *memcpy (void *__restrict __dest, const void *__restrict __src,\n        |              ^~~~~~\n\nThat call to memcpy() is in a conditional block that requires\nREF_HAVE_NEW to be set. But in ref_transaction_update(), we make sure it\nisn't set coming in:\n\n  if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n          BUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n\nand then only set it if the variable isn't NULL:\n\n  flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n\nSo it should be impossible to reach that memcpy() with a NULL oid. But\nfor whatever reason, gcc doesn't accept that hitting the BUG() means we\nwon't go any further, even though it's marked with the noreturn\nattribute. And the conditional is correct; ALLOWED_FLAGS doesn't contain\nHAVE_NEW or HAVE_OLD, and you can even simplify it to check for those\nflags explicitly and the compiler still complains.\n\nWe can work around this by just clearing the disallowed flags\nexplicitly. This should be a noop because of the BUG() check, but it\nmakes the compiler happy.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex d7cc0a23a3..33e8867a9c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1089,6 +1089,13 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)\n \t\tBUG(\"illegal flags 0x%x passed to ref_transaction_update()\", flags);\n \n+\t/*\n+\t * Clear flags outside the allowed set; this should be a noop because\n+\t * of the BUG() check above, but it works around a -Wnonnull warning\n+\t * with some versions of \"gcc -O3\".\n+\t */\n+\tflags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;\n+\n \tflags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);\n \n \tref_transaction_add_update(transaction, refname, flags,\n-- \n2.34.0.635.gde47f84164\n\n"}]}