On Wed, May 25 2022, Johannes Schindelin wrote: > Hi Junio, > > On Tue, 24 May 2022, Junio C Hamano wrote: > >> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> >> > On Tue, 24 May 2022, Junio C Hamano wrote: >> > >> >> The "clear slot->finished", by the way, is what I think is the right >> >> thing to do, especially that the objective is to squelch the false >> >> positive warning from a new compiler. If there is a way to annotate >> >> the line for the compiler to tell it not to warn about it, that would >> >> have been even better. >> > >> > We could do something like this: >> >> Yuck. >> >> > -- snip -- >> > diff --git a/http.c b/http.c >> > index b08795715f8a..2ac8d51d3668 100644 >> > --- a/http.c >> > +++ b/http.c >> > @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot) >> > struct timeval select_timeout; >> > int finished = 0; >> > >> > +#if __GNUC__ >= 12 >> > +#pragma GCC diagnostic push >> > +#pragma GCC diagnostic ignored "-Wdangling-pointer" >> > +#endif >> > slot->finished = &finished; >> > +#if __GNUC__ >= 12 >> > +#pragma GCC diagnostic pop >> > +#endif >> > while (!finished) { >> > step_active_slots(); >> > -- snap -- >> > >> > That's quite ugly, though. And what's worse, it is pretty unreadable, too. >> >> Yes, very ugly. Would an unconditional >> >> slot->finished = NULL; >> >> at the end squelch the warning? > > No, unfortunately not: > https://github.com/dscho/git/actions/runs/2383492484 Yes it does. Didn't you just have a PBCAK between writing that test & pushing it? Your https://github.com/dscho/git/blob/tmp/http.c#L1370-L1371 shows that you didn't make that edit. This on top of "seen" makes the warning go away: - if (slot->finished == &finished) - slot->finished = NULL; + slot->finished = NULL; This is also all covered in the initial thread from back in January, which if you're slowly re-discovering the learnings from there I encourage you to read in more detail... :) > As you mentioned elsewhere, the error is clearly about the assignment in > the first place, and it does not matter that the function will rectify the > situation. It's the correct thing to do for the compiler, too: since > `slot->finished` already has the pointer, and since the > `active_request_slot` struct is declared globally, code outside the > current file might squirrel that pointer away for later use. Per the above this isn't true, and in some side-thread replies (and in the initial thread) I've linked to the GCC code in question. NULL-ing the slot is sufficient, it doesn't matter that the struct it's in survives the function, just that the pointer isn't exposed. >> Or there is a way to say "we make all warnings into errors with >> -Werror, but we do not want to turn this dangling-pointer warning to >> an error, because it has false positives"? >> >> Or we could add "-Wno-dangling-pointer" globally, perhaps. > > I'd like to avoid that because it would quite likely hide legitimate > issues elsewhere. > > It currently seems to be the easiest solution to simply turn the local > variable into a heap variable: > > -- snip -- > diff --git a/http.c b/http.c > index f92859f43fa..0712debd558 100644 > --- a/http.c > +++ b/http.c > @@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot) > fd_set excfds; > int max_fd; > struct timeval select_timeout; > - int finished = 0; > + int *finished = xcalloc(1, sizeof(int)); > > - slot->finished = &finished; > - while (!finished) { > + slot->finished = finished; > + while (!*finished) { > step_active_slots(); > > if (slot->in_use) { > @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot) > select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout); > } > } > + if (slot->finished == finished) > + slot->finished = NULL; > + free(finished); > } Also, if we were going to tweak this extensively I'd think this slightly larger POC patch would be a better direction, i.e. we don't need a pointer at all, we're just (ab)using it for a tri-state NULL/0/1, why not just use an enum? I think the "if it's what we just set it to" is just cargo-culting, is there anything to show that run_active_slot() is reentrant? Wouldn't we be better off with a static variable + BUG() that we increment/decremant and panic if it's anything but 0 and 1 if we'd like to add paranoia around that? diff --git a/http-walker.c b/http-walker.c index b8f0f98ae14..26184a82ddb 100644 --- a/http-walker.c +++ b/http-walker.c @@ -225,13 +225,13 @@ static void process_alternates_response(void *callback_data) alt_req->url->buf); active_requests++; slot->in_use = 1; - if (slot->finished) - (*slot->finished) = 0; + if (slot->f != FIN_UNSET) + slot->f = FIN_NO; if (!start_active_slot(slot)) { cdata->got_alternates = -1; slot->in_use = 0; - if (slot->finished) - (*slot->finished) = 1; + if (slot->f != FIN_UNSET) + slot->f = FIN_YES; } return; } diff --git a/http.c b/http.c index b148468b267..845dd40768c 100644 --- a/http.c +++ b/http.c @@ -197,8 +197,8 @@ static void finish_active_slot(struct active_request_slot *slot) closedown_active_slot(slot); curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code); - if (slot->finished) - (*slot->finished) = 1; + if (slot->f != FIN_UNSET) + slot->f = FIN_YES; /* Store slot results so they can be read after the slot is reused */ if (slot->results) { @@ -1204,7 +1204,7 @@ struct active_request_slot *get_active_slot(void) active_requests++; slot->in_use = 1; slot->results = NULL; - slot->finished = NULL; + slot->f = FIN_UNSET; slot->callback_data = NULL; slot->callback_func = NULL; curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file); @@ -1327,10 +1327,9 @@ void run_active_slot(struct active_request_slot *slot) fd_set excfds; int max_fd; struct timeval select_timeout; - int finished = 0; - slot->finished = &finished; - while (!finished) { + slot->f = FIN_NO; + while (slot->f == FIN_NO) { step_active_slots(); if (slot->in_use) { diff --git a/http.h b/http.h index df1590e53a4..fc664d90bc9 100644 --- a/http.h +++ b/http.h @@ -19,12 +19,13 @@ struct slot_results { long http_connectcode; }; +enum fin { FIN_UNSET, FIN_NO, FIN_YES }; struct active_request_slot { CURL *curl; int in_use; CURLcode curl_result; long http_code; - int *finished; + enum fin f; struct slot_results *results; void *callback_data; void (*callback_func)(void *data);