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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2022, 22:12 UTC
Message-ID: <xmqq8rtq8z41.fsf@gitster.g>
In-Reply-To: <20220303090640.190307-1-gitter.spiros@gmail.com>

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

> Compared to the first version, the code has been simplified, based on Junio's
> indications, introducing some redundancy in the setting of the glibc variables
> covered by the patch

I think we can even lose the separate case statement and write its body inside if/then...fi. The following shows the result of shuffling the lines to do so.

>  	setup_malloc_check () {
> +		if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) &&
> +		   _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} &&
> +		   expr 2.34 \<= "$_GLIBC_VERSION" >/dev/null

I am wondering if we want to make sure "getconf" returned a string that begins with "glibc " before stripping that prefix. Hopefully such a paranoia is probably unneeded.

Show 8 quoted lines

> +		then
> +			g=
> +			LD_PRELOAD="libc_malloc_debug.so.0"
> +			for t in \
> +				glibc.malloc.check=1 \
> +				glibc.malloc.perturb=165
> +			do
> +				g="${g##:}:$t"

Cute. As this strips a constant, we do not need to be ##greedy and using just a single # would suffice, right?

Show 13 quoted lines

> +			done
> +			GLIBC_TUNABLES=$g
> +			export LD_PRELOAD GLIBC_TUNABLES
> +			;;
> +		fi
>  		MALLOC_CHECK_=3	MALLOC_PERTURB_=165
>  		export MALLOC_CHECK_ MALLOC_PERTURB_
>  	}
>  	teardown_malloc_check () {
>  		unset MALLOC_CHECK_ MALLOC_PERTURB_
> +		unset LD_PRELOAD GLIBC_TUNABLES
>  	}
>  fi

especially the teardown side unsets these unconditionally.