threads / patch / 57485

patchtest-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34

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

## tl;dr

4 messages between Feb 28, 2022 and Mar 1, 2022. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Elia Pinto· Feb 28, 2022, 16:08 UTC · lore

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(-)

Show changes to t/test-lib.sh +18 −3

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

Junio C Hamano· Feb 28, 2022, 19:13 UTC · re: Elia Pinto · lore

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

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.

Show 6 quoted lines

> +	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.

Show 10 quoted lines

>  	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?

Show 8 quoted lines

>  	}
>  	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

brian m. carlson· Mar 1, 2022, 01:25 UTC · re: Junio C Hamano · lore

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

On 2022-02-28 at 19:13:40, Junio C Hamano wrote:

Show 8 quoted lines

> 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.

Show 12 quoted lines

> > +	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

Junio C Hamano· Mar 1, 2022, 02:27 UTC · re: brian m. carlson · lore

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

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

Show 17 quoted lines

>> > +	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