{"thread":{"id":"57858","subject":"[PATCH 1/2] dir.c: avoid gcc warning","startedAt":"2022-05-06T18:06:49Z","lastAt":"2022-05-26T14:15:20Z","messageCount":38,"participants":["Michael J Gruber","Junio C Hamano","Carlo Marcelo Arenas Belón","Carlo Arenas","Taylor Blau","rsbecker@nexbridge.com","Johannes Schindelin","Daniel Stenberg","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"454930","messageId":"cd50ec73ddafaaeba04298ae79cbf625cc0d7697.1651859773.git.git@grubix.eu","threadId":"57858","inReplyTo":"cover.1651859773.git.git@grubix.eu","subject":"[PATCH 1/2] dir.c: avoid gcc warning","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-05-06T18:04:05Z","receivedAt":"2022-05-06T18:06:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Related to -Wstringop-overread.\n\nIn fact, this may be a false positive, but reading until the correct end\nis desirable here anyways.\n\nSigned-off-by: Michael J Gruber \n---\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex 26c4d141ab..32fcaae4c0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3145,7 +3145,7 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)\n \t * result in a dir '2222' being guessed due to backwards\n \t * compatibility.\n \t */\n-\tif (memchr(start, '/', end - start) == NULL\n+\tif (memchr(start, '/', end - start + 1) == NULL\n \t && memchr(start, ':', end - start) != NULL) {\n \t\tptr = end;\n \t\twhile (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')\n-- \n2.36.0.553.g068b50827d\n\n"},{"id":"454931","messageId":"3f0e462e86625a3c253653e4a4eefabcd8590bf9.1651859773.git.git@grubix.eu","threadId":"57858","inReplyTo":"cover.1651859773.git.git@grubix.eu","subject":"[PATCH 2/2] http.c: avoid gcc warning","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-05-06T18:04:06Z","receivedAt":"2022-05-06T18:07:22Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Related to -Wdangling-pointer.\n\nIn fact, this use of the pointer looks scary and has not created\nproblems so far only because the pointer in the struct is not used when\nexecution is out of the scope of the local function (and the pointer\ninvalid).\n\nSigned-off-by: Michael J Gruber \n---\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex 229da4d148..2f67fbb33c 100644\n--- a/http.c\n+++ b/http.c\n@@ -1367,6 +1367,7 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n+\tslot->finished = NULL;\n }\n \n static void release_active_slot(struct active_request_slot *slot)\n-- \n2.36.0.553.g068b50827d\n\n"},{"id":"454932","messageId":"cover.1651859773.git.git@grubix.eu","threadId":"57858","inReplyTo":null,"subject":"[PATCH 0/2] quell a few gcc warnings","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-05-06T18:04:04Z","receivedAt":"2022-05-06T18:12:15Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"New Fedora, new gcc, new warnings. This mini series quells them.\n\nMichael J Gruber (2):\n dir.c: avoid gcc warning\n http.c: avoid gcc warning\n\n dir.c | 2 +-\n http.c | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\n-- \n2.36.0.553.g068b50827d\n\n"},{"id":"454941","messageId":"xmqqy1zejtte.fsf@gitster.g","threadId":"57858","inReplyTo":"cd50ec73ddafaaeba04298ae79cbf625cc0d7697.1651859773.git.git@grubix.eu","subject":"Re: [PATCH 1/2] dir.c: avoid gcc warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T20:21:33Z","receivedAt":"2022-05-06T20:21:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Related to -Wstringop-overread.\n>\n> In fact, this may be a false positive, but reading until the correct end\n> is desirable here anyways.\n\nBut the correct end is start + (end - start), not start + (end -\nstart + 1), isn't it? We've stripped trailing junk like /.git and\nend is point at one byte beyond the end of URL to the repository.\n\nE.g. for \"https://auth@host/\", we have advanced start to point at\n\"h\" at the beginning of \"host\", and we have moved end back from\npointing at the NUL at the end to point at \"/\" at the end of\n\"host/\".\n\nWe are trying to make sure that the resulting \"host\" string between\nstart and end do not have a slash to apply this special case.\n\nIf the original URL were \"https://auth@host:4321/\", the end points\nat \"/\" at the end of \"host:4321/\", making the string to be checked\nto \"host:4321\" and we are trying to see it has no '/' in it (which\nis the case). By extending the string by one, memchr() will see the\n'/' at the end that is outside.\n\nThis seems to be a behaviour breaking change and I am not sure what\nwe are trying to achieve with it. Is this a suggestion made by a\nbroken compiler you have, or something?\n\nPuzzled....\n\n> Signed-off-by: Michael J Gruber \n> ---\n> dir.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 26c4d141ab..32fcaae4c0 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -3145,7 +3145,7 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)\n> \t * result in a dir '2222' being guessed due to backwards\n> \t * compatibility.\n> \t */\n> -\tif (memchr(start, '/', end - start) == NULL\n> +\tif (memchr(start, '/', end - start + 1) == NULL\n> \t && memchr(start, ':', end - start) != NULL) {\n> \t\tptr = end;\n> \t\twhile (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':')\n"},{"id":"454943","messageId":"xmqqtua2jtr0.fsf@gitster.g","threadId":"57858","inReplyTo":"3f0e462e86625a3c253653e4a4eefabcd8590bf9.1651859773.git.git@grubix.eu","subject":"Re: [PATCH 2/2] http.c: avoid gcc warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T20:22:59Z","receivedAt":"2022-05-06T20:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\nSee previous discussion on the topic and help clarify it for me,\nthanks.\n\nhttps://lore.kernel.org/git/xmqqo8131tr8.fsf@gitster.g/\n\n> Related to -Wdangling-pointer.\n>\n> In fact, this use of the pointer looks scary and has not created\n> problems so far only because the pointer in the struct is not used when\n> execution is out of the scope of the local function (and the pointer\n> invalid).\n>\n> Signed-off-by: Michael J Gruber \n> ---\n> http.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/http.c b/http.c\n> index 229da4d148..2f67fbb33c 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1367,6 +1367,7 @@ void run_active_slot(struct active_request_slot *slot)\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n> +\tslot->finished = NULL;\n> }\n> \n> static void release_active_slot(struct active_request_slot *slot)\n"},{"id":"454945","messageId":"20220506204102.iyn7mxogtz2t7gh6@carlos-mbp.lan","threadId":"57858","inReplyTo":"3f0e462e86625a3c253653e4a4eefabcd8590bf9.1651859773.git.git@grubix.eu","subject":"Re: [PATCH 2/2] http.c: avoid gcc warning","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-06T20:41:02Z","receivedAt":"2022-05-06T20:41:14Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, May 06, 2022 at 08:04:06PM +0200, Michael J Gruber wrote:\n> Related to -Wdangling-pointer.\n> \n> In fact, this use of the pointer looks scary and has not created\n> problems so far only because the pointer in the struct is not used when\n> execution is out of the scope of the local function (and the pointer\n> invalid).\n\nI think it might had been used by a different thread though and therefore\nit should be at least a thread local static to be safe (which should be\npossible to do now that we are supporting C99).\n\nIf you are going that route, would be important to tell you that I tried\nand got in trouble because of Windows and the build environment in use\nthere, but it didn't seem that difficult to fix, before I got sidetracked.\n\nCarlo\n"},{"id":"454950","messageId":"xmqqczgqjr8y.fsf_-_@gitster.g","threadId":"57858","inReplyTo":"xmqqtua2jtr0.fsf@gitster.g","subject":"[PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T21:17:01Z","receivedAt":"2022-05-06T21:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In http.c, the run_active_slot() function allows the given \"slot\" to\nmake progress by calling step_active_slots() in a loop repeatedly,\nand the loop is not left until the request held in the slot\ncompletes.\n\nAges ago, we used to use the slot->in_use member to get out of the\nloop, which misbehaved when the request in \"slot\" completes (at\nwhich time, the result of the request is copied away from the slot,\nand the in_use member is cleared, making the slot ready to be\nreused), and the \"slot\" gets reused to service a different request\n(at which time, the \"slot\" becomes in_use again, even though it is\nfor a different request). The loop terminating condition mistakenly\nthought that the original request has yet to be completed.\n\nToday's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10)\nfixed this issue, uses a separate \"slot->finished\" member that is\nset in run_active_slot() to point to an on-stack variable, and the\ncode that completes the request in finish_active_slot() clears the\non-stack variable via the pointer to signal that the particular\nrequest held by the slot has completed. It also clears the in_use\nmember (as before that fix), so that the slot itself can safely be\nreused for an unrelated request.\n\nOne thing that is not quite clean in this arrangement is that,\nunless the slot gets reused, at which point the finished member is\nreset to NULL, the member keeps the value of &finished, which\nbecomes a dangling pointer into the stack when run_active_slot()\nreturns. Clear the finished member before the control leaves the\nfunction, but make sure to limit it to the case where the pointer\nstill points at the on-stack variable of ours (the pointer may be\nset to point at the on-stack variable of somebody else after the\nslot gets reused, in which case we do not want to touch it).\n\nSigned-off-by: Junio C Hamano \n---\n\n * So, this has been sitting in my pile of random patches for a few\n weeks.\n\n http.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 229da4d148..85437b1980 100644\n--- a/http.c\n+++ b/http.c\n@@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n+\n+\tif (slot->finished == &finished)\n+\t\tslot->finished = NULL;\n }\n \n static void release_active_slot(struct active_request_slot *slot)\n-- \n2.36.1-200-gf89ea983ca\n\n"},{"id":"454966","messageId":"20220507054017.fnvb6xisr6s7m2l5@carlos-mbp.lan","threadId":"57858","inReplyTo":"xmqqczgqjr8y.fsf_-_@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-07T05:40:17Z","receivedAt":"2022-05-07T05:40:59Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, May 06, 2022 at 02:17:01PM -0700, Junio C Hamano wrote:\n> diff --git a/http.c b/http.c\n> index 229da4d148..85437b1980 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n> +\n> +\tif (slot->finished == &finished)\n> +\t\tslot->finished = NULL;\n\nI am not completely sure yet (since I looked at it long ago and got\nsidetracked) but I think this might be optimized out (at least by gcc12)\nsince it is technically UB, which is why it never \"fixed\" the warning.\n\nthe \"correct\" way to implement this would be to make \"finished\" a thread\nlocal static, which is finally one good reason to support C99, but the\nsyntax to do so with Windows broke my first attempt at doing so and now\nI can even find the code I used then which required a per platform macro\nand was better looking than the following\n\nCarlo\n--- >8 ----\nDate: Wed, 20 Apr 2022 23:25:55 -0700\nSubject: [PATCH] http: avoid using out of scope pointers\n\nbaa7b67d091 (HTTP slot reuse fixes, 2006-03-10) introduces a way\nto notify a curl thread that its slot is finished by using a pointer\nto a stack variable from run_active_slot(), but then gcc 12 was\nreleased and started rightfully to complain about it (-Wdangling-pointer).\n\nUse instead a thread storage static variable which is safe to use\nbetween threads since C99 and doesn't go out of scope, while being\nfunctionally equivalent to the original code, and also remove the\nworkaround from 9c539d1027d (config.mak.dev: alternative workaround\nto gcc 12 warning in http.c, 2022-04-15)\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n config.mak.dev | 1 -\n http.c | 2 +-\n 2 files changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex c3104f400b..335efd4620 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -68,7 +68,6 @@ endif\n # https://bugzilla.redhat.com/show_bug.cgi?id=2075786\n ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wno-error=stringop-overread\n-DEVELOPER_CFLAGS += -Wno-error=dangling-pointer\n endif\n \n GIT_TEST_PERL_FATAL_WARNINGS = YesPlease\ndiff --git a/http.c b/http.c\nindex 229da4d148..cb9acfca19 100644\n--- a/http.c\n+++ b/http.c\n@@ -1327,7 +1327,7 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-\tint finished = 0;\n+\tstatic __thread int finished;\n \n \tslot->finished = &finished;\n \twhile (!finished) {\n-- \n2.36.0.352.g0cd7feaf86f\n"},{"id":"454967","messageId":"20220507061401.onpbrd35w5xjrrh6@carlos-mbp.lan","threadId":"57858","inReplyTo":"cd50ec73ddafaaeba04298ae79cbf625cc0d7697.1651859773.git.git@grubix.eu","subject":"Re: [PATCH 1/2] dir.c: avoid gcc warning","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-07T06:14:47Z","receivedAt":"2022-05-07T06:31:57Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, May 06, 2022 at 08:04:05PM +0200, Michael J Gruber wrote:\n> Related to -Wstringop-overread.\n> \n> In fact, this may be a false positive\n\nIndeed it seems more like a bug[1] in gcc12, probably with their optimizer.\n\nGetting to the bottom of it with a minimized version of the code that would\ntrigger it would be a good way to help it move forward instead of \"fixing\"\ngit's codebase IMHO.\n\nIt would be also nice if someone from the gcc team would confirm or deny if\nthis is indeed something worth waiting for a fix on their side, or would\nneed some workaround in ours, or maybe even a real fix.\n\nFWIW there is already a workaround of sorts in our codebase since 846a29afb0\n(config.mak.dev: workaround gcc 12 bug affecting \"pedantic\" CI job, 2022-04-15)\nso that this warning should be expected when building with DEVELOPER=1 but\nit won't break the build as it would normally do.\n\nCarlo\n\n[1] https://bugzilla.redhat.com/show_bug.cgi?id=2075786\n"},{"id":"454981","messageId":"xmqq4k21gp6g.fsf@gitster.g","threadId":"57858","inReplyTo":"20220507054017.fnvb6xisr6s7m2l5@carlos-mbp.lan","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-07T18:42:15Z","receivedAt":"2022-05-07T18:42:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> On Fri, May 06, 2022 at 02:17:01PM -0700, Junio C Hamano wrote:\n>> diff --git a/http.c b/http.c\n>> index 229da4d148..85437b1980 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n>> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n>> \t\t}\n>> \t}\n>> +\n>> +\tif (slot->finished == &finished)\n>> +\t\tslot->finished = NULL;\n>\n> I am not completely sure yet (since I looked at it long ago and got\n> sidetracked) but I think this might be optimized out (at least by gcc12)\n> since it is technically UB, which is why it never \"fixed\" the warning.\n\nUB meaning \"undefined behaviour\"? Which part is? Taking the\naddress of an on-stack variable \"finished\"? Comparing it with a\npointer that may or may not have been assigned/overwritten elsewhere\nin a structure? Not clearing the member in the struct unconditionally?\n\nPuzzled.\n"},{"id":"454984","messageId":"CAPUEspjm2_6Omk1_VanXZJnRREnLS08H4t9tbxx=dnoqA+P43g@mail.gmail.com","threadId":"57858","inReplyTo":"xmqq4k21gp6g.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-05-07T19:11:26Z","receivedAt":"2022-05-07T19:11:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, May 7, 2022 at 11:42 AM Junio C Hamano wrote:\n>\n> Carlo Marcelo Arenas Belón writes:\n>\n> > On Fri, May 06, 2022 at 02:17:01PM -0700, Junio C Hamano wrote:\n> >> diff --git a/http.c b/http.c\n> >> index 229da4d148..85437b1980 100644\n> >> --- a/http.c\n> >> +++ b/http.c\n> >> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n> >> select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> >> }\n> >> }\n> >> +\n> >> + if (slot->finished == &finished)\n> >> + slot->finished = NULL;\n> >\n> > I am not completely sure yet (since I looked at it long ago and got\n> > sidetracked) but I think this might be optimized out (at least by gcc12)\n> > since it is technically UB, which is why it never \"fixed\" the warning.\n>\n> UB meaning \"undefined behaviour\"? Which part is? Taking the\n> address of an on-stack variable \"finished\"?\n> Comparing it with a\n> pointer that may or may not have been assigned/overwritten elsewhere\n> in a structure?\n\nit is not very intuitive, but using a pointer to a variable that is\nout of scope is UB, and in this case the value of slot->finished might\npoint to an address that is not in our own stack (because it came from\na different thread), hence undefined\n\nCarlo\n"},{"id":"454997","messageId":"f306f43f375bc9b9c98e85260587442e5d9ef0ba.1652094958.git.git@grubix.eu","threadId":"57858","inReplyTo":"cover.1651859773.git.git@grubix.eu","subject":"[PATCH] detect-compiler: make detection independent of locale","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-05-09T11:22:02Z","receivedAt":"2022-05-09T11:22:31Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"`detect-compiler` has accumulated a few compiler dependent workarounds\nlately for the more and more ubiquitious gcc12. This is intended to make\nCI set-ups work across tool-chain updates, but also help those\ndevelopers who build with `DEVELOPER=1`.\n\nAlas, `detect-compiler` uses the locale dependent output of `$(CC) -v`\nto parse for the version string, which fails unless it literally\ncontains ` version`.\n\nUse `LANG=C $(CC) -v` instead to grep for stable output.\n\nSigned-off-by: Michael J Gruber \n---\nSorry for not checking the ML before sending the previous patches. I\nknow now that the dir.c warning is a false psoitive and http.c's use of\nstack variables and globals is a mess ;)\n\nTo my excuse: Over here, the problem with the warnings was made worse\nbecause `DEVELOPER=1` turned them into errors for reasons fixed by this\npatch ...\n\n detect-compiler | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/detect-compiler b/detect-compiler\nindex 11d60da5b7..473f3bd4fe 100755\n--- a/detect-compiler\n+++ b/detect-compiler\n@@ -9,7 +9,7 @@ CC=\"$*\"\n #\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n-\t$CC -v 2>&1 | grep ' version '\n+\tLANG=C $CC -v 2>&1 | grep ' version '\n }\n \n get_family() {\n-- \n2.36.1.512.g0d1bd43151\n\n"},{"id":"455013","messageId":"xmqq7d6ug0un.fsf@gitster.g","threadId":"57858","inReplyTo":"f306f43f375bc9b9c98e85260587442e5d9ef0ba.1652094958.git.git@grubix.eu","subject":"Re: [PATCH] detect-compiler: make detection independent of locale","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-09T15:52:16Z","receivedAt":"2022-05-09T15:52:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> `detect-compiler` has accumulated a few compiler dependent workarounds\n> lately for the more and more ubiquitious gcc12. This is intended to make\n> CI set-ups work across tool-chain updates, but also help those\n> developers who build with `DEVELOPER=1`.\n>\n> Alas, `detect-compiler` uses the locale dependent output of `$(CC) -v`\n> to parse for the version string, which fails unless it literally\n> contains ` version`.\n>\n> Use `LANG=C $(CC) -v` instead to grep for stable output.\n\nI think this patch is a bit insufficient.\n\n $ LC_ALL=ja_JP.utf8 LANG=C gcc -v 2>&1 | head -n 1\n 組み込み spec を使用しています。\n $ LC_ALL=C LANG=ja_JP.utf8 gcc -v 2>&1 | head -n 1\n Using built-in specs.\n\nIn theory overriding LC_ALL alone may be sufficient these days where\neverybody seems to know about LC_*, but just out of habit, I would\nrecommend forcing both, i.e.\n\n> get_version_line() {\n> -\t$CC -v 2>&1 | grep ' version '\n> +\tLANG=C $CC -v 2>&1 | grep ' version '\n\nthis on top of the posted patch, which is what I'll squash in when\nqueuing this patch (no need to resend if you agree with the above\nand unless you have other changes and improvements).\n\nThanks.\n\ndiff --git i/detect-compiler w/detect-compiler\nindex 473f3bd4fe..50087f5670 100755\n--- i/detect-compiler\n+++ w/detect-compiler\n@@ -9,7 +9,7 @@ CC=\"$*\"\n #\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n-\tLANG=C $CC -v 2>&1 | grep ' version '\n+\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n }\n \n get_family() {\n"},{"id":"455014","messageId":"Ynk6HdsPqYH9Np92@nand.local","threadId":"57858","inReplyTo":"xmqqy1zejtte.fsf@gitster.g","subject":"Re: [PATCH 1/2] dir.c: avoid gcc warning","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T15:58:21Z","receivedAt":"2022-05-09T15:58:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 01:21:33PM -0700, Junio C Hamano wrote:\n> Michael J Gruber writes:\n>\n> > Related to -Wstringop-overread.\n> >\n> > In fact, this may be a false positive, but reading until the correct end\n> > is desirable here anyways.\n>\n> But the correct end is start + (end - start), not start + (end -\n> start + 1), isn't it? We've stripped trailing junk like /.git and\n> end is point at one byte beyond the end of URL to the repository.\n>\n> E.g. for \"https://auth@host/\", we have advanced start to point at\n> \"h\" at the beginning of \"host\", and we have moved end back from\n> pointing at the NUL at the end to point at \"/\" at the end of\n> \"host/\".\n>\n> We are trying to make sure that the resulting \"host\" string between\n> start and end do not have a slash to apply this special case.\n>\n> If the original URL were \"https://auth@host:4321/\", the end points\n> at \"/\" at the end of \"host:4321/\", making the string to be checked\n> to \"host:4321\" and we are trying to see it has no '/' in it (which\n> is the case). By extending the string by one, memchr() will see the\n> '/' at the end that is outside.\n>\n> This seems to be a behaviour breaking change and I am not sure what\n> we are trying to achieve with it. Is this a suggestion made by a\n> broken compiler you have, or something?\n\nI agree with this reasoning; the change here does not seem correct to\nme, and the original version looks to be doing what it advertises.\n\nThanks,\nTaylor\n"},{"id":"455015","messageId":"034701d863bd$d3688200$7a398600$@nexbridge.com","threadId":"57858","inReplyTo":"xmqq7d6ug0un.fsf@gitster.g","subject":"RE: [PATCH] detect-compiler: make detection independent of locale","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-05-09T15:59:49Z","receivedAt":"2022-05-09T16:00:04Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On May 9, 2022 11:52 AM, Junio C Hamano wrote:\n>Michael J Gruber writes:\n>\n>> `detect-compiler` has accumulated a few compiler dependent workarounds\n>> lately for the more and more ubiquitious gcc12. This is intended to\n>> make CI set-ups work across tool-chain updates, but also help those\n>> developers who build with `DEVELOPER=1`.\n>>\n>> Alas, `detect-compiler` uses the locale dependent output of `$(CC) -v`\n>> to parse for the version string, which fails unless it literally\n>> contains ` version`.\n>>\n>> Use `LANG=C $(CC) -v` instead to grep for stable output.\n>\n>I think this patch is a bit insufficient.\n>\n> $ LC_ALL=ja_JP.utf8 LANG=C gcc -v 2>&1 | head -n 1\n> 組み込み spec を使用しています。\n> $ LC_ALL=C LANG=ja_JP.utf8 gcc -v 2>&1 | head -n 1\n> Using built-in specs.\n>\n>In theory overriding LC_ALL alone may be sufficient these days where everybody\n>seems to know about LC_*, but just out of habit, I would recommend forcing\n>both, i.e.\n>\n>> get_version_line() {\n>> -\t$CC -v 2>&1 | grep ' version '\n>> +\tLANG=C $CC -v 2>&1 | grep ' version '\n>\n>this on top of the posted patch, which is what I'll squash in when queuing this\n>patch (no need to resend if you agree with the above and unless you have other\n>changes and improvements).\n>\n>Thanks.\n>\n>diff --git i/detect-compiler w/detect-compiler index 473f3bd4fe..50087f5670\n>100755\n>--- i/detect-compiler\n>+++ w/detect-compiler\n>@@ -9,7 +9,7 @@ CC=\"$*\"\n> #\n> # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n> get_version_line() {\n>-\tLANG=C $CC -v 2>&1 | grep ' version '\n>+\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n> }\n>\n> get_family() {\n\nJust a small transfer of experience from a different project - if we transition or expand LOCALE functions into C at some point. Be aware that the locale_t series in C is not supported universally, despite being in POSIX going back a few years. We found, at least on the OpenSSL project, that using locale_t caused compile breakages on a variety of platforms, including some older but active Linux variants. Just raising awareness as I'm working this issue there.\n\nSincerely,\nRandall\n\n"},{"id":"455855","messageId":"nycvar.QRO.7.76.6.2205232248360.352@tvgsbejvaqbjf.bet","threadId":"57858","inReplyTo":"xmqqczgqjr8y.fsf_-_@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-23T21:58:56Z","receivedAt":"2022-05-23T21:59:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 6 May 2022, Junio C Hamano wrote:\n\n> In http.c, the run_active_slot() function allows the given \"slot\" to\n> make progress by calling step_active_slots() in a loop repeatedly,\n> and the loop is not left until the request held in the slot\n> completes.\n>\n> Ages ago, we used to use the slot->in_use member to get out of the\n> loop, which misbehaved when the request in \"slot\" completes (at\n> which time, the result of the request is copied away from the slot,\n> and the in_use member is cleared, making the slot ready to be\n> reused), and the \"slot\" gets reused to service a different request\n> (at which time, the \"slot\" becomes in_use again, even though it is\n> for a different request). The loop terminating condition mistakenly\n> thought that the original request has yet to be completed.\n>\n> Today's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10)\n> fixed this issue, uses a separate \"slot->finished\" member that is\n> set in run_active_slot() to point to an on-stack variable, and the\n> code that completes the request in finish_active_slot() clears the\n> on-stack variable via the pointer to signal that the particular\n> request held by the slot has completed. It also clears the in_use\n> member (as before that fix), so that the slot itself can safely be\n> reused for an unrelated request.\n>\n> One thing that is not quite clean in this arrangement is that,\n> unless the slot gets reused, at which point the finished member is\n> reset to NULL, the member keeps the value of &finished, which\n> becomes a dangling pointer into the stack when run_active_slot()\n> returns. Clear the finished member before the control leaves the\n> function, but make sure to limit it to the case where the pointer\n> still points at the on-stack variable of ours (the pointer may be\n> set to point at the on-stack variable of somebody else after the\n> slot gets reused, in which case we do not want to touch it).\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * So, this has been sitting in my pile of random patches for a few\n> weeks.\n\nI stumbled over the need for this while investigating the build failures\ncaused by upgrading Git for Windows' SDK's GCC to v12.x.\n\n> diff --git a/http.c b/http.c\n> index 229da4d148..85437b1980 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n> +\n> +\tif (slot->finished == &finished)\n> +\t\tslot->finished = NULL;\n\nFirst of all, I suspect that\nhttps://github.com/git/git/blob/v2.36.1/http.c#L1207 makes sure that GCC's\ncomplaint is not actually accurate: we always re-set `finished` to `NULL`\nwhen getting an unused slot, so even if there is a left-over dangling\npointer, it is not actually used, ever.\n\nBut we need something to pacify GCC. Let's look at your patch.\n\nThe first thing to note is that this is not _quite_ thread-safe: between\nchecking the condition `slot->finished == &finished` and assigning\n`slot->finished`, another thread could potentially have noticed that the\nslot is not in use and overwritten the `finished` attribute, which would\nthen be set to `NULL` in this thread, in which case _that other_ thread's\n`while (!finished)` loop would become an infinite loop.\n\nHaving said that, the time window is really narrow.\n\nBesides, I suspect that we _already_ have an equivalent \"offender\" in\nhttps://github.com/git/git/blob/v2.36.1/http.c#L1336: we look at `in_use`\nthere, assuming that it is either `1` if \"our\" request is still active, and\notherwise it is `0`. However, it might have turned to `0` _and_ to `1`\nagain in the meantime (but the `in_use` would now refer to _another_\nrequest).\n\nI am not quite sure how correct my reading of the situation is, so please\ndouble-check my analysis.\n\nIf that analysis is correct, I would expect the correct solution to turn\n`finished` into an attribute of the slot, and change its role to be a flag\nthat this slot is spoken for and cannot be re-used quite yet even if it is\nnot currently in use.\n\nSomething like this:\n\n-- snip --\ndiff --git a/http-walker.c b/http-walker.c\nindex 910fae539b89..5cc369dea853 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -225,13 +225,9 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\t alt_req->url->buf);\n \t\t\tactive_requests++;\n \t\t\tslot->in_use = 1;\n-\t\t\tif (slot->finished != NULL)\n-\t\t\t\t(*slot->finished) = 0;\n \t\t\tif (!start_active_slot(slot)) {\n \t\t\t\tcdata->got_alternates = -1;\n \t\t\t\tslot->in_use = 0;\n-\t\t\t\tif (slot->finished != NULL)\n-\t\t\t\t\t(*slot->finished) = 1;\n \t\t\t}\n \t\t\treturn;\n \t\t}\ndiff --git a/http.c b/http.c\nindex b08795715f8a..2d125132fb90 100644\n--- a/http.c\n+++ b/http.c\n@@ -205,8 +205,7 @@ static void finish_active_slot(struct active_request_slot *slot)\n \tclosedown_active_slot(slot);\n \tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n\n-\tif (slot->finished != NULL)\n-\t\t(*slot->finished) = 1;\n+\tslot->in_use = 0;\n\n \t/* Store slot results so they can be read after the slot is reused */\n \tif (slot->results != NULL) {\n@@ -1212,13 +1211,14 @@ struct active_request_slot *get_active_slot(void)\n \t\t\tprocess_curl_messages();\n \t}\n\n-\twhile (slot != NULL && slot->in_use)\n+\twhile (slot != NULL && (slot->in_use || slot->reserved_for_use))\n \t\tslot = slot->next;\n\n \tif (slot == NULL) {\n \t\tnewslot = xmalloc(sizeof(*newslot));\n \t\tnewslot->curl = NULL;\n \t\tnewslot->in_use = 0;\n+\t\tnewslot->reserved_for_use = 0;\n \t\tnewslot->next = NULL;\n\n \t\tslot = active_queue_head;\n@@ -1240,7 +1240,6 @@ struct active_request_slot *get_active_slot(void)\n \tactive_requests++;\n \tslot->in_use = 1;\n \tslot->results = NULL;\n-\tslot->finished = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n@@ -1332,7 +1331,7 @@ void fill_active_slots(void)\n \t}\n\n \twhile (slot != NULL) {\n-\t\tif (!slot->in_use && slot->curl != NULL\n+\t\tif (!slot->in_use && !slot->reserved_for_use && slot->curl\n \t\t\t&& curl_session_count > min_curl_sessions) {\n \t\t\tcurl_easy_cleanup(slot->curl);\n \t\t\tslot->curl = NULL;\n@@ -1363,10 +1362,9 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-\tint finished = 0;\n\n-\tslot->finished = &finished;\n-\twhile (!finished) {\n+\tslot->reserved_for_use = 1;\n+\twhile (slot->in_use) {\n \t\tstep_active_slots();\n\n \t\tif (slot->in_use) {\n@@ -1403,6 +1401,7 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n+\tslot->reserved_for_use = 0;\n }\n\n static void release_active_slot(struct active_request_slot *slot)\ndiff --git a/http.h b/http.h\nindex df1590e53a45..3b2f6da570cd 100644\n--- a/http.h\n+++ b/http.h\n@@ -22,9 +22,9 @@ struct slot_results {\n struct active_request_slot {\n \tCURL *curl;\n \tint in_use;\n+\tint reserved_for_use;\n \tCURLcode curl_result;\n \tlong http_code;\n-\tint *finished;\n \tstruct slot_results *results;\n \tvoid *callback_data;\n \tvoid (*callback_func)(void *data);\n-- snap --\n\nI integrated this into a local branch that fixes the build with GCC v12.x\n(required so that our CI/PR builds work again after Git for Windows' SDK\nupgraded its GCC) and plan on contributing these patches in a bit.\n\nCiao,\nDscho\n\n> }\n>\n> static void release_active_slot(struct active_request_slot *slot)\n> --\n> 2.36.1-200-gf89ea983ca\n>\n>\n>\n"},{"id":"455859","messageId":"xmqqr14jluu4.fsf@gitster.g","threadId":"57858","inReplyTo":"nycvar.QRO.7.76.6.2205232248360.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T22:58:43Z","receivedAt":"2022-05-23T22:58:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I stumbled over the need for this while investigating the build failures\n> caused by upgrading Git for Windows' SDK's GCC to v12.x.\n>\n>> diff --git a/http.c b/http.c\n>> index 229da4d148..85437b1980 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n>> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n>> \t\t}\n>> \t}\n>> +\n>> +\tif (slot->finished == &finished)\n>> +\t\tslot->finished = NULL;\n>\n> First of all, I suspect that\n> https://github.com/git/git/blob/v2.36.1/http.c#L1207 makes sure that GCC's\n> complaint is not actually accurate: we always re-set `finished` to `NULL`\n> when getting an unused slot, so even if there is a left-over dangling\n> pointer, it is not actually used, ever.\n>\n> But we need something to pacify GCC. Let's look at your patch.\n>\n> The first thing to note is that this is not _quite_ thread-safe: between\n\nDoes this part of the code ever run multi-threaded?\n\n> If that analysis is correct, I would expect the correct solution to turn\n> `finished` into an attribute of the slot, and change its role to be a flag\n> that this slot is spoken for and cannot be re-used quite yet even if it is\n> not currently in use.\n\nI have a feeling that we've mentioned that at least twice (perhaps\nthree times) in the recent past that it is in essense reverting what\nthe \"finished\" change baa7b67d (HTTP slot reuse fixes, 2006-03-10)\ndid. We used to use the in-use bit of the slot as an indicator that\nthe slot dispatched by run_active_slot() has finished (i.e. the\nin-use bit must be cleared when the request held in the struct is\nfully done), but that broke when a slot we are looking at in\nrun_active_slot() is serviced (which makes in_use false), and then\nanother request reuses the slot (now no longer in_use), before the\ncontrol comes back to the loop. \"while (slot->in_use)\" at the\nbeginning of the loop was still true, but the original request the\nslot was being used for, the one that the run_active_slot() function\ncares about, has completed.\n\nSo...\n"},{"id":"455860","messageId":"xmqqleurlt31.fsf@gitster.g","threadId":"57858","inReplyTo":"xmqqr14jluu4.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-23T23:36:34Z","receivedAt":"2022-05-23T23:36:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> I stumbled over the need for this while investigating the build failures\n>> caused by upgrading Git for Windows' SDK's GCC to v12.x.\n>>\n>>> diff --git a/http.c b/http.c\n>>> index 229da4d148..85437b1980 100644\n>>> --- a/http.c\n>>> +++ b/http.c\n>>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n>>> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n>>> \t\t}\n>>> \t}\n>>> +\n>>> +\tif (slot->finished == &finished)\n>>> +\t\tslot->finished = NULL;\n> ...\n> I have a feeling that we've mentioned that at least twice (perhaps\n> three times) in the recent past that it is in essense reverting what\n> the \"finished\" change baa7b67d (HTTP slot reuse fixes, 2006-03-10)\n> did. We used to use the in-use bit of the slot as an indicator that\n> the slot dispatched by run_active_slot() has finished (i.e. the\n> in-use bit must be cleared when the request held in the struct is\n> fully done), but that broke when a slot we are looking at in\n> run_active_slot() is serviced (which makes in_use false), and then\n> another request reuses the slot (now no longer in_use), before the\n> control comes back to the loop. \"while (slot->in_use)\" at the\n> beginning of the loop was still true, but the original request the\n> slot was being used for, the one that the run_active_slot() function\n> cares about, has completed.\n\nGiven that the breakage we fixed in 2006 is about run_active_slot()\ncalling step_active_slots() repeatedly, during which this and other\nrequests in flight completes when curl_multi_perform() receives and\nhandles responses, and recursively ends up calling run_active_slot()\nfor _another_ request reusing the slot we are interested in in the\ncodepath in the above disccussion, I _think_ we do not have to\nconsider the case where slot->finished is pointing at somebody\nelse's finished variable on stack here. Yes, while we repeatedly\ncall step_active_slots(), our request in the slot may complete, the\nslot may be marked as unused, somebody else may reuse the slot,\nmarking it as in_use again and using slot->finished pointer to their\non-stack finsihed. But that somebody else's invocation of\nrun_active_slot() will not give control back before their on-stack\nfinished indicates that their recursive call to step_active_slots()\ncompletes their request. So after they come back and we exit our\nwhile() loop, either slot->finished points at our finished if slot\ndid not get reused, or it points at an unused part of the stack that\nhas long been rewound when we returned from the recursive call. In\neither case, slot->finished never points at an on-stack address of\nan ongoing run_active_slot() call made by somebody else that the\nguard I added (i.e. we must only clear it if it points our on-stack\n\"finished\") was trying to protect against clobbering.\n\nSo, I guess an unconditional assignment of\n\n\tslot->finished = NULL;\n\nthere would be sufficient.\n"},{"id":"455861","messageId":"nycvar.QRO.7.76.6.2205240124280.352@tvgsbejvaqbjf.bet","threadId":"57858","inReplyTo":"xmqqr14jluu4.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-23T23:41:02Z","receivedAt":"2022-05-23T23:41:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 23 May 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I stumbled over the need for this while investigating the build failures\n> > caused by upgrading Git for Windows' SDK's GCC to v12.x.\n> >\n> >> diff --git a/http.c b/http.c\n> >> index 229da4d148..85437b1980 100644\n> >> --- a/http.c\n> >> +++ b/http.c\n> >> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)\n> >> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> >> \t\t}\n> >> \t}\n> >> +\n> >> +\tif (slot->finished == &finished)\n> >> +\t\tslot->finished = NULL;\n> >\n> > First of all, I suspect that\n> > https://github.com/git/git/blob/v2.36.1/http.c#L1207 makes sure that GCC's\n> > complaint is not actually accurate: we always re-set `finished` to `NULL`\n> > when getting an unused slot, so even if there is a left-over dangling\n> > pointer, it is not actually used, ever.\n> >\n> > But we need something to pacify GCC. Let's look at your patch.\n> >\n> > The first thing to note is that this is not _quite_ thread-safe: between\n>\n> Does this part of the code ever run multi-threaded?\n\nIt calls into cURL, which I suspect has a multi-threaded mode of\noperation, and we do use some callbacks in `http-walker.c` and I was\nworried that these callbacks could be called via cURL calls. That's why I\nam concerned about thread-safety. I have looked for a while and not found\nany way that code in `http.c` or `http-walker.c` could set `finished` by\nway of a cURL callback, but there is a lot of code there and I could have\nmissed something crucial.\n\n> > If that analysis is correct, I would expect the correct solution to turn\n> > `finished` into an attribute of the slot, and change its role to be a flag\n> > that this slot is spoken for and cannot be re-used quite yet even if it is\n> > not currently in use.\n>\n> I have a feeling that we've mentioned that at least twice (perhaps\n> three times) in the recent past that it is in essense reverting what\n> the \"finished\" change baa7b67d (HTTP slot reuse fixes, 2006-03-10)\n> did. We used to use the in-use bit of the slot as an indicator that\n> the slot dispatched by run_active_slot() has finished (i.e. the\n> in-use bit must be cleared when the request held in the struct is\n> fully done), but that broke when a slot we are looking at in\n> run_active_slot() is serviced (which makes in_use false), and then\n> another request reuses the slot (now no longer in_use), before the\n> control comes back to the loop. \"while (slot->in_use)\" at the\n> beginning of the loop was still true, but the original request the\n> slot was being used for, the one that the run_active_slot() function\n> cares about, has completed.\n>\n> So...\n\nNo, I suggested to replace the `finished` variable with an attribute (or\n\"field\" or \"member variable\") of the slot, and to respect it when looking\nfor an unused slot, i.e. not only look for a slot whose `in_use` is 0 but\nalso require `reserved_for_use` to be 0. In essence, the\n`run_active_slot()` function owns the slot, even if it is not marked as\n`in_use`. That should address the same concern as baa7b67d but without\nusing a pointer to a local variable.\n\nCiao,\nDscho\n"},{"id":"455862","messageId":"xmqqa6b7lrw6.fsf@gitster.g","threadId":"57858","inReplyTo":"nycvar.QRO.7.76.6.2205240124280.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T00:02:17Z","receivedAt":"2022-05-24T00:02:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> It calls into cURL, which I suspect has a multi-threaded mode of\n> operation,\n\nhttps://curl.se/libcurl/c/threadsafe.html ;-)\n\nMy understanding is that what we have is pretty much select() driven\nsingle-threaded multi-fd transfer.\n\n> No, I suggested to replace the `finished` variable with an attribute (or\n> \"field\" or \"member variable\") of the slot, and to respect it when looking\n> for an unused slot, i.e. not only look for a slot whose `in_use` is 0 but\n> also require `reserved_for_use` to be 0. In essence, the\n> `run_active_slot()` function owns the slot, even if it is not marked as\n> `in_use`. That should address the same concern as baa7b67d but without\n> using a pointer to a local variable.\n\nNot really. An outer run_active_slot() and an inner\nrun_active_slot() have a pointer to the same slot object.\n\nThe inner one got hold of that object because the request the slot\nused to represent for the outer run_active_slot() has finished, so\nwe would toggle either *(slot->finished) or the new slot->done in an\nattempt to signal the completion to the outer run_active_slot() and\nthen make the slot not-in-use. The slot becomes in-use again with a\ndifferent request and the inner run_active_slot() is run. It first\nsays \"this slot is not done yet---we are making a request using\nit\". How would the inner one say that, exactly?\n\nIn baa7b67d's fix, it is done by setting slot->finished = &finished\nto its own stackframe. Because the outer run_active_slot() does not\nlook at slot->finished, but it looks at the finished on its\nstackframe, what the inner run_active_slot() does here would not\nbreak the outer one.\n\nIf we replace the mechanism with a separate member in the slot\nstructure, so that the outer run_active_slot() looks at slot->done\nand the inner run_active_slot() also clears slot->done before\nproceeding, then the inner one clobbers what the outer one will look\nat when the recursive call that led to the inner one returns.\n"},{"id":"455874","messageId":"q274s3nn-pp38-4sn-53ro-o2q63447r341@unkk.fr","threadId":"57858","inReplyTo":"xmqqa6b7lrw6.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2022-05-24T06:31:55Z","receivedAt":"2022-05-24T06:40:12Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Mon, 23 May 2022, Junio C Hamano wrote:\n\n>> It calls into cURL, which I suspect has a multi-threaded mode of\n>> operation,\n>\n> https://curl.se/libcurl/c/threadsafe.html ;-)\n>\n> My understanding is that what we have is pretty much select() driven \n> single-threaded multi-fd transfer.\n\nConfirmed. libcurl *can* use threads (if built that way), but the only use it \nhas for such subthreads is for resolving host names. libcurl, its API and its \ncallbacks etc always operate in the same single thread.\n\n-- \n\n / daniel.haxx.se\n"},{"id":"455882","messageId":"nycvar.QRO.7.76.6.2205241253270.352@tvgsbejvaqbjf.bet","threadId":"57858","inReplyTo":"q274s3nn-pp38-4sn-53ro-o2q63447r341@unkk.fr","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T10:57:18Z","receivedAt":"2022-05-24T10:57:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Tue, 24 May 2022, Daniel Stenberg wrote:\n\n> On Mon, 23 May 2022, Junio C Hamano wrote:\n>\n> > > It calls into cURL, which I suspect has a multi-threaded mode of\n> > > operation,\n> >\n> > https://curl.se/libcurl/c/threadsafe.html ;-)\n> >\n> > My understanding is that what we have is pretty much select() driven\n> > single-threaded multi-fd transfer.\n>\n> Confirmed. libcurl *can* use threads (if built that way), but the only use it\n> has for such subthreads is for resolving host names. libcurl, its API and its\n> callbacks etc always operate in the same single thread.\n\nGreat, thanks for clarifying!\n\nCiao,\nDscho\n\nP.S.: I'm enjoying your book, Uncurled. It feels great to see validation\nin some experiences (I'm a youngster compared to you, maintaining Open\nSource software only since 2001 or so, but still), and to get a fresh and\ninspiring perspective on others.\n"},{"id":"455883","messageId":"nycvar.QRO.7.76.6.2205241258510.352@tvgsbejvaqbjf.bet","threadId":"57858","inReplyTo":"xmqqa6b7lrw6.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-24T11:03:41Z","receivedAt":"2022-05-24T11:04:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 23 May 2022, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I suggested to replace the `finished` variable with an attribute (or\n> > \"field\" or \"member variable\") of the slot, and to respect it when\n> > looking for an unused slot, i.e. not only look for a slot whose\n> > `in_use` is 0 but also require `reserved_for_use` to be 0. In essence,\n> > the `run_active_slot()` function owns the slot, even if it is not\n> > marked as `in_use`. That should address the same concern as baa7b67d\n> > but without using a pointer to a local variable.\n>\n> Not really. An outer run_active_slot() and an inner\n> run_active_slot() have a pointer to the same slot object.\n\nHow is that possible? One of the first things that function does is to\nassign `slot->finished = &finished`, and then run that `while (!finished)`\nloop.\n\nHow would the outer `run_active_slot()` ever get signaled via `finished`\nwhen the inner `run_active_slot()` would overwrite `slot->finished`? I am\npuzzled why we do not see infinite loops in such outer calls all the time,\nthen.\n\nCiao,\nDscho\n"},{"id":"455915","messageId":"xmqqleuqj1gy.fsf@gitster.g","threadId":"57858","inReplyTo":"nycvar.QRO.7.76.6.2205241258510.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T17:15:57Z","receivedAt":"2022-05-24T17:16:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Not really. An outer run_active_slot() and an inner\n>> run_active_slot() have a pointer to the same slot object.\n>\n> How is that possible? One of the first things that function does is to\n> assign `slot->finished = &finished`, and then run that `while (!finished)`\n> loop.\n>\n> How would the outer `run_active_slot()` ever get signaled via `finished`\n> when the inner `run_active_slot()` would overwrite `slot->finished`? I am\n> puzzled why we do not see infinite loops in such outer calls all the time,\n> then.\n\nThe idea in http subsystem goes like this.\n\n * Generally, we have multiple curl requests in flight. A curlm\n passed to curl_multi_perform() call knows about them and attempts\n to make as much progress without blocking.\n\n * After calling curl_multi_perform(), we call process_curl_messages()\n to collect the response that corresponds to the request. This is\n done using the slot data structure. Once we read the response,\n we may process it further by making a callback.\n\n * A slot, when finished, can be reused. THe reuse is controlled by\n its in_use member.\n\nSo, let's trace a code flow, http-walker.c::fetch_object() is used\nas a sample starting point.\n\n * http-walker.c::fetch_object()\n - pushes the object name to object_request queue.\n - calls step_active_slots() to make progress. This function in turn\n - calls curl_multi_perform() repeatedly to make progress\n - calls process_curl_messages() to possibly complete some active slots\n - calls fill_active_slots() to fill more requests. This function\n calls the \"fill\" function repeatedly to make more requests,\n which is http-walker.c::fill_active_slot() in this code path. It\n - repeatedly calls start_object_request()\n * start_object_request() does these:\n - calls new_http_object_request(), which prepares object-request\n structure, in which there is a slot member that was\n\t obtained by calling get_active_slot().\n\t * get_active_slot() does many things, but all we need to know\t\n\t here is that it does \"in_use = 1\".\n - sets callback for the slot to process_object_response()\n - calls start_active_slot(),\n which adds the slot to curlm and calls curl_multi_perform()\n to make progress on the active slots.\n - calls run_active_slots() repeatedly.\n\nNow run_active_slots() we know about. Before baa7b67d (HTTP slot\nreuse fixes, 2006-03-10), we used to loop on slot->in_use but to fix\na bug we updated it to use slot->finished.\n\n * run_active_slot()\n - takes a slot\n - clears finished on its stack\n - makes slot->finished point at &finished on its stack\n - loops until \"finished\" is set\n - calls step_active_slots(); what it does can be seen above,\n but here, we need to know what process_curl_messages() it\n calls does, in order to complete some requests.\n * process_curl_messages() \n - reads the response from curl\n - finds the slot with request that resulted in the response\n - sets its result member\n - calls finish_active_slot() on it, which in turn does these:\n\t - calls closedown_active_slot(), slot->in_use becomes 0\n - sets (*slot->finished) = 1\n\t - calls slot->callback_func\n\nThe callback_func was set to process_object_response() earlier in\nthis code flow.\n\n * http-walker.c::process_object_response()\n - calls process_http_object_request(), which dissociates the slot\n from the http_object_request object.\n - may call fetch_alternates() when the object is not found,\n otherwise calls finish_object_request().\n\nLet's see what happens when fetch_alternates() gets called here.\n\n * http-walker.c::fetch_alternates()\n - calls step_active_slots() to make progress\n - calls get_active_slot() \n - calls start_active_slot()\n - calls run_active_slot()\n\nNow we can see how the \"slot\" we used in the \"outer\" run_active_slot()\ncan be reused for a different request. We received response to the\nrequest, and in process_curl_messages(), we called finish_active_slot()\non the slot, which did three things: (1) slot is now not-in-use, (2) the\n\"finished\" on the stack of the outer run_active_slot() is set to 1, and\n(3) called the process_object_response() callback.\n\nThe callback then asked for an unused slot, and got the slot we just\nused, because we no longer need it (the necessary information in the\nresponse have been copied away to http_object_request object before\nthe slot was dissociated from it, and the only one bit of\ninformation the outer run_active_slot() needs has already been sent\nthere on its on-stack \"finished\" variable). The reused slot goes\nthrough the usual start_active_slot() call to add it to curlm, and\nthen the \"inner\" run_active_slot() is started on it. Until the\ninner run_active_slot() returns, fetch_alternates() would not\nreturn, but once it does, the control goes back to the outer\nrun_active_slot(), where it finds that its \"finished\" is now set to\n1.\n\nThis incidentally is a good illustration why the thread-starter\npatch that did\n\n\tif (&finished == slot->finished)\n\t\tslot->finished = NULL;\n\nwould be sufficient, and the \"clear only ours\" guard is not\nnecessary, I think. If the inner run_active_slot() did not trigger\na callback that adds more reuse of the slot, it will clear\nslot->finished to NULL itself, with or without the guard. And the\nouter run_active_slot() may fail to clear if the guard is there, but\nslot->finished is NULL in that case, so there is no point in clearing\nit again.\n\nAnd if the inner run_active_slot() did trigger a callback that ended\nup reusing the slot, then eventually the innermost one would have\ncleared slot->finished to NULL, with or without the guard, before it\nreturned the control to inner run_active_slot(). The inference goes\nthe same way to show that the guard is not necessary but is not\nhurting.\n\nI _think_ we can even get away by not doing anything to\nslot->finished at the end of run_active_slot(), as we are not\nmulti-threaded and the callee only returns to the caller, but if it\nhelps pleasing the warning compiler, I'd prefer the simplest\nworkaround, perhaps with an unconditional clearing there?\n\nWhat did I miss? I must be missing something, as I can explain how\nthe current \"(*slot->finished) = 1\" with \"while (finished)\"\ncorrectly works, but I cannot quite explain why the original \"while\n(slot->in_use)\" would not, which is annoying.\n\nIn other words, why we needed baa7b67d (HTTP slot reuse fixes,\n2006-03-10) in the first place? It is possible that we had some\ncode paths that forgot to drop in_use before the inner run_active\nreturned that have been fixed in the 16 years and this fix was\nhiding that bug, but I dunno.\n"},{"id":"455919","messageId":"xmqqa6b6j04b.fsf@gitster.g","threadId":"57858","inReplyTo":"q274s3nn-pp38-4sn-53ro-o2q63447r341@unkk.fr","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T17:45:08Z","receivedAt":"2022-05-24T17:45:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Stenberg writes:\n\n> On Mon, 23 May 2022, Junio C Hamano wrote:\n>\n>>> It calls into cURL, which I suspect has a multi-threaded mode of\n>>> operation,\n>>\n>> https://curl.se/libcurl/c/threadsafe.html ;-)\n>>\n>> My understanding is that what we have is pretty much select() driven\n>> single-threaded multi-fd transfer.\n>\n> Confirmed. libcurl *can* use threads (if built that way), but the only\n> use it has for such subthreads is for resolving host names. libcurl,\n> its API and its callbacks etc always operate in the same single\n> thread.\n\nThanks. \n\nIt always is nice to have the authority/expert readily answering our\nstupid questions ;-)\n\n"},{"id":"455949","messageId":"20220524201639.2gucdkzponddk5qt@carlos-mbp.lan","threadId":"57858","inReplyTo":"xmqqleuqj1gy.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-05-24T20:16:39Z","receivedAt":"2022-05-24T20:16:44Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, May 24, 2022 at 10:15:57AM -0700, Junio C Hamano wrote:\n> \n> I _think_ we can even get away by not doing anything to\n> slot->finished at the end of run_active_slot(), as we are not\n> multi-threaded and the callee only returns to the caller, but if it\n> helps pleasing the warning compiler, I'd prefer the simplest\n> workaround, perhaps with an unconditional clearing there?\n\nAssuming that some overly clever compiler might optimize that out (either\nbecause it might think it is Undefined Behaviour or for other unknown\nreasons) then Ævar's version would be better for clearing the \"warning\".\n\nBut your patch fixed the \"bug\" that a probably overeager compiler was\n\"detecting\".\n\n> What did I miss? I must be missing something, as I can explain how\n> the current \"(*slot->finished) = 1\" with \"while (finished)\"\n> correctly works, but I cannot quite explain why the original \"while\n> (slot->in_use)\" would not, which is annoying.\n\nMy guess is that there is a curl version somewhere that is patched to use\nthreads more extensible than upstream and where this code is stil needed.\nI think it is also safe to assume (like you did) that this is a 16 year bug\nthat was already fixed and reverting that code would be an alternative too.\n\nCarlo\n"},{"id":"455953","messageId":"220524.86r14ivewt.gmgdl@evledraar.gmail.com","threadId":"57858","inReplyTo":"xmqqleuqj1gy.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T20:38:30Z","receivedAt":"2022-05-24T20:44:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Junio C Hamano wrote:\n\n> [...]\n> This incidentally is a good illustration why the thread-starter\n> patch that did\n>\n> \tif (&finished == slot->finished)\n> \t\tslot->finished = NULL;\n>\n> would be sufficient, and the \"clear only ours\" guard is not\n> necessary, I think. If the inner run_active_slot() did not trigger\n> a callback that adds more reuse of the slot, it will clear\n> slot->finished to NULL itself, with or without the guard. And the\n> outer run_active_slot() may fail to clear if the guard is there, but\n> slot->finished is NULL in that case, so there is no point in clearing\n> it again.\n>\n> And if the inner run_active_slot() did trigger a callback that ended\n> up reusing the slot, then eventually the innermost one would have\n> cleared slot->finished to NULL, with or without the guard, before it\n> returned the control to inner run_active_slot(). The inference goes\n> the same way to show that the guard is not necessary but is not\n> hurting.\n>\n> I _think_ we can even get away by not doing anything to\n> slot->finished at the end of run_active_slot(), as we are not\n> multi-threaded and the callee only returns to the caller, but if it\n> helps pleasing the warning compiler, I'd prefer the simplest\n> workaround, perhaps with an unconditional clearing there?\n\nI'll admit I haven't fully looked into this again, but does anything in\nthe subsequent analysis suggest that my original patch wouldn't be a\nworking solution to this, still:\nhttps://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ ?\n\nThe advantage of it over any small and narrow fix like your hunk quoted\nabove is that it doesn't make the end result look as though we care\nabout e.g. thread races, which evidently is something more than one\nperson looking at this has (needlessly) ended up worrying about.\n\n> What did I miss? I must be missing something, as I can explain how\n> the current \"(*slot->finished) = 1\" with \"while (finished)\"\n> correctly works, but I cannot quite explain why the original \"while\n> (slot->in_use)\" would not, which is annoying.\n\nPerhaps that change was also worried about thread safety? I only briefly\nre-looked at this again, but I don't think I ever found exactly what\nthat 2006-era fix was meant to fix, specifically.\n\n> In other words, why we needed baa7b67d (HTTP slot reuse fixes,\n> 2006-03-10) in the first place? It is possible that we had some\n> code paths that forgot to drop in_use before the inner run_active\n> returned that have been fixed in the 16 years and this fix was\n> hiding that bug, but I dunno.\n\nI haven't found that out either, either back in January or just now.\n"},{"id":"455958","messageId":"220524.86mtf6ve89.gmgdl@evledraar.gmail.com","threadId":"57858","inReplyTo":"20220524201639.2gucdkzponddk5qt@carlos-mbp.lan","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-24T20:45:03Z","receivedAt":"2022-05-24T20:59:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Carlo Marcelo Arenas Belón wrote:\n\n> On Tue, May 24, 2022 at 10:15:57AM -0700, Junio C Hamano wrote:\n>> \n>> I _think_ we can even get away by not doing anything to\n>> slot->finished at the end of run_active_slot(), as we are not\n>> multi-threaded and the callee only returns to the caller, but if it\n>> helps pleasing the warning compiler, I'd prefer the simplest\n>> workaround, perhaps with an unconditional clearing there?\n>\n> Assuming that some overly clever compiler might optimize that out (either\n> because it might think it is Undefined Behaviour or for other unknown\n> reasons) then Ævar's version would be better for clearing the \"warning\".\n>\n> But your patch fixed the \"bug\" that a probably overeager compiler was\n> \"detecting\".\n\nJust briefly for those who perhaps didn't fully read the initial\nthread. Per [1] and [2] (search for -fanalyzer in that [2]) it's not a\nbug, undesired behavior etc. from GCC that it's \"overeager\" to warn in\nthis case.\n\nMost warnings from compilers are in the category of not being triggered\non the basis of exhaustive code analysis which tries to prove that it's\na practical issue for *your codebase*. It's equally about warning you\nabout patterns that might be a problem in the future.\n\nIn this case I don't know how this line of reasoning started, or how the\noutput is confusing.\n\nE.g. Johannes notes upthread that the \"complaint is not actually\naccurate\". Well, yes it is, because the warning says:\n\t\n\thttp.c: In function ‘run_active_slot’:\n\thttp.c:1332:24: warning: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Wdangling-pointer=]\n\t 1332 | slot->finished = &finished;\n\t | ~~~~~~~~~~~~~~~^~~~~~~~~~~\n\nI.e. it's telling us that we're *storing* the address, which we're\ndoing. \"Storing\" meaning \"past the lifetime of the function\".\n\nIt doesn't mean that GCC has additionally proved that we'll later used\nit in a way that will have a meaningful impact on the behavior of our\nprogram, or even that it's tried to do that. See an excerpt from the GCC\ncode (a comment) in [1].\n\n1. https://lore.kernel.org/git/220127.86mtjhdeme.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/220414.86h76vd69t.gmgdl@evledraar.gmail.com/\n"},{"id":"455977","messageId":"xmqqwneaeftz.fsf@gitster.g","threadId":"57858","inReplyTo":"20220524201639.2gucdkzponddk5qt@carlos-mbp.lan","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T22:16:56Z","receivedAt":"2022-05-24T22:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> My guess is that there is a curl version somewhere that is patched to use\n> threads more extensible than upstream and where this code is stil needed.\n> I think it is also safe to assume (like you did) that this is a 16 year bug\n> that was already fixed and reverting that code would be an alternative too.\n\nSorry, but this does not make any sense to me.\n\nIt wasn't like we were working around somebody else's bug 16 years\nago. Reverting the bugfix we made 16 years ago will make the issue\nfixed 16 years ago to reappear.\n\nAlso, even if your version of curl library uses multi-threading\ninternally, it cannot magically make _our_ calls into curl library\nto be multi-threaded, letting a control flow that came from\nrun_active_slot() down to another recursive invocation of\nrun_active_slot() to spin there calling curl_multi_perform()\nrepeatedly, while returning the control to the outer\nrun_active_slot() at the same time. So \"a curl version somewhere\nthat is patched\" does not sound like a plausible explanation,\neither.\n"},{"id":"455981","messageId":"xmqqleuqefa4.fsf@gitster.g","threadId":"57858","inReplyTo":"220524.86r14ivewt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T22:28:51Z","receivedAt":"2022-05-24T22:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> I _think_ we can even get away by not doing anything to\n>> slot->finished at the end of run_active_slot(), as we are not\n>> multi-threaded and the callee only returns to the caller, but if it\n>> helps pleasing the warning compiler, I'd prefer the simplest\n>> workaround, perhaps with an unconditional clearing there?\n>\n> I'll admit I haven't fully looked into this again, but does anything in\n> the subsequent analysis suggest that my original patch wouldn't be a\n> working solution to this, still:\n> https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ ?\n\nI traced _one_ code path as a demonstration to show why the current\n\"slot->finished = &finished\" based solution works. \n\nBut I think what we need is to demonstrate a code path in the old\nversion that shows why the old slot->in_use would not have worked\nand the slot->finished was needed, and demonstrate why it NO LONGER\nis the case in today's code. Without that, especially with the\nlatter, I cannot take the \"just revert 16-year old bugfix because a\nnew compiler throws a warning related to multi-threaded code to it,\neven though we are strictly single-threaded\" as a serious solution.\n\nAnd because I do not think I've seen anybody has done that necessary\ndigging, I would still prefer the \"if the compiler somehow cares,\nthen let's clear the finished member once we are done with it\" much\nbetter than \"we do not know why but we somehow think we can do\nwithout this bugfix, even though we wouldn't be making noises about\nthis piece of code if a new compiler did not start emitting a\nwarning\".\n\nThanks.\n\n"},{"id":"455982","messageId":"xmqqh75eef0f.fsf@gitster.g","threadId":"57858","inReplyTo":"220524.86mtf6ve89.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T22:34:40Z","receivedAt":"2022-05-24T22:34:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> It doesn't mean that GCC has additionally proved that we'll later used\n> it in a way that will have a meaningful impact on the behavior of our\n> program, or even that it's tried to do that. See an excerpt from the GCC\n> code (a comment) in [1].\n\nBut that means the warning just as irrelevant as \"you stored 438 to\nthis integer variable\". Sure, there may be cases where that integer\nvariable should not exceed 400 and if the compiler can tell us that,\nthat would be a valuable help to developers. But \"you stored an\naddress of an object that can go out of scope in another object\nwhose lifetime lasts beyond the scope\" alone is not, without \"and\nthe caller that passed the latter object later dereferences that\naddress here\". We certainly shouldn't -Werror on such a warning\nand bend our code because of it.\n"},{"id":"455986","messageId":"xmqqo7zmcydv.fsf@gitster.g","threadId":"57858","inReplyTo":"20220524201639.2gucdkzponddk5qt@carlos-mbp.lan","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T23:19:08Z","receivedAt":"2022-05-24T23:19:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> On Tue, May 24, 2022 at 10:15:57AM -0700, Junio C Hamano wrote:\n>> \n>> I _think_ we can even get away by not doing anything to\n>> slot->finished at the end of run_active_slot(), as we are not\n>> multi-threaded and the callee only returns to the caller, but if it\n>> helps pleasing the warning compiler, I'd prefer the simplest\n>> workaround, perhaps with an unconditional clearing there?\n>\n> Assuming that some overly clever compiler might optimize that out (either\n> because it might think it is Undefined Behaviour or for other unknown\n> reasons) then Ævar's version would be better for clearing the \"warning\".\n\nYou keep saying undefined behaviour but in this case I do not quite\nsee there is anything undefined.\n\nThe warning, as Ævar said in a message, is about storing an address\nof an object on the stack in another object whose lifetime lasts\nbeyond the life of the stackframe. If you dereference such a\npointer _after_ we return from run_active_slot() function, the\nbehaviour may indeed be undefined.\n\nBut if you recall one such call trace I walked through for Dscho in\nanother message this morning, we do not make such a dereferencing.\nThe run_active_slot() function sets up the slot with the pointer to\nits on-stack variable in it, we make a call chain that is several\nlevels deep, and at some point in the call chain, the request that\nis represented by the slot may complete and slot may be passed to\nfinish_active_slot(), which would update (*slot->finished) thus\nmodifying the on-stack variable of the run_active_slot() that we\nwill eventually return to.\n\nIs such a use of the pointer in the structure a cause for an\nundefined behaviour?\n\n\n"},{"id":"455993","messageId":"CAPUEsph5udmc7pyza0WdhXDFfHSm96Xwt3CnEiPnSDSqY6fOUg@mail.gmail.com","threadId":"57858","inReplyTo":"xmqqo7zmcydv.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-05-25T02:02:15Z","receivedAt":"2022-05-25T02:02:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Please, I apologize in advance because I am making statements way\nabove my paygrade and without the relevant foundation (because as I\nmentioned, I looked at it briefly a while ago, and hadn't been able to\nget the time to complete my analysis).\n\nI did read all your analysis and while as Aevar pointed out, I might\nbe mistaken, or not making myself clear enough, it is not intentional\nand if told to do so, will extract myself from this thread until I can\ndo a full analysis.\n\nOn Tue, May 24, 2022 at 4:19 PM Junio C Hamano wrote:\n>\n> Is such a use of the pointer in the structure a cause for an\n> undefined behaviour?\n\nIf you make an IMHO overly strict reading of the standard (which it\nwould seem gcc12 implementers might have done) then ANY access to a\npointer that might be out of scope is Undefined Behaviour.\n\ngcc doesn't know what happens to the variable after it gets shipped\nout of this function inside that opaque structure that is passed\naround between threads in curl, so it MIGHT reasonably assume that:\n\n1) There is a layering violation somewhere and another thread is\nmodifying that field to point it to a different thread local from the\nsame function.\n2) Once it gets the value back, then it can assume that reading that\npointer might be undefined behaviour because it might be coming from a\ndifferent stack frame than ours (after all, any sane developer would\nstore instead a flag)\n3) since the if uses a condition that is UB then it can optimize it out\n\nI can't see any other reason why in the original code, with the if,\nthe warning was still being triggered, but not without it.\n\nBTW while trying to debug gcc to find out if this is a bug or a lost\nopportunity for optimization of something else, I noticed that it\nwould only trigger in cases where the structure was shipped outside\nthe function through another function, not when it was returned back\nfrom it, for example.\n\nI don't even have gcc-12 in the current workstation I am using, but\nwould prepare a machine with it and debug further if you think giving\na more complete answer is necessary.\n\nCarlo\n"},{"id":"456005","messageId":"165346968167.4313.13200529030347354219.git@grubix.eu","threadId":"57858","inReplyTo":"xmqqh75eef0f.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-05-25T09:08:01Z","receivedAt":"2022-05-25T09:17:33Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 2022-05-25 00:34:40:\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > It doesn't mean that GCC has additionally proved that we'll later used\n> > it in a way that will have a meaningful impact on the behavior of our\n> > program, or even that it's tried to do that. See an excerpt from the GCC\n> > code (a comment) in [1].\n> \n> But that means the warning just as irrelevant as \"you stored 438 to\n> this integer variable\". Sure, there may be cases where that integer\n> variable should not exceed 400 and if the compiler can tell us that,\n> that would be a valuable help to developers. \n\nAn integer can hold 438 perfectly, without any help by carefully\ndesigned code.\n\n> But \"you stored an\n> address of an object that can go out of scope in another object\n> whose lifetime lasts beyond the scope\" alone is not, without \"and\n> the caller that passed the latter object later dereferences that\n> address here\". We certainly shouldn't -Werror on such a warning\n> and bend our code because of it.\n\nA global variable cannot hold a meaningful pointer to a local variable,\nunless the carefully designed code helps. So that \"analogy\" rather\nhighlights the essential difference, unless you think about a pointer\nas \"just some number\" rather than \"something that can be dereferenced\".\n\n[read global as outer scope, local as inner scope for simplicity]\n\nCommon practice is not necessarily good practice. In a traditional C\nmindset, everything around pointers and memory management is doomed to\nboom unless the code is designed carefully and \"you know what you are\ndoing\". I'm not indicating that you do not - on the contrary, you very\nwell do, as your detailed analysis of the code flow shows. At the same\ntime, it shows that we cannot be certain about that piece of code\nwithout that detailed expert analysis.\n\nC is not C++ nor rust, nor should it be; but the warnings and errors in\nnewer standards typically try to avoid those pitfalls by making sure\nthat, e.g., pointers do not go stale for \"obvious reasons\". They might\nmissflag cases where this is preempted for non-obvious reasons, but forcing\nyou to be explicit (more obvious) in your code is a good thing,\nespecially for maintainability of the code base.\n\nPointing from outer scope to memory in an inner scope should be a no-go;\nthat's what this error is about. Unsetting that pointer (by setting the\npointer to NULL) right before the inner scope ends is exactly the\nright solution. If this \"breaks\" the code, the code is broken already.\n\nIronically, my original one line patch seems to work here, as your\ndetailed analysis shows. Truth in advertising: I arrived at that patch\nafter a considerably less detailed analysis ;)\n\nMichael\n"},{"id":"456037","messageId":"nycvar.QRO.7.76.6.2205251111300.352@tvgsbejvaqbjf.bet","threadId":"57858","inReplyTo":"xmqqleuqefa4.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-05-25T10:07:45Z","receivedAt":"2022-05-25T10:08:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 May 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n> >> I _think_ we can even get away by not doing anything to\n> >> slot->finished at the end of run_active_slot(), as we are not\n> >> multi-threaded and the callee only returns to the caller, but if it\n> >> helps pleasing the warning compiler, I'd prefer the simplest\n> >> workaround, perhaps with an unconditional clearing there?\n> >\n> > I'll admit I haven't fully looked into this again, but does anything in\n> > the subsequent analysis suggest that my original patch wouldn't be a\n> > working solution to this, still:\n> > https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ ?\n>\n> I traced _one_ code path as a demonstration to show why the current\n> \"slot->finished = &finished\" based solution works.\n>\n> But I think what we need is to demonstrate a code path in the old\n> version that shows why the old slot->in_use would not have worked\n> and the slot->finished was needed, and demonstrate why it NO LONGER\n> is the case in today's code. Without that, especially with the\n> latter, I cannot take the \"just revert 16-year old bugfix because a\n> new compiler throws a warning related to multi-threaded code to it,\n> even though we are strictly single-threaded\" as a serious solution.\n>\n> And because I do not think I've seen anybody has done that necessary\n> digging, I would still prefer the \"if the compiler somehow cares,\n> then let's clear the finished member once we are done with it\" much\n> better than \"we do not know why but we somehow think we can do\n> without this bugfix, even though we wouldn't be making noises about\n> this piece of code if a new compiler did not start emitting a\n> warning\".\n\nThe commit in question is baa7b67d091 (HTTP slot reuse fixes, 2006-03-10),\nand I did look around in the Git mailing list archives for mails that were\nsent around the same date, but did not see much that would help understand\nthe context, except that the patch series clearly talks about `http-push`:\nhttps://lore.kernel.org/git/20060311041749.GB3997@reactrix.com/\n\nThe thing about `http-push` is that it adds a \"fill function\" that is\nexecuted in `fill_active_slots()`, which is called in turn by\n`step_active_slots()`, which, as you will recall, is called within that\nbusy loop in `run_active_slot()`.\n\nAnd that \"fill function\" is where it starts to get interesting.\n\nIt's called `fill_active_slot()`:\nhttps://github.com/git/git/blob/v2.36.1/http-push.c#L604-L625\n\nThis function starts requests, such as fetching loose objects, starting a\nPUT or a MKCOL. Notably, though, `fill_active_slot()` does not wait for\nthe request to finish. In other words, it will potentially reuse the\ncurrent slot if it was _just_ marked as no longer in use, and then the\ncode flow will eventually return to that loop in `run_active_slot()`, with\nany reused slot still being marked as `in_use`.\n\nSo yes, reverting that commit would reintroduce the regression, and I am\nvery happy that we now have a grip on this Chesterton's Fence.\n\nThis same analysis, of course, also puts a nail into the coffin of the\n`reserved_for_use` idea because while it would fix the reuse bug, it would\nunnecessarily squat on slots that might well be needed.\n\nCiao,\nDscho\n"},{"id":"456065","messageId":"220525.86bkvlu4bx.gmgdl@evledraar.gmail.com","threadId":"57858","inReplyTo":"xmqqh75eef0f.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-25T13:27:57Z","receivedAt":"2022-05-25T13:31:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 24 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> It doesn't mean that GCC has additionally proved that we'll later used\n>> it in a way that will have a meaningful impact on the behavior of our\n>> program, or even that it's tried to do that. See an excerpt from the GCC\n>> code (a comment) in [1].\n>\n> But that means the warning just as irrelevant as \"you stored 438 to\n> this integer variable\". Sure, there may be cases where that integer\n> variable should not exceed 400 and if the compiler can tell us that,\n> that would be a valuable help to developers. But \"you stored an\n> address of an object that can go out of scope in another object\n> whose lifetime lasts beyond the scope\" alone is not, without \"and\n> the caller that passed the latter object later dereferences that\n> address here\". We certainly shouldn't -Werror on such a warning\n> and bend our code because of it.\n\nI think it says something that 1) we had exactly one of these in our\ncodebase 2) as we've discussed the pointer isn't actually *needed*\noutside the scope of the function, it's just left-over.\n\nNow, if it were used, e.g. let's say we had some code that took the\nstruct and inspected its members we'd likely have a segfault here, or\nworse it would \"work\", but only on the platforms we'd test at first.\n\nWhich isn't the case with a leftover \"int finished\" holding a 438.\n\nThe point of this warning, like so many others, is to ask \"hey, do you\nreally need to be running around with this particular pair of\nscissors?\".\n\n"},{"id":"456107","messageId":"xmqqr14h8t10.fsf@gitster.g","threadId":"57858","inReplyTo":"nycvar.QRO.7.76.6.2205251111300.352@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-25T16:40:43Z","receivedAt":"2022-05-25T16:40:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This same analysis, of course, also puts a nail into the coffin of the\n> `reserved_for_use` idea because while it would fix the reuse bug, it would\n> unnecessarily squat on slots that might well be needed.\n\nIt is like that an in-kernel structure that represents a process has\nto stay around in the zombie state until its exit status is culled.\nWith s/reserved_for_use/zombie/ the name of the new member would\nmake more sense ;-)\n\nWith the \"slot->finished\" trick, compared to the approach to delay\nthe reuse, we can reuse them a bit earlier, but because I do not\nthink we accumulate unbounded number of these zombie requests, and\nwhen we run out the active slots in the active queue, and because\nget_active_slot() will allocate a new one, the wastage might not be\ntoo bad.\n\nSo, I am not sure if it is that bad to be called a nail in the\ncoffin.\n\nIn any case, https://github.com/git/git/actions/runs/2381379417 is\nthe run with the single liner \"clear slot->finished before leaving\"\nwith your other 3 gcc12 fixes. The tests are not clean because we\nhave linux-leaks complaining on ds/bundle-uri-more RFC patches and\nwin test (9) seems to have issues with t7527 (fsmonitor), but I am\ntaking the fact that any of the \"win test\" jobs even start as an\nevidence that we have pleased gcc12 enough to get there?\n\nThanks.\n"},{"id":"456174","messageId":"1452491s-8415-9182-4015-q22sn63234p2@unkk.fr","threadId":"57858","inReplyTo":"xmqqa6b6j04b.fsf@gitster.g","subject":"Re: [PATCH] http.c: clear the 'finished' member once we are done with it","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2022-05-26T14:15:09Z","receivedAt":"2022-05-26T14:15:20Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 24 May 2022, Junio C Hamano wrote:\n\n> It always is nice to have the authority/expert readily answering our stupid \n> questions ;-)\n\nI probably miss a few here and there, but I do keep an eye out for curl \nrelated subjects where I can help. After all, I'm git fan and user myself.\n\n-- \n\n / daniel.haxx.se\n"}]}