{"thread":{"id":"57510","subject":"[PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","startedAt":"2022-03-04T13:37:16Z","lastAt":"2022-04-05T21:50:11Z","messageCount":19,"participants":["Elia Pinto","Junio C Hamano","Carlo Marcelo Arenas Belón","Eric Sunshine","Ævar Arnfjörð Bjarmason","Carlo Arenas","Phillip Wood"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"450396","messageId":"20220304133702.26706-1-gitter.spiros@gmail.com","threadId":"57510","inReplyTo":null,"subject":"[PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-03-04T13:37:02Z","receivedAt":"2022-03-04T13:37:16Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\nvariables have been replaced by GLIBC_TUNABLES.  Also the new\nglibc requires that you preload a library called libc_malloc_debug.so\nto get these features.\n\nUsing the ordinary glibc system variable detect if this is glibc >= 2.34 and\nuse GLIBC_TUNABLES and the new library.\n\nThis patch was inspired by a Richard W.M. Jones ndbkit patch\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\nThis is the third version of the patch.\n\nCompared to the second version[1], the code is further simplified,\neliminating a case statement and modifying a string statement.\n\n[1] https://www.spinics.net/lists/git/msg433917.html\n\n t/test-lib.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9af5fb7674..4d10646015 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -550,9 +550,25 @@ else\n \tsetup_malloc_check () {\n \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n+\t\t_GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n+\t\texpr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\t\tthen\n+\t\t\tg=\n+\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n+\t\t\tfor t in \\\n+\t\t\t\tglibc.malloc.check=1 \\\n+\t\t\t\tglibc.malloc.perturb=165\n+\t\t\tdo\n+\t\t\t\tg=\"${g#:}:$t\"\n+\t\t\tdone\n+\t\t\tGLIBC_TUNABLES=$g\n+\t\t\texport LD_PRELOAD GLIBC_TUNABLES\n+\t\tfi\n \t}\n \tteardown_malloc_check () {\n \t\tunset MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\tunset LD_PRELOAD GLIBC_TUNABLES\n \t}\n fi\n\n--\n2.35.1\n\n"},{"id":"450437","messageId":"xmqqfsnx1oc9.fsf@gitster.g","threadId":"57510","inReplyTo":"20220304133702.26706-1-gitter.spiros@gmail.com","subject":"Re: [PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T19:59:02Z","receivedAt":"2022-03-04T20:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\n> variables have been replaced by GLIBC_TUNABLES.  Also the new\n> glibc requires that you preload a library called libc_malloc_debug.so\n> to get these features.\n>\n> Using the ordinary glibc system variable detect if this is glibc >= 2.34 and\n> use GLIBC_TUNABLES and the new library.\n>\n> This patch was inspired by a Richard W.M. Jones ndbkit patch\n>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n> This is the third version of the patch.\n>\n> Compared to the second version[1], the code is further simplified,\n> eliminating a case statement and modifying a string statement.\n\nThanks; will queue.  Let's declare victory and merge it down to\n'next' and 'master/main'.\n"},{"id":"450723","messageId":"20220308113305.39395-1-carenas@gmail.com","threadId":"57510","inReplyTo":"20220304133702.26706-1-gitter.spiros@gmail.com","subject":"[PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2022-03-08T11:33:05Z","receivedAt":"2022-03-08T11:34:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Restrict the glibc version to a single version number and compare it\narithmetically against the base glibc version to avoid accidentally\nmatching against \"2.3\" and better supporting versions like \"2.34.9000\"\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n t/test-lib.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8e59c58e7e7..f624f87eb81 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -518,9 +518,9 @@ else\n \tsetup_malloc_check () {\n \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n-\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n-\t\t   _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n-\t\t   expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n+\t\tlocal _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n+\t\tif echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n+\t\t\tawk '{ if ($2 - 2.34 < 0) exit 1 }'\n \t\tthen\n \t\t\tg=\n \t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n-- \n2.35.1.505.g27486cd1b2d\n\n"},{"id":"450815","messageId":"CAPig+cT3TNFBMesYvYoncawfBdLqKL971SoP_J7F9FgnL10Eqw@mail.gmail.com","threadId":"57510","inReplyTo":"CAPig+cSNgQ7SEYk9M=L7z0G=hteTdupKS6sHJL8T7zEp=zkLEA@mail.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-03-08T23:58:03Z","receivedAt":"2022-03-09T01:13:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 8, 2022 at 6:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n> <carenas@gmail.com> wrote:\n> > Restrict the glibc version to a single version number and compare it\n> > arithmetically against the base glibc version to avoid accidentally\n> > matching against \"2.3\" and better supporting versions like \"2.34.9000\"\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> > ---\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > @@ -518,9 +518,9 @@ else\n> > -               if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> > -                  _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> > -                  expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> > +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n> > +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n> > +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n>\n> No need for `cut` since `awk` can accomplish the same by itself.\n>\n>     if echo \"$_GLIBC_VERSION\" | awk '/^glibc / { if ($2 - 2.34 < 0) exit 1 }'\n>\n> should work, I would think.\n\nNevermind, I forgot you want to better support \"2.34.9000\" matches.\nThough, awk should still be able to do so on its own, one would\nexpect, but not too important.\n"},{"id":"450817","messageId":"CAPig+cSNgQ7SEYk9M=L7z0G=hteTdupKS6sHJL8T7zEp=zkLEA@mail.gmail.com","threadId":"57510","inReplyTo":"20220308113305.39395-1-carenas@gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-03-08T23:55:59Z","receivedAt":"2022-03-09T01:13:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n> Restrict the glibc version to a single version number and compare it\n> arithmetically against the base glibc version to avoid accidentally\n> matching against \"2.3\" and better supporting versions like \"2.34.9000\"\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> @@ -518,9 +518,9 @@ else\n> -               if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> -                  _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> -                  expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n> +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n> +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n\nNo need for `cut` since `awk` can accomplish the same by itself.\n\n    if echo \"$_GLIBC_VERSION\" | awk '/^glibc / { if ($2 - 2.34 < 0) exit 1 }'\n\nshould work, I would think.\n"},{"id":"450818","messageId":"CAPig+cSUTaPRvALJyJ8AxNB4wMFLyaWBOa8f+_8K6quPbxTT5A@mail.gmail.com","threadId":"57510","inReplyTo":"CAPig+cT3TNFBMesYvYoncawfBdLqKL971SoP_J7F9FgnL10Eqw@mail.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-03-09T00:05:47Z","receivedAt":"2022-03-09T01:20:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 8, 2022 at 6:58 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Mar 8, 2022 at 6:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n> > <carenas@gmail.com> wrote:\n> > > +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n> > > +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n> > > +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n> >\n> > No need for `cut` since `awk` can accomplish the same by itself.\n> >\n> >     if echo \"$_GLIBC_VERSION\" | awk '/^glibc / { if ($2 - 2.34 < 0) exit 1 }'\n> >\n> > should work, I would think.\n>\n> Nevermind, I forgot you want to better support \"2.34.9000\" matches.\n> Though, awk should still be able to do so on its own, one would\n> expect, but not too important.\n\nThis seems to work, though it's getting a bit verbose:\n\n    awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n- 2.34 < 0) exit 1 }'\n"},{"id":"450893","messageId":"xmqqv8wnm30q.fsf@gitster.g","threadId":"57510","inReplyTo":"CAPig+cSUTaPRvALJyJ8AxNB4wMFLyaWBOa8f+_8K6quPbxTT5A@mail.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-09T17:47:33Z","receivedAt":"2022-03-09T17:47:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Mar 8, 2022 at 6:58 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Mar 8, 2022 at 6:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> > On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n>> > <carenas@gmail.com> wrote:\n>> > > +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n>> > > +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n>> > > +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n>> >\n>> > No need for `cut` since `awk` can accomplish the same by itself.\n>> >\n>> >     if echo \"$_GLIBC_VERSION\" | awk '/^glibc / { if ($2 - 2.34 < 0) exit 1 }'\n>> >\n>> > should work, I would think.\n>>\n>> Nevermind, I forgot you want to better support \"2.34.9000\" matches.\n>> Though, awk should still be able to do so on its own, one would\n>> expect, but not too important.\n>\n> This seems to work, though it's getting a bit verbose:\n>\n>     awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n> - 2.34 < 0) exit 1 }'\n\nIf we are losing \"cut\" (which I think is a good thing to do), we\nprobably can lose the pipe, too and refer to $_GLIBC_VERSION as an\nelement in ARGV[] and make the command used as \"if\" condition to a\nsingle \"awk\" script?\n\nIn general it is a good discipline to question a pipeline that\npreprocesses input fed to a script written in a language with full\nprogramming power like awk and perl (and to lessor extent, sed) to\nsee if we can come up with a simpler solution without pipeline\nhelping to solve what these languages are invented to solve, and I\nvery much appreciate your exploration ;-)\n\nThanks.\n"},{"id":"450928","messageId":"220309.86pmmulw77.gmgdl@evledraar.gmail.com","threadId":"57510","inReplyTo":"xmqqv8wnm30q.fsf@gitster.g","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-09T20:07:38Z","receivedAt":"2022-03-09T20:14:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 09 2022, Junio C Hamano wrote:\n\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> On Tue, Mar 8, 2022 at 6:58 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Tue, Mar 8, 2022 at 6:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> > On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n>>> > <carenas@gmail.com> wrote:\n>>> > > +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n>>> > > +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n>>> > > +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n>>> >\n>>> > No need for `cut` since `awk` can accomplish the same by itself.\n>>> >\n>>> >     if echo \"$_GLIBC_VERSION\" | awk '/^glibc / { if ($2 - 2.34 < 0) exit 1 }'\n>>> >\n>>> > should work, I would think.\n>>>\n>>> Nevermind, I forgot you want to better support \"2.34.9000\" matches.\n>>> Though, awk should still be able to do so on its own, one would\n>>> expect, but not too important.\n>>\n>> This seems to work, though it's getting a bit verbose:\n>>\n>>     awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n>> - 2.34 < 0) exit 1 }'\n>\n> If we are losing \"cut\" (which I think is a good thing to do), we\n> probably can lose the pipe, too and refer to $_GLIBC_VERSION as an\n> element in ARGV[] and make the command used as \"if\" condition to a\n> single \"awk\" script?\n>\n> In general it is a good discipline to question a pipeline that\n> preprocesses input fed to a script written in a language with full\n> programming power like awk and perl (and to lessor extent, sed) to\n> see if we can come up with a simpler solution without pipeline\n> helping to solve what these languages are invented to solve, and I\n> very much appreciate your exploration ;-)\n\nI agree :) But the first language we've got here is C. Rather than\nfiddle around with getconf, awk/sed etc. why not just the rather\ntrivial:\n\t\n\tdiff --git a/Makefile b/Makefile\n\tindex 6f0b4b775fe..f566c9c5df2 100644\n\t--- a/Makefile\n\t+++ b/Makefile\n\t@@ -732,6 +732,7 @@ TEST_BUILTINS_OBJS += test-parse-pathspec-file.o\n\t TEST_BUILTINS_OBJS += test-partial-clone.o\n\t TEST_BUILTINS_OBJS += test-path-utils.o\n\t TEST_BUILTINS_OBJS += test-pcre2-config.o\n\t+TEST_BUILTINS_OBJS += test-glibc-config.o\n\t TEST_BUILTINS_OBJS += test-pkt-line.o\n\t TEST_BUILTINS_OBJS += test-prio-queue.o\n\t TEST_BUILTINS_OBJS += test-proc-receive.o\n\tdiff --git a/t/helper/test-glibc-config.c b/t/helper/test-glibc-config.c\n\tnew file mode 100644\n\tindex 00000000000..3c3cc2a8ba5\n\t--- /dev/null\n\t+++ b/t/helper/test-glibc-config.c\n\t@@ -0,0 +1,12 @@\n\t+#include \"test-tool.h\"\n\t+#include \"cache.h\"\n\t+\n\t+int cmd__glibc_config(int argc, const char **argv)\n\t+{\n\t+#ifdef __GNU_LIBRARY__\n\t+\tprintf(\"%d\\n%d\\n\", __GLIBC__, __GLIBC_MINOR__);\n\t+\treturn 0;\n\t+#else\n\t+\treturn 1;\n\t+#endif\n\t+}\n\tdiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n\tindex e6ec69cf326..c01422f5cab 100644\n\t--- a/t/helper/test-tool.c\n\t+++ b/t/helper/test-tool.c\n\t@@ -35,6 +35,7 @@ static struct test_cmd cmds[] = {\n\t \t{ \"genrandom\", cmd__genrandom },\n\t \t{ \"genzeros\", cmd__genzeros },\n\t \t{ \"getcwd\", cmd__getcwd },\n\t+\t{ \"glibc-config\", cmd__glibc_config },\n\t \t{ \"hashmap\", cmd__hashmap },\n\t \t{ \"hash-speed\", cmd__hash_speed },\n\t \t{ \"index-version\", cmd__index_version },\n\tdiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n\tindex 20756eefdda..dc061bc7833 100644\n\t--- a/t/helper/test-tool.h\n\t+++ b/t/helper/test-tool.h\n\t@@ -26,6 +26,7 @@ int cmd__fast_rebase(int argc, const char **argv);\n\t int cmd__genrandom(int argc, const char **argv);\n\t int cmd__genzeros(int argc, const char **argv);\n\t int cmd__getcwd(int argc, const char **argv);\n\t+int cmd__glibc_config(int argc, const char **argv);\n\t int cmd__hashmap(int argc, const char **argv);\n\t int cmd__hash_speed(int argc, const char **argv);\n\t int cmd__index_version(int argc, const char **argv);\n\nI mainly copied the pcre2-config template I added in 95ca1f987ed\n(grep/pcre2: better support invalid UTF-8 haystacks, 2021-01-24), which\nlikewise would have been quite a bit more complex to do from non-C.\n"},{"id":"451183","messageId":"CAPig+cQT3801Fok9Uvk1TOs5WscdM+hRX7x3sW38izRqAR1N1A@mail.gmail.com","threadId":"57510","inReplyTo":"xmqqv8wnm30q.fsf@gitster.g","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-03-11T23:02:57Z","receivedAt":"2022-03-11T23:04:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 9, 2022 at 12:47 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > On Tue, Mar 8, 2022 at 6:58 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> >> > On Tue, Mar 8, 2022 at 6:44 PM Carlo Marcelo Arenas Belón\n> >> > <carenas@gmail.com> wrote:\n> >> > > +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n> >> > > +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n> >> > > +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n> >\n> > This seems to work, though it's getting a bit verbose:\n> >\n> >     awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n> > - 2.34 < 0) exit 1 }'\n>\n> If we are losing \"cut\" (which I think is a good thing to do), we\n> probably can lose the pipe, too and refer to $_GLIBC_VERSION as an\n> element in ARGV[] and make the command used as \"if\" condition to a\n> single \"awk\" script?\n\nErm, something like this perhaps?\n\n    awk 'BEGIN { split(ARGV[1], v, \"[ .]\"); if (v[1]==\"glibc\" &&\nsprintf(\"%s.%s\", v[2], v[3]) - 2.34 < 0) exit 1 }'\n\n> In general it is a good discipline to question a pipeline that\n> preprocesses input fed to a script written in a language with full\n> programming power like awk and perl (and to lessor extent, sed) to\n> see if we can come up with a simpler solution without pipeline\n> helping to solve what these languages are invented to solve, and I\n> very much appreciate your exploration ;-)\n\nYup, although in this case, the pure-awk solution may not convey the\nintent as easily as the original posted by Carlo which had the benefit\nof being perhaps easier to digest.\n"},{"id":"451184","messageId":"CAPig+cQNeTAvWHm2GUGc2i=FKF2V6Gqkmmsw4kDOTzrSYEbgxA@mail.gmail.com","threadId":"57510","inReplyTo":"220309.86pmmulw77.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-03-11T23:06:35Z","receivedAt":"2022-03-11T23:08:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 9, 2022 at 3:14 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> On Wed, Mar 09 2022, Junio C Hamano wrote:\n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> >> This seems to work, though it's getting a bit verbose:\n> >>\n> >>     awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n> >> - 2.34 < 0) exit 1 }'\n> >\n> > In general it is a good discipline to question a pipeline that\n> > preprocesses input fed to a script written in a language with full\n> > programming power like awk and perl (and to lessor extent, sed) to\n> > see if we can come up with a simpler solution without pipeline\n> > helping to solve what these languages are invented to solve, and I\n> > very much appreciate your exploration ;-)\n>\n> I agree :) But the first language we've got here is C. Rather than\n> fiddle around with getconf, awk/sed etc. why not just the rather\n> trivial:\n>\n>         +#include \"test-tool.h\"\n>         +#include \"cache.h\"\n>         +\n>         +int cmd__glibc_config(int argc, const char **argv)\n>         +{\n>         +#ifdef __GNU_LIBRARY__\n>         +       printf(\"%d\\n%d\\n\", __GLIBC__, __GLIBC_MINOR__);\n>         +       return 0;\n>         +#else\n>         +       return 1;\n>         +#endif\n>         +}\n\nIt feels overkill to add this just for this one case which is\notherwise done easily enough with existing shell tools.\n\nThat said, perhaps I'm missing something, but I don't see how this\nsolution helps us get away from the need for `expr`, `awk`, or `perl`\nsince one of those languages would be needed to perform the arithmetic\ncomparison (checking if glibc is >= 2.34).\n"},{"id":"451217","messageId":"220312.86o82bfo7x.gmgdl@evledraar.gmail.com","threadId":"57510","inReplyTo":"CAPig+cQNeTAvWHm2GUGc2i=FKF2V6Gqkmmsw4kDOTzrSYEbgxA@mail.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-12T10:38:33Z","receivedAt":"2022-03-12T10:40:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 11 2022, Eric Sunshine wrote:\n\n> On Wed, Mar 9, 2022 at 3:14 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>> On Wed, Mar 09 2022, Junio C Hamano wrote:\n>> > Eric Sunshine <sunshine@sunshineco.com> writes:\n>> >> This seems to work, though it's getting a bit verbose:\n>> >>\n>> >>     awk '/^glibc / { split($2,v,\".\"); if (sprintf(\"%s.%s\", v[1], v[2])\n>> >> - 2.34 < 0) exit 1 }'\n>> >\n>> > In general it is a good discipline to question a pipeline that\n>> > preprocesses input fed to a script written in a language with full\n>> > programming power like awk and perl (and to lessor extent, sed) to\n>> > see if we can come up with a simpler solution without pipeline\n>> > helping to solve what these languages are invented to solve, and I\n>> > very much appreciate your exploration ;-)\n>>\n>> I agree :) But the first language we've got here is C. Rather than\n>> fiddle around with getconf, awk/sed etc. why not just the rather\n>> trivial:\n>>\n>>         +#include \"test-tool.h\"\n>>         +#include \"cache.h\"\n>>         +\n>>         +int cmd__glibc_config(int argc, const char **argv)\n>>         +{\n>>         +#ifdef __GNU_LIBRARY__\n>>         +       printf(\"%d\\n%d\\n\", __GLIBC__, __GLIBC_MINOR__);\n>>         +       return 0;\n>>         +#else\n>>         +       return 1;\n>>         +#endif\n>>         +}\n>\n> It feels overkill to add this just for this one case which is\n> otherwise done easily enough with existing shell tools.\n>\n> That said, perhaps I'm missing something, but I don't see how this\n> solution helps us get away from the need for `expr`, `awk`, or `perl`\n> since one of those languages would be needed to perform the arithmetic\n> comparison (checking if glibc is >= 2.34).\n\nIn this case they're \\n delimited, so we can use the shell's native\nwhitespace splitting to $(())-compare $1 and $2.\n\nBut probably better is to just amend that to call it as \"test-tool libc\nis-glibc-2.34-or-newer\" or whatever. Then just do:\n\n\tif (__GLIBC__ > 2 || (__GLIBC__ == 2 && 34 >= __GLIBC_MINOR__))\n\t\treturn 0;\n\treturn 1;\n"},{"id":"451231","messageId":"xmqqtuc2lhis.fsf@gitster.g","threadId":"57510","inReplyTo":"220312.86o82bfo7x.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-13T02:20:59Z","receivedAt":"2022-03-13T02:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But probably better is to just amend that to call it as \"test-tool libc\n> is-glibc-2.34-or-newer\" or whatever. Then just do:\n>\n> \tif (__GLIBC__ > 2 || (__GLIBC__ == 2 && 34 >= __GLIBC_MINOR__))\n> \t\treturn 0;\n> \treturn 1;\n\nYuck.  Then we'd have yet another libc-is-glibc-2.36-or-newer\noption, too, in the future?\n\n\n"},{"id":"451232","messageId":"CAPUEspgdmaztSShPd6vJpT7801_czRuBt_QaPu_W2JGOw+UqrQ@mail.gmail.com","threadId":"57510","inReplyTo":"xmqqtuc2lhis.fsf@gitster.g","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2022-03-13T02:37:27Z","receivedAt":"2022-03-13T02:37:44Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Mar 12, 2022 at 6:21 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n> > But probably better is to just amend that to call it as \"test-tool libc\n> > is-glibc-2.34-or-newer\" or whatever. Then just do:\n> >\n> >       if (__GLIBC__ > 2 || (__GLIBC__ == 2 && 34 >= __GLIBC_MINOR__))\n> >               return 0;\n> >       return 1;\n>\n> Yuck.  Then we'd have yet another libc-is-glibc-2.36-or-newer\n> option, too, in the future?\n\nLuckily that won't be needed, as this the original version (with expr)\nis practically good enough even if it might be a little odd looking\nand incorrect for 2.4 <= glibc <= 2.9 (which are over 10 years old).\n\n  $ expr 2.34 \\<= \"2.34.9000\"\n  1\n  $ expr 2.34 \\<= \"\"\n  0\n\nApologies for the confusion, and feel free to drop this patch\n\nCarlo\n"},{"id":"451235","messageId":"xmqqilsiwbjo.fsf@gitster.g","threadId":"57510","inReplyTo":"CAPUEspgdmaztSShPd6vJpT7801_czRuBt_QaPu_W2JGOw+UqrQ@mail.gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-13T07:34:35Z","receivedAt":"2022-03-13T07:34:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> On Sat, Mar 12, 2022 at 6:21 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>>\n>> > But probably better is to just amend that to call it as \"test-tool libc\n>> > is-glibc-2.34-or-newer\" or whatever. Then just do:\n>> >\n>> >       if (__GLIBC__ > 2 || (__GLIBC__ == 2 && 34 >= __GLIBC_MINOR__))\n>> >               return 0;\n>> >       return 1;\n>>\n>> Yuck.  Then we'd have yet another libc-is-glibc-2.36-or-newer\n>> option, too, in the future?\n>\n> Luckily that won't be needed, as this the original version (with expr)\n> is practically good enough even if it might be a little odd looking\n> and incorrect for 2.4 <= glibc <= 2.9 (which are over 10 years old).\n>\n>   $ expr 2.34 \\<= \"2.34.9000\"\n>   1\n>   $ expr 2.34 \\<= \"\"\n>   0\n\nYeah, that is good.\n\nWhat I was trying to get at was to extend Ævar's one trivially to\n\n  $ test-tool libc-is-at-or-later-than 2.34\n\nso that we can deal with\n\n  $ test-tool libc-is-at-or-later-than 2.36\n\nfor free, but if we do not have to do anything, that is even better\n;-)\n\n"},{"id":"451258","messageId":"CA+EOSBm1wfO-RPJHiHZiUxokdt8MW3ufoCk_aJ4o7O=x1_nwDQ@mail.gmail.com","threadId":"57510","inReplyTo":"20220308113305.39395-1-carenas@gmail.com","subject":"Re: [PATCH] test-lib.sh: use awk instead of expr for a POSIX non integer check","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-03-13T19:02:33Z","receivedAt":"2022-03-13T19:02:44Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Il giorno mar 8 mar 2022 alle ore 12:34 Carlo Marcelo Arenas Belón\n<carenas@gmail.com> ha scritto:\n>\n> Restrict the glibc version to a single version number and compare it\n> arithmetically against the base glibc version to avoid accidentally\n> matching against \"2.3\" and better supporting versions like \"2.34.9000\"\n>\n\nI didn't understand the problem. How glibc names the versions is known:\n\nhttps://sourceware.org/glibc/wiki/Glibc%20Timeline\n\nWhat is wrong with the expr statement ?\n\n+ expr 2.34 '<=' 2.35\n1\n+ expr 2.34 '<=' 2.34\n1\n+ expr 2.34 '<=' 2.33\n0\n+ expr 2.34 '<=' 2.32\n0\n+ expr 2.34 '<=' 2.31\n0\n+ expr 2.34 '<=' 2.30\n0\n+ expr 2.34 '<=' 2.29\n0\n+ expr 2.34 '<=' 2.28\n0\n+ expr 2.34 '<=' 2.27\n0\n+ expr 2.34 '<=' 2.26\n0\n+ expr 2.34 '<=' 2.25\n0\n+ expr 2.34 '<=' 2.24\n0\n+ expr 2.34 '<=' 2.23\n0\n+ expr 2.34 '<=' 2.22\n0\n+ expr 2.34 '<=' 2.21\n0\n+ expr 2.34 '<=' 2.20\n0\n+ expr 2.34 '<=' 2.19\n0\n+ expr 2.34 '<=' 2.18\n0\n+ expr 2.34 '<=' 2.17\n0\n+ expr 2.34 '<=' 2.16\n0\n+ expr 2.34 '<=' 2.15\n0\n+ expr 2.34 '<=' 2.13\n\n\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  t/test-lib.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 8e59c58e7e7..f624f87eb81 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -518,9 +518,9 @@ else\n>         setup_malloc_check () {\n>                 MALLOC_CHECK_=3 MALLOC_PERTURB_=165\n>                 export MALLOC_CHECK_ MALLOC_PERTURB_\n> -               if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> -                  _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> -                  expr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +               local _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null)\n> +               if echo \"$_GLIBC_VERSION\" | cut -d. -f1-2 |\n> +                       awk '{ if ($2 - 2.34 < 0) exit 1 }'\n>                 then\n>                         g=\n>                         LD_PRELOAD=\"libc_malloc_debug.so.0\"\n> --\n> 2.35.1.505.g27486cd1b2d\n>\n"},{"id":"453060","messageId":"975e203d-6bd3-f5ea-c21b-3e7518a04bb9@gmail.com","threadId":"57510","inReplyTo":"20220304133702.26706-1-gitter.spiros@gmail.com","subject":"Re: [PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-04T20:39:03Z","receivedAt":"2022-04-04T21:23:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/03/2022 13:37, Elia Pinto wrote:\n> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\n> variables have been replaced by GLIBC_TUNABLES.  Also the new\n> glibc requires that you preload a library called libc_malloc_debug.so\n> to get these features.\n> \n> Using the ordinary glibc system variable detect if this is glibc >= 2.34 and\n> use GLIBC_TUNABLES and the new library.\n> \n> This patch was inspired by a Richard W.M. Jones ndbkit patch\n> \n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n> This is the third version of the patch.\n> \n> Compared to the second version[1], the code is further simplified,\n> eliminating a case statement and modifying a string statement.\n> \n> [1] https://www.spinics.net/lists/git/msg433917.html\n> \n>   t/test-lib.sh | 16 ++++++++++++++++\n>   1 file changed, 16 insertions(+)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 9af5fb7674..4d10646015 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -550,9 +550,25 @@ else\n>   \tsetup_malloc_check () {\n>   \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n>   \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n> +\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n> +\t\t_GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n> +\t\texpr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n> +\t\tthen\n> +\t\t\tg=\n> +\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n\nWhen compiling with \"SANITIZE = address,leak\" this use of LD_PRELOAD \nmakes the tests fail with\n\n==9750==ASan runtime does not come first in initial library list; you \nshould either link runtime to your application or manually preload it \nwith LD_PRELOAD.\n\nbecause libc_malloc_debug.so is being loaded before libasan.so. If I set \nTEST_NO_MALLOC_CHECK=1 when I run the tests then ASAN does not complain \nbut it would be nicer if I did not have to do that. I'm confused as to \nwhy the CI leak tests are running fine - am I missing something with my \nsetup?\n\nBest Wishes\n\nPhillip\n"},{"id":"453112","messageId":"220405.86k0c3lt2l.gmgdl@evledraar.gmail.com","threadId":"57510","inReplyTo":"975e203d-6bd3-f5ea-c21b-3e7518a04bb9@gmail.com","subject":"Making the tests ~2.5x faster (was: [PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-05T10:03:46Z","receivedAt":"2022-04-05T11:43:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 04 2022, Phillip Wood wrote:\n\n> On 04/03/2022 13:37, Elia Pinto wrote:\n>> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\n>> variables have been replaced by GLIBC_TUNABLES.  Also the new\n>> glibc requires that you preload a library called libc_malloc_debug.so\n>> to get these features.\n>> Using the ordinary glibc system variable detect if this is glibc >=\n>> 2.34 and\n>> use GLIBC_TUNABLES and the new library.\n>> This patch was inspired by a Richard W.M. Jones ndbkit patch\n>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n>> ---\n>> This is the third version of the patch.\n>> Compared to the second version[1], the code is further simplified,\n>> eliminating a case statement and modifying a string statement.\n>> [1] https://www.spinics.net/lists/git/msg433917.html\n>>   t/test-lib.sh | 16 ++++++++++++++++\n>>   1 file changed, 16 insertions(+)\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 9af5fb7674..4d10646015 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -550,9 +550,25 @@ else\n>>   \tsetup_malloc_check () {\n>>   \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n>>   \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n>> +\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n>> +\t\t_GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n>> +\t\texpr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n>> +\t\tthen\n>> +\t\t\tg=\n>> +\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n>\n> When compiling with \"SANITIZE = address,leak\" this use of LD_PRELOAD\n> makes the tests fail with\n>\n> ==9750==ASan runtime does not come first in initial library list; you\n> should either link runtime to your application or manually preload it \n> with LD_PRELOAD.\n>\n> because libc_malloc_debug.so is being loaded before libasan.so. If I\n> set TEST_NO_MALLOC_CHECK=1 when I run the tests then ASAN does not\n> complain but it would be nicer if I did not have to do that. I'm\n> confused as to why the CI leak tests are running fine - am I missing\n> something with my setup?\n\nPerhaps they have an older glibc? They're on Ubunt, and e.g. my Debian\nversion is on 2.33.\n\nBut more generally, I'd somehow managed to not notice for all my time in\nhacking on git (including on SANITIZE=leak, another tracing mode!) that\nthis check was being enabled *by default*, which could have saved me\nsome time waiting for tests...:\n\t\n\t$ git hyperfine -L rev HEAD~0 -L off yes, -s 'make CFLAGS=-O3' '(cd t && TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh)' --warmup 1 -r 3\n\tBenchmark 1: (cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0\n\t  Time (mean ± σ):      4.191 s ±  0.012 s    [User: 3.600 s, System: 0.746 s]\n\t  Range (min … max):    4.181 s …  4.204 s    3 runs\n\t \n\tBenchmark 2: (cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0\n\t  Time (mean ± σ):      5.945 s ±  0.101 s    [User: 4.989 s, System: 1.146 s]\n\t  Range (min … max):    5.878 s …  6.062 s    3 runs\n\t \n\tSummary\n\t  '(cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0' ran\n\t    1.42 ± 0.02 times faster than '(cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0'\n\nI.e. I get that it's catching actual issues, but I was also doing runs\nwith SANITIZE=address, which I believe are going to catch a superset of\nissues that this check does, so...\n\nWhatever we do with this narrow patch it would be a really nice\nimprovement if the test-lib.sh could fold all of these\n\"instrumentations\" behind a single flag, and that both it and \"make\ntest\" would make it clear that you're testing in a slower \"tracing\" or\n\"instrumentation\" mode.\n\nDitto things like chain lint and the bin-wrappers, e.g.:\n\n    $ git hyperfine -L rev HEAD~0 -L off yes, -L cl 0,1 -L nbw --no-bin-wrappers, -s 'make CFLAGS=-O3' '(cd t && GIT_TEST_CHAIN_LINT={cl} TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh {nbw})' -r 1\n    [...]\t\n\tSummary\n\t  '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0' ran\n\t    1.23 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n\t    1.30 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n\t    1.54 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n\t    1.63 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n\t    1.87 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n\t    1.92 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n\t    2.24 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n\nI.e. between this, chain lint and bin wrappers we're coming up on our\ntests running almost 3x as slow as they otherwise could *by default*.\n\nBut right now knowing which things you need to chase around to turn off\nif you're just looking to test the semantics of your code without all\nthis instrumentation is a matter of archane knowledge, I'm not even sure\nI remembered all the major ones (I didn't know about this one until\ntoday).\n"},{"id":"453132","messageId":"57c85e88-93af-acbe-f1ee-22c28dbec602@gmail.com","threadId":"57510","inReplyTo":"220405.86k0c3lt2l.gmgdl@evledraar.gmail.com","subject":"Re: Making the tests ~2.5x faster (was: [PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-04-05T13:36:33Z","receivedAt":"2022-04-05T21:50:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 05/04/2022 11:03, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Apr 04 2022, Phillip Wood wrote:\n> \n>> On 04/03/2022 13:37, Elia Pinto wrote:\n>>> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\n>>> variables have been replaced by GLIBC_TUNABLES.  Also the new\n>>> glibc requires that you preload a library called libc_malloc_debug.so\n>>> to get these features.\n>>> Using the ordinary glibc system variable detect if this is glibc >=\n>>> 2.34 and\n>>> use GLIBC_TUNABLES and the new library.\n>>> This patch was inspired by a Richard W.M. Jones ndbkit patch\n>>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n>>> ---\n>>> This is the third version of the patch.\n>>> Compared to the second version[1], the code is further simplified,\n>>> eliminating a case statement and modifying a string statement.\n>>> [1] https://www.spinics.net/lists/git/msg433917.html\n>>>    t/test-lib.sh | 16 ++++++++++++++++\n>>>    1 file changed, 16 insertions(+)\n>>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>>> index 9af5fb7674..4d10646015 100644\n>>> --- a/t/test-lib.sh\n>>> +++ b/t/test-lib.sh\n>>> @@ -550,9 +550,25 @@ else\n>>>    \tsetup_malloc_check () {\n>>>    \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n>>>    \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n>>> +\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n>>> +\t\t_GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n>>> +\t\texpr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n>>> +\t\tthen\n>>> +\t\t\tg=\n>>> +\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n>>\n>> When compiling with \"SANITIZE = address,leak\" this use of LD_PRELOAD\n>> makes the tests fail with\n>>\n>> ==9750==ASan runtime does not come first in initial library list; you\n>> should either link runtime to your application or manually preload it\n>> with LD_PRELOAD.\n>>\n>> because libc_malloc_debug.so is being loaded before libasan.so. If I\n>> set TEST_NO_MALLOC_CHECK=1 when I run the tests then ASAN does not\n>> complain but it would be nicer if I did not have to do that. I'm\n>> confused as to why the CI leak tests are running fine - am I missing\n>> something with my setup?\n> \n> Perhaps they have an older glibc? They're on Ubunt, and e.g. my Debian\n> version is on 2.33.\n\nGood point, I'd not realized quite how new glibc 2.34 was\n\n> But more generally, I'd somehow managed to not notice for all my time in\n> hacking on git (including on SANITIZE=leak, another tracing mode!) that\n> this check was being enabled *by default*, which could have saved me\n> some time waiting for tests...:\n> \t\n> \t$ git hyperfine -L rev HEAD~0 -L off yes, -s 'make CFLAGS=-O3' '(cd t && TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh)' --warmup 1 -r 3\n> \tBenchmark 1: (cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0\n> \t  Time (mean ± σ):      4.191 s ±  0.012 s    [User: 3.600 s, System: 0.746 s]\n> \t  Range (min … max):    4.181 s …  4.204 s    3 runs\n> \t\n> \tBenchmark 2: (cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0\n> \t  Time (mean ± σ):      5.945 s ±  0.101 s    [User: 4.989 s, System: 1.146 s]\n> \t  Range (min … max):    5.878 s …  6.062 s    3 runs\n> \t\n> \tSummary\n> \t  '(cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0' ran\n> \t    1.42 ± 0.02 times faster than '(cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0'\n> \n> I.e. I get that it's catching actual issues, but I was also doing runs\n> with SANITIZE=address, which I believe are going to catch a superset of\n> issues that this check does, so...\n\nI assumed SANITIZE=address would catch a superset of issues as well but \nI haven't actually checked the glibc tunables documentation. We disable \nMALLOC_PERTURB_ when running under valgrind so perhaps we should do the \nsame when compiling with SANITIZE=address.\n\nI just noticed that setup_malloc_check() is called by \ntest_expect_success() and test_when_finished() so it really should be \ncaching the result of the check rather than forking getconf and expr \neach time it is called. Overwriting LD_PRELOAD is not very friendly \neither, it would be better if it appended the debug library if the \nvariable is already set.\n\n> Whatever we do with this narrow patch it would be a really nice\n> improvement if the test-lib.sh could fold all of these\n> \"instrumentations\" behind a single flag, and that both it and \"make\n> test\" would make it clear that you're testing in a slower \"tracing\" or\n> \"instrumentation\" mode.\n> \n> Ditto things like chain lint and the bin-wrappers, e.g.:\n\nI sometimes wish there was a way to only chain lint the tests that have \nchanged since the last run.\n\n>      $ git hyperfine -L rev HEAD~0 -L off yes, -L cl 0,1 -L nbw --no-bin-wrappers, -s 'make CFLAGS=-O3' '(cd t && GIT_TEST_CHAIN_LINT={cl} TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh {nbw})' -r 1\n>      [...]\t\n> \tSummary\n> \t  '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0' ran\n> \t    1.23 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n> \t    1.30 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n> \t    1.54 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n> \t    1.63 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n> \t    1.87 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n> \t    1.92 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n> \t    2.24 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n> \n> I.e. between this, chain lint and bin wrappers we're coming up on our\n> tests running almost 3x as slow as they otherwise could *by default*.\n> \n> But right now knowing which things you need to chase around to turn off\n> if you're just looking to test the semantics of your code without all\n> this instrumentation is a matter of archane knowledge, I'm not even sure\n> I remembered all the major ones (I didn't know about this one until\n> today).\n\nThat is quite a difference in run time - I wonder how much scope there \nis for optimizing some of these features like the chain-lint vs \ndisabling them completely.\n\nBest Wishes\n\nPhillip\n"},{"id":"453141","messageId":"220405.86ilrnfgb1.gmgdl@evledraar.gmail.com","threadId":"57510","inReplyTo":"57c85e88-93af-acbe-f1ee-22c28dbec602@gmail.com","subject":"Re: Making the tests ~2.5x faster (was: [PATCH v3] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-05T19:59:53Z","receivedAt":"2022-04-05T21:50:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 05 2022, Phillip Wood wrote:\n\n> On 05/04/2022 11:03, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Apr 04 2022, Phillip Wood wrote:\n>> \n>>> On 04/03/2022 13:37, Elia Pinto wrote:\n>>>> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment\n>>>> variables have been replaced by GLIBC_TUNABLES.  Also the new\n>>>> glibc requires that you preload a library called libc_malloc_debug.so\n>>>> to get these features.\n>>>> Using the ordinary glibc system variable detect if this is glibc >=\n>>>> 2.34 and\n>>>> use GLIBC_TUNABLES and the new library.\n>>>> This patch was inspired by a Richard W.M. Jones ndbkit patch\n>>>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>>>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n>>>> ---\n>>>> This is the third version of the patch.\n>>>> Compared to the second version[1], the code is further simplified,\n>>>> eliminating a case statement and modifying a string statement.\n>>>> [1] https://www.spinics.net/lists/git/msg433917.html\n>>>>    t/test-lib.sh | 16 ++++++++++++++++\n>>>>    1 file changed, 16 insertions(+)\n>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>>>> index 9af5fb7674..4d10646015 100644\n>>>> --- a/t/test-lib.sh\n>>>> +++ b/t/test-lib.sh\n>>>> @@ -550,9 +550,25 @@ else\n>>>>    \tsetup_malloc_check () {\n>>>>    \t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n>>>>    \t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n>>>> +\t\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n>>>> +\t\t_GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n>>>> +\t\texpr 2.34 \\<= \"$_GLIBC_VERSION\" >/dev/null\n>>>> +\t\tthen\n>>>> +\t\t\tg=\n>>>> +\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\"\n>>>\n>>> When compiling with \"SANITIZE = address,leak\" this use of LD_PRELOAD\n>>> makes the tests fail with\n>>>\n>>> ==9750==ASan runtime does not come first in initial library list; you\n>>> should either link runtime to your application or manually preload it\n>>> with LD_PRELOAD.\n>>>\n>>> because libc_malloc_debug.so is being loaded before libasan.so. If I\n>>> set TEST_NO_MALLOC_CHECK=1 when I run the tests then ASAN does not\n>>> complain but it would be nicer if I did not have to do that. I'm\n>>> confused as to why the CI leak tests are running fine - am I missing\n>>> something with my setup?\n>> Perhaps they have an older glibc? They're on Ubunt, and e.g. my\n>> Debian\n>> version is on 2.33.\n>\n> Good point, I'd not realized quite how new glibc 2.34 was\n>\n>> But more generally, I'd somehow managed to not notice for all my time in\n>> hacking on git (including on SANITIZE=leak, another tracing mode!) that\n>> this check was being enabled *by default*, which could have saved me\n>> some time waiting for tests...:\n>> \t\n>> \t$ git hyperfine -L rev HEAD~0 -L off yes, -s 'make CFLAGS=-O3' '(cd t && TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh)' --warmup 1 -r 3\n>> \tBenchmark 1: (cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0\n>> \t  Time (mean ± σ):      4.191 s ±  0.012 s    [User: 3.600 s, System: 0.746 s]\n>> \t  Range (min … max):    4.181 s …  4.204 s    3 runs\n>> \t\n>> \tBenchmark 2: (cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0\n>> \t  Time (mean ± σ):      5.945 s ±  0.101 s    [User: 4.989 s, System: 1.146 s]\n>> \t  Range (min … max):    5.878 s …  6.062 s    3 runs\n>> \t\n>> \tSummary\n>> \t  '(cd t && TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh)' in 'HEAD~0' ran\n>> \t    1.42 ± 0.02 times faster than '(cd t && TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh)' in 'HEAD~0'\n>> I.e. I get that it's catching actual issues, but I was also doing\n>> runs\n>> with SANITIZE=address, which I believe are going to catch a superset of\n>> issues that this check does, so...\n>\n> I assumed SANITIZE=address would catch a superset of issues as well\n> but I haven't actually checked the glibc tunables documentation. We\n> disable MALLOC_PERTURB_ when running under valgrind so perhaps we\n> should do the same when compiling with SANITIZE=address.\n\nI'm not sure either, but given how exhaustive SANITIZE=address is I'd be\nsurprised if not.\n\n> I just noticed that setup_malloc_check() is called by\n> test_expect_success() and test_when_finished() so it really should be \n> caching the result of the check rather than forking getconf and expr\n> each time it is called. Overwriting LD_PRELOAD is not very friendly \n> either, it would be better if it appended the debug library if the\n> variable is already set.\n\nWe really should just be checking this when building, we even have C\ncode already that detects glibc and its version, I have some local (but\nsemi-unrelated) patches. Anyway...\n\n>> Whatever we do with this narrow patch it would be a really nice\n>> improvement if the test-lib.sh could fold all of these\n>> \"instrumentations\" behind a single flag, and that both it and \"make\n>> test\" would make it clear that you're testing in a slower \"tracing\" or\n>> \"instrumentation\" mode.\n>> Ditto things like chain lint and the bin-wrappers, e.g.:\n>\n> I sometimes wish there was a way to only chain lint the tests that\n> have changed since the last run.\n\nMm, perhaps some make-based solution... :)\n\nI had some experiments to go even further, and have \"make test\" only run\nthe tests relevant to the code that just changed, which with trace2's\nfilenames and GCC/clang's -MF option you can bridge that gap.\n\n>>      $ git hyperfine -L rev HEAD~0 -L off yes, -L cl 0,1 -L nbw --no-bin-wrappers, -s 'make CFLAGS=-O3' '(cd t && GIT_TEST_CHAIN_LINT={cl} TEST_NO_MALLOC_CHECK={off} ./t3070-wildmatch.sh {nbw})' -r 1\n>>      [...]\t\n>> \tSummary\n>> \t  '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0' ran\n>> \t    1.23 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n>> \t    1.30 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n>> \t    1.54 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK=yes ./t3070-wildmatch.sh )' in 'HEAD~0'\n>> \t    1.63 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n>> \t    1.87 times faster than '(cd t && GIT_TEST_CHAIN_LINT=0 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n>> \t    1.92 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh --no-bin-wrappers)' in 'HEAD~0'\n>> \t    2.24 times faster than '(cd t && GIT_TEST_CHAIN_LINT=1 TEST_NO_MALLOC_CHECK= ./t3070-wildmatch.sh )' in 'HEAD~0'\n>> I.e. between this, chain lint and bin wrappers we're coming up on\n>> our\n>> tests running almost 3x as slow as they otherwise could *by default*.\n>> But right now knowing which things you need to chase around to turn\n>> off\n>> if you're just looking to test the semantics of your code without all\n>> this instrumentation is a matter of archane knowledge, I'm not even sure\n>> I remembered all the major ones (I didn't know about this one until\n>> today).\n>\n> That is quite a difference in run time - I wonder how much scope there\n> is for optimizing some of these features like the chain-lint vs \n> disabling them completely.\n\nPer my series at\nhttps://lore.kernel.org/git/cover-v2-00.25-00000000000-20220325T182534Z-avarab@gmail.com/\nI'd much rather see us go in the direction of mainly piggy-backing on CI\nfor such extended testing, and just having easy to use targets for \"do\nexhaustive tests please\".\n\nE.g. so you could run \"make test-like-ci\" or whatever, and it would do\nall the N permutations we do in CI locally.\n\n\n\n"}]}