Re: [PATCH] http.c: clear the 'finished' member once we are done with it

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 24, 2022, 20:45 UTC
Message-ID: <220524.86mtf6ve89.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220524201639.2gucdkzponddk5qt@carlos-mbp.lan>

On Tue, May 24 2022, Carlo Marcelo Arenas Belón wrote:

Show 14 quoted lines

> On Tue, May 24, 2022 at 10:15:57AM -0700, Junio C Hamano wrote:
>> 
>> I _think_ we can even get away by not doing anything to
>> slot->finished at the end of run_active_slot(), as we are not
>> multi-threaded and the callee only returns to the caller, but if it
>> helps pleasing the warning compiler, I'd prefer the simplest
>> workaround, perhaps with an unconditional clearing there?
>
> Assuming that some overly clever compiler might optimize that out (either
> because it might think it is Undefined Behaviour or for other unknown
> reasons) then Ævar's version would be better for clearing the "warning".
>
> But your patch fixed the "bug" that a probably overeager compiler was
> "detecting".

Just briefly for those who perhaps didn't fully read the initial thread. Per [1] and [2] (search for -fanalyzer in that [2]) it's not a bug, undesired behavior etc. from GCC that it's "overeager" to warn in this case.

Most warnings from compilers are in the category of not being triggered on the basis of exhaustive code analysis which tries to prove that it's a practical issue for *your codebase*. It's equally about warning you about patterns that might be a problem in the future.

In this case I don't know how this line of reasoning started, or how the output is confusing.

E.g. Johannes notes upthread that the "complaint is not actually
accurate". Well, yes it is, because the warning says:
	
	http.c: In function ‘run_active_slot’:
	http.c:1332:24: warning: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Wdangling-pointer=]
	 1332 |         slot->finished = &finished;
	      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~

I.e. it's telling us that we're *storing* the address, which we're doing. "Storing" meaning "past the lifetime of the function".

It doesn't mean that GCC has additionally proved that we'll later used it in a way that will have a meaningful impact on the behavior of our program, or even that it's tried to do that. See an excerpt from the GCC code (a comment) in [1].

1. https://lore.kernel.org/git/220127.86mtjhdeme.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/220414.86h76vd69t.gmgdl@evledraar.gmail.com/