On Fri, Mar 25 2022, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxx/
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.
