On Thu, May 26 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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). > 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.
