From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>, git@vger.kernel.org
Subject: Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
Date: Thu, 14 Apr 2022 17:27:30 +0200 [thread overview]
Message-ID: <220414.86h76vd69t.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqy20x7eqv.fsf@gitster.g>
On Fri, Mar 25 2022, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
>> On Fri, Mar 25, 2022 at 03:34:49PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>> This isn't the only caller that assigns to "slot->finished", see see
>>
>> s/see see/see ?
>>
>>> the assignments in http-walker.c:process_alternates_response() and
>>> http.c:finish_active_slot().
>>>
>>> But those assignments are both to the pointer to our local variable
>>> here, so this fix is correct. The only way that code in http-walker.c
>>> could have done its assignments is to the pointer to this specific
>>> variable.
>>
>> Got it; this is the key piece that I was missing in my earlier review.
>> Sorry about that, this looks completely safe to me now.
>
> It does not exactly look safe to me, though.
>
> With a bit of rewrite, here is what I'd queue for now. I really
> hope that the pre-release compiler will be fixed soon so that we do
> not have to worry about this patch.
Late reply, sorry.
I don't think the compiler is broken in this case.
Having spelunked in the GCC docs, source, commits involved & in their
bugtracker I don't think they'd consider this broken. It's working as
designed.
Now, of course as with any new compiler warning you might argue that
it's too overzealous, but in this case it's included it a -Wall in GCC
12.0.
The warning is specifically about the local pointer being stored in a
structure that survives the exit of function, and therefore if you were
to dereference that pointer after the function exists the results are
undefined.
Now, we happen to be able to carefully read the code in this case and
reason that we won't be looking at it except in the code that's called
within this function, so in practice we're OK.
But we'd have a bug sneak up on us if that wasn't the case, so it's
safer to NULL this out, which is exactly what the new GCC warning is
concerning itself with.
I.e. it's not promising to narrow itself to only those cases where GCC
can know for absolute certain that it's a *practical* issue.
Which, basically would be asking for it to do as much work to emit it as
it has to do on -fanalyzer, which makes compilation of git.git take
somewhere in the mid-range of double digit minutes for me, instead of
~20s.
So I'd prefer:
1. Adjust for release etc., but that what I submitted in
https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/
go in as-is.
2. If you're convinced this is a compiler bug could you please either
drop the current version, or rewrite the commit envelope so that
you're the author?
I very much appreciate when you do local commit fix-ups for
rewording, typos, or even rewriting something I did for the better.
But in this case it's got my name on the envelope & has been
reworded to say pretty much the exact opposite of what I
think/believe about this warning.
Which isn't something wrong or not permitted by the
license/SOB/whatever.
I'd just find it confusing when I'll dig this up at some point in
the future to find my name on it, read the explanation, and then be
perplexed why I ever thought that about this particular warning
... :)
Thanks.
next prev parent reply other threads:[~2022-04-14 16:13 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-26 21:30 [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 Ævar Arnfjörð Bjarmason
2022-01-26 21:59 ` Taylor Blau
2022-01-27 0:50 ` Junio C Hamano
2022-01-27 0:57 ` Junio C Hamano
2022-01-27 3:45 ` Ævar Arnfjörð Bjarmason
2022-01-27 18:23 ` Junio C Hamano
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-02-25 22:58 ` Junio C Hamano
2022-02-26 18:01 ` Taylor Blau
2022-03-25 14:34 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2022-03-25 18:11 ` Taylor Blau
2022-03-26 0:13 ` Junio C Hamano
2022-04-14 15:27 ` Ævar Arnfjörð Bjarmason [this message]
2022-04-14 17:04 ` Junio C Hamano
2022-04-15 13:30 ` Ævar Arnfjörð Bjarmason
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220414.86h76vd69t.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).