{"thread":{"id":"57485","subject":"[PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","startedAt":"2022-02-28T16:08:47Z","lastAt":"2022-03-01T02:27:18Z","messageCount":4,"participants":["Elia Pinto","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"449774","messageId":"20220228160827.465488-1-gitter.spiros@gmail.com","threadId":"57485","inReplyTo":null,"subject":"[PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-02-28T16:08:27Z","receivedAt":"2022-02-28T16:08:47Z","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\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n t/test-lib.sh | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b86..136614ac8c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -517,12 +517,27 @@ then\n \t\t: nothing\n \t}\n else\n+\tif type -p getconf >/dev/null 2>&1; then\n+\t\t_GLIBC_VERSION=\"$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')\"\n+\t\tif [ -n \"$_GLIBC_VERSION\" -a $(expr \"$_GLIBC_VERSION\" \\>= \"2.34\") ]; then\n+\t\t\t_HAVE_GLIBC_234=\"yes\"\n+\t\tfi\n+\tfi\n \tsetup_malloc_check () {\n-\t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n-\t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\t\tif test \"x$_HAVE_GLIBC_234\" = xyes ; then\n+\t\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\" GLIBC_TUNABLES=\"glibc.malloc.check=1:glibc.malloc.perturb=165\"\n+\t\t\t\texport LD_PRELOAD GLIBC_TUNABLES\n+\t\t\telse\n+\t\t\t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n+\t\t\t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\t\tfi\n \t}\n \tteardown_malloc_check () {\n-\t\tunset MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\t\tif test \"x$_HAVE_GLIBC_234\" = xyes ; then\n+\t\t\t\tunset LD_PRELOAD GLIBC_TUNABLES\n+\t\t\telse\n+\t\t\t\tunset MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\t\tfi\n \t}\n fi\n \n-- \n2.35.1\n\n"},{"id":"449801","messageId":"xmqq7d9e249n.fsf@gitster.g","threadId":"57485","inReplyTo":"20220228160827.465488-1-gitter.spiros@gmail.com","subject":"Re: [PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T19:13:40Z","receivedAt":"2022-02-28T19:13:49Z","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\nDoes it hurt to have these older environment variables?  If not,\nwe would prefer to see redundant but less deeply indented code,\nI would imagine.\n\n> +\tif type -p getconf >/dev/null 2>&1; then\n> +\t\t_GLIBC_VERSION=\"$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')\"\n> +\t\tif [ -n \"$_GLIBC_VERSION\" -a $(expr \"$_GLIBC_VERSION\" \\>= \"2.34\") ]; then\n> +\t\t\t_HAVE_GLIBC_234=\"yes\"\n> +\t\tfi\n> +\tfi\n\nStyle.  We prefer \"test ...\" over \"[ ... ]\" and more importantly we\ndon't use \"test X -a Y\".\n\nDo we absolutely need \"test -p getconf\" with an extra indentation?\nI suspect we don't.\n\n\tif _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&\n\t   _GLIBC_VERSION=${_GLIBC_VERSION#\"glibc \"} &&\n\t   test 2.34 \\<= \"$_GLIBC_VERSION\"\n\tthen\n\t\tUSE_GLIBC_TUNABLES=YesPlease\n\tfi\n\nperhaps?  I am not sure if glibc 2.4 still matters, getconf reports\nit as 2.04 or 2.4, for the above comparison to be OK, though.\n\nIn any case, HAVE_GLIBC_234 is a horrible variable name to use for\nthis purpose, as the primary thing the two use sites care about is\nnot the version but if they should use the GLIBC_TUNABLES mechanism,\nso it would be better to name the variable after the feature.\n\n>  \tsetup_malloc_check () {\n> -\t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n> -\t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n> +\t\t\tif test \"x$_HAVE_GLIBC_234\" = xyes ; then\n> +\t\t\t\tLD_PRELOAD=\"libc_malloc_debug.so.0\" GLIBC_TUNABLES=\"glibc.malloc.check=1:glibc.malloc.perturb=165\"\n> +\t\t\t\texport LD_PRELOAD GLIBC_TUNABLES\n> +\t\t\telse\n> +\t\t\t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n> +\t\t\t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n> +\t\t\tfi\n\nAvoid overly long lines when you can easily do so.\n\n\t\tMALLOC_CHECK_=3\tMALLOC_PERTURB_=165\n\t\texport MALLOC_CHECK_ MALLOC_PERTURB_\n+\t\tcase \"$USE_GLIBC_TUNABLES\" in\n+\t\tYesPlease)\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${g:+:}$t\"\n+\t\t\tdone\n+\t\t\tGLIBC_TUNABLES=$g\n+\t\t\t;;\n+\t\tesac\n\nperhaps?\n\n>  \t}\n>  \tteardown_malloc_check () {\n> -\t\tunset MALLOC_CHECK_ MALLOC_PERTURB_\n> +\t\t\tif test \"x$_HAVE_GLIBC_234\" = xyes ; then\n> +\t\t\t\tunset LD_PRELOAD GLIBC_TUNABLES\n> +\t\t\telse\n> +\t\t\t\tunset MALLOC_CHECK_ MALLOC_PERTURB_\n> +\t\t\tfi\n\nSimilarly.\n\n>  \t}\n>  fi\n"},{"id":"449840","messageId":"Yh12FNTGuJkRlqBE@camp.crustytoothpaste.net","threadId":"57485","inReplyTo":"xmqq7d9e249n.fsf@gitster.g","subject":"Re: [PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-03-01T01:25:40Z","receivedAt":"2022-03-01T01:25:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-02-28 at 19:13:40, Junio C Hamano wrote:\n> 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> \n> Does it hurt to have these older environment variables?  If not,\n> we would prefer to see redundant but less deeply indented code,\n> I would imagine.\n\nSetting both sets of environment variables is probably just fine and the\nsimplest solution, I'd imagine.\n\n> > +\tif type -p getconf >/dev/null 2>&1; then\n> > +\t\t_GLIBC_VERSION=\"$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')\"\n> > +\t\tif [ -n \"$_GLIBC_VERSION\" -a $(expr \"$_GLIBC_VERSION\" \\>= \"2.34\") ]; then\n> > +\t\t\t_HAVE_GLIBC_234=\"yes\"\n> > +\t\tfi\n> > +\tfi\n> \n> Style.  We prefer \"test ...\" over \"[ ... ]\" and more importantly we\n> don't use \"test X -a Y\".\n> \n> Do we absolutely need \"test -p getconf\" with an extra indentation?\n> I suspect we don't.\n\ngetconf is specified by POSIX, but that doesn't mean it's in the default\ninstall or in PATH on all systems.  However, we should write \"command -v\ngetconf\" instead if we need to check, since that's the POSIX way to\nwrite it, and \"type\" is not guaranteed to be present on all systems.\n\nIt looks like the code might silently not match if getconf isn't\npresent, and if so then we can avoid the check, but we should of course\nput a comment noting that behavior to help future readers.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"449843","messageId":"xmqqtuciwgot.fsf@gitster.g","threadId":"57485","inReplyTo":"Yh12FNTGuJkRlqBE@camp.crustytoothpaste.net","subject":"Re: [PATCH] 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-01T02:27:14Z","receivedAt":"2022-03-01T02:27:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>> > +\tif type -p getconf >/dev/null 2>&1; then\n>> > +\t\t_GLIBC_VERSION=\"$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')\"\n>> > +\t\tif [ -n \"$_GLIBC_VERSION\" -a $(expr \"$_GLIBC_VERSION\" \\>= \"2.34\") ]; then\n>> > +\t\t\t_HAVE_GLIBC_234=\"yes\"\n>> > +\t\tfi\n>> > +\tfi\n>> \n>> Style.  We prefer \"test ...\" over \"[ ... ]\" and more importantly we\n>> don't use \"test X -a Y\".\n>> \n>> Do we absolutely need \"test -p getconf\" with an extra indentation?\n>> I suspect we don't.\n>\n> getconf is specified by POSIX, but that doesn't mean it's in the default\n> install or in PATH on all systems.  However, we should write \"command -v\n> getconf\" instead if we need to check, since that's the POSIX way to\n> write it, and \"type\" is not guaranteed to be present on all systems.\n\nMy point was that the code relies on having getconf on PATH anyway,\nso it is sufficient to attempt running getconf and using its output\nafter checking it begins with \"glibc\".  Missing getconf or getconf\nthat is different from what we expect it to be would be rejected by\nthe same code, without needing the above nested if .. if .. fi .. fi\n"}]}