[PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

[PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

[Elia Pinto]

In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment
variables have been replaced by GLIBC_TUNABLES.  Also the new
glibc requires that you preload a library called libc_malloc_debug.so
to get these features.

Using the ordinary glibc system variable detect if this is glibc >= 2.34 and
use GLIBC_TUNABLES and the new library.

This patch was inspired by a Richard W.M. Jones ndbkit patch

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/test-lib.sh | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e4716b0b86..136614ac8c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -517,12 +517,27 @@ then
 		: nothing
 	}
 else
+	if type -p getconf >/dev/null 2>&1; then
+		_GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')"
+		if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then
+			_HAVE_GLIBC_234="yes"
+		fi
+	fi
 	setup_malloc_check () {
-		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
-		export MALLOC_CHECK_ MALLOC_PERTURB_
+			if test "x$_HAVE_GLIBC_234" = xyes ; then
+				LD_PRELOAD="libc_malloc_debug.so.0" GLIBC_TUNABLES="glibc.malloc.check=1:glibc.malloc.perturb=165"
+				export LD_PRELOAD GLIBC_TUNABLES
+			else
+				MALLOC_CHECK_=3	MALLOC_PERTURB_=165
+				export MALLOC_CHECK_ MALLOC_PERTURB_
+			fi
 	}
 	teardown_malloc_check () {
-		unset MALLOC_CHECK_ MALLOC_PERTURB_
+			if test "x$_HAVE_GLIBC_234" = xyes ; then
+				unset LD_PRELOAD GLIBC_TUNABLES
+			else
+				unset MALLOC_CHECK_ MALLOC_PERTURB_
+			fi
 	}
 fi
 
-- 
2.35.1

Re: [PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment
> variables have been replaced by GLIBC_TUNABLES.  Also the new

Does it hurt to have these older environment variables?  If not,
we would prefer to see redundant but less deeply indented code,
I would imagine.

> +	if type -p getconf >/dev/null 2>&1; then
> +		_GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')"
> +		if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then
> +			_HAVE_GLIBC_234="yes"
> +		fi
> +	fi

Style.  We prefer "test ..." over "[ ... ]" and more importantly we
don't use "test X -a Y".

Do we absolutely need "test -p getconf" with an extra indentation?
I suspect we don't.

	if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
	   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
	   test 2.34 \<= "$_GLIBC_VERSION"
	then
		USE_GLIBC_TUNABLES=YesPlease
	fi

perhaps?  I am not sure if glibc 2.4 still matters, getconf reports
it as 2.04 or 2.4, for the above comparison to be OK, though.

In any case, HAVE_GLIBC_234 is a horrible variable name to use for
this purpose, as the primary thing the two use sites care about is
not the version but if they should use the GLIBC_TUNABLES mechanism,
so it would be better to name the variable after the feature.

>  	setup_malloc_check () {
> -		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
> -		export MALLOC_CHECK_ MALLOC_PERTURB_
> +			if test "x$_HAVE_GLIBC_234" = xyes ; then
> +				LD_PRELOAD="libc_malloc_debug.so.0" GLIBC_TUNABLES="glibc.malloc.check=1:glibc.malloc.perturb=165"
> +				export LD_PRELOAD GLIBC_TUNABLES
> +			else
> +				MALLOC_CHECK_=3	MALLOC_PERTURB_=165
> +				export MALLOC_CHECK_ MALLOC_PERTURB_
> +			fi

Avoid overly long lines when you can easily do so.

		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
		export MALLOC_CHECK_ MALLOC_PERTURB_
+		case "$USE_GLIBC_TUNABLES" in
+		YesPlease)
+			g=
+			LD_PRELOAD=libc_malloc_debug.so.0
+			for t in \
+				glibc.malloc.check=1 \
+				glibc.malloc.perturb=165 \
+			do
+				g="$g${g:+:}$t"
+			done
+			GLIBC_TUNABLES=$g
+			;;
+		esac

perhaps?

>  	}
>  	teardown_malloc_check () {
> -		unset MALLOC_CHECK_ MALLOC_PERTURB_
> +			if test "x$_HAVE_GLIBC_234" = xyes ; then
> +				unset LD_PRELOAD GLIBC_TUNABLES
> +			else
> +				unset MALLOC_CHECK_ MALLOC_PERTURB_
> +			fi

Similarly.

>  	}
>  fi

Re: [PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On 2022-02-28 at 19:13:40, Junio C Hamano wrote:
> Elia Pinto <gitter.spiros@gmail.com> writes:
> 
> > In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment
> > variables have been replaced by GLIBC_TUNABLES.  Also the new
> 
> Does it hurt to have these older environment variables?  If not,
> we would prefer to see redundant but less deeply indented code,
> I would imagine.

Setting both sets of environment variables is probably just fine and the
simplest solution, I'd imagine.

> > +	if type -p getconf >/dev/null 2>&1; then
> > +		_GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')"
> > +		if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then
> > +			_HAVE_GLIBC_234="yes"
> > +		fi
> > +	fi
> 
> Style.  We prefer "test ..." over "[ ... ]" and more importantly we
> don't use "test X -a Y".
> 
> Do we absolutely need "test -p getconf" with an extra indentation?
> I suspect we don't.

getconf is specified by POSIX, but that doesn't mean it's in the default
install or in PATH on all systems.  However, we should write "command -v
getconf" instead if we need to check, since that's the POSIX way to
write it, and "type" is not guaranteed to be present on all systems.

It looks like the code might silently not match if getconf isn't
present, and if so then we can avoid the check, but we should of course
put a comment noting that behavior to help future readers.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> > +	if type -p getconf >/dev/null 2>&1; then
>> > +		_GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')"
>> > +		if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then
>> > +			_HAVE_GLIBC_234="yes"
>> > +		fi
>> > +	fi
>> 
>> Style.  We prefer "test ..." over "[ ... ]" and more importantly we
>> don't use "test X -a Y".
>> 
>> Do we absolutely need "test -p getconf" with an extra indentation?
>> I suspect we don't.
>
> getconf is specified by POSIX, but that doesn't mean it's in the default
> install or in PATH on all systems.  However, we should write "command -v
> getconf" instead if we need to check, since that's the POSIX way to
> write it, and "type" is not guaranteed to be present on all systems.

My point was that the code relies on having getconf on PATH anyway,
so it is sufficient to attempt running getconf and using its output
after checking it begins with "glibc".  Missing getconf or getconf
that is different from what we expect it to be would be rejected by
the same code, without needing the above nested if .. if .. fi .. fi