Re: [PATCH] http.c: clear the 'finished' member once we are done with it
- From
Carlo Marcelo Arenas Belón <carenas@gmail.com>
- Date
- May 7, 2022, 05:40 UTC
- Message-ID
- <20220507054017.fnvb6xisr6s7m2l5@carlos-mbp.lan>
- In-Reply-To
- <xmqqczgqjr8y.fsf_-_@gitster.g>
On Fri, May 06, 2022 at 02:17:01PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> diff --git a/http.c b/http.c > index 229da4d148..85437b1980 100644 > --- a/http.c > +++ b/http.c > @@ -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;
I am not completely sure yet (since I looked at it long ago and got sidetracked) but I think this might be optimized out (at least by gcc12) since it is technically UB, which is why it never "fixed" the warning.
the "correct" way to implement this would be to make "finished" a thread local static, which is finally one good reason to support C99, but the syntax to do so with Windows broke my first attempt at doing so and now I can even find the code I used then which required a per platform macro and was better looking than the following
Carlo --- >8 ---- Date: Wed, 20 Apr 2022 23:25:55 -0700 Subject: [PATCH] http: avoid using out of scope pointers
baa7b67d091 (HTTP slot reuse fixes, 2006-03-10) introduces a way to notify a curl thread that its slot is finished by using a pointer to a stack variable from run_active_slot(), but then gcc 12 was released and started rightfully to complain about it (-Wdangling-pointer).
Use instead a thread storage static variable which is safe to use between threads since C99 and doesn't go out of scope, while being functionally equivalent to the original code, and also remove the workaround from 9c539d1027d (config.mak.dev: alternative workaround to gcc 12 warning in http.c, 2022-04-15)
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- config.mak.dev | 1 - http.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/config.mak.dev b/config.mak.dev index c3104f400b..335efd4620 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -68,7 +68,6 @@ endif # https://bugzilla.redhat.com/show_bug.cgi?id=2075786 ifneq ($(filter gcc12,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wno-error=stringop-overread -DEVELOPER_CFLAGS += -Wno-error=dangling-pointer endif GIT_TEST_PERL_FATAL_WARNINGS = YesPlease diff --git a/http.c b/http.c index 229da4d148..cb9acfca19 100644 --- a/http.c +++ b/http.c @@ -1327,7 +1327,7 @@ void run_active_slot(struct active_request_slot *slot) fd_set excfds; int max_fd; struct timeval select_timeout; - int finished = 0; + static __thread int finished; slot->finished = &finished; while (!finished) {
-- 2.36.0.352.g0cd7feaf86f