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);
