Re: Re* jc/http-clear-finished-pointer

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 1, 2022, 07:26 UTC
Message-ID: <220601.86pmjs2679.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq7d68ytj8.fsf_-_@gitster.g>

On Thu, May 26 2022, Junio C Hamano wrote:

Show 38 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, May 26 2022, Junio C Hamano wrote:
>>
>>> * jc/http-clear-finished-pointer (2022-05-24) 1 commit
>>>  - http.c: clear the 'finished' member once we are done with it
>>>
>>>  Meant to go with js/ci-gcc-12-fixes
>>>
>>>  Will merge to 'next'?
>>>  source: <xmqqczgqjr8y.fsf_-_@gitster.g>
>>
>> The end of the proposed commit message says:
>>
>>     [...]Clear the finished member before the control leaves the
>>     function, which has a side effect of unconfusing compilers like
>>     recent GCC 12 that is over-eager to warn against such an assignment.
>>
>> I cannot reproduce this suppressing the warning as noted in past
>> exchanges, it's not affected by this "clear if we set it" pattern. It
>> needs to be unconditionally cleared.
>
> Interesting.  I still have conditional clearing in the tree, though
> I was reasonably sure I got rid of the conditional and made it
> always clear, when I rewrote that part of the log message.  After
> all, I ran "commit --amend" so that I do not forget the issue after
> sending https://lore.kernel.org/git/xmqqleurlt31.fsf@gitster.g/ X-<.
>
> Thanks for catching.  What is queued is not what I intended to
> queue.
>
> But there is one thing that is puzzling.  Ever since this, together
> with the three patches from Dscho for gcc12, got included in 'seen',
> the branch started passing the Windows build that used to complain
> and did not work, so at least with the version of gcc12 used over
> there, it apparently is sufficient to clear only when we are
> responsible for placing an address that is about to become invalid,
> while leaving the pointer we didn't stuff in unmodified.

I didn't find what specific build(s) you were referring to, but perhaps this is due to an interaction with 9c539d1027d (config.mak.dev: alternative workaround to gcc 12 warning in http.c, 2022-04-15)? I.e. with DEVELOPER=1 we'd already get past the warning with gcc+Windows, presumably (but I haven't confirmed whether the detect-compiler etc. works the same there).

Show 14 quoted lines

> As far as I understand, with the most recent analysis by Dscho on
> the http-push codepath, we can return to the loop while the slot is
> holding a different request that is unrelated to ours that has
> already finished without recursively calling run_active_slot(), and
> with the current *(slot->finished)=1 trick, it will successfully
> notify our loop that our request is done, even though slot->in_use
> is set to true back again when it happens.  But by definition, at
> that point, slot->finished is not used by anybody (obviously not by
> us, but also not by the request that is currently using the slot,
> because it hasn't used run_active_slot() and slot->finished is not
> touched by it), so it is safe to unconditionally clear the member.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH v3] http.c: clear the 'finished' member once we are done with it

I see this is in "next" already, as a follow-up we should have a revert of 9c539d1027d (config.mak.dev: alternative workaround to gcc 12 warning in http.c, 2022-04-15) which this patch makes redundant.