Re: [PATCH] Add valgrind smoke test

[DJ Delorie <dj@redhat.com>]

Alexandra Hjkov via Libc-alpha <libc-alpha@sourceware.org>
writes:
> From: Alexandra Hájková <ahajkova@redhat.com>
>
> Check if valgrind is present during the configure time and
> run smoke tests with valgrind to verify dynamic loader.
>
> Co-authored-by: Mark Wielaard <mark@klomp.org>

Legalities are OK.

> --- a/elf/Makefile
> +	 valgrind-test

Standard list of tests; valgrind-test.c is the "target" despite wrapping
it in the script.

> @@ -253,6 +254,12 @@ tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \
>  endif
>  endif
>  endif
> +
> +tests-special += $(objpfx)tst-valgrind-smoke.out
> +$(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so
> +	$(SHELL) $< $(objpfx)ld.so '$(test-wrapper-env)' '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; \
> +	$(evaluate-test)
> +

This is our usual way of running a test, ok.

> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> +#!/bin/sh

Note we ignore this as we invoke it via $(SHELL) but ok

> +# Valgrind smoke test.
> +# Copyright (C) 2021 Free Software Foundation, Inc.
> +# This file is part of the GNU C Library.
> +
> +# The GNU C Library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +
> +# The GNU C Library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with the GNU C Library; if not, see
> +# <https://www.gnu.org/licenses/>.

Ok.

> +set -e
> +
> +rtld=$1
> +test_wrapper_env=$2
> +run_program_env=$3
> +library_path=$4
> +test_prog=$5

The Makefile took pains to wrap these in quotes; they should be
similarly quoted here, although in reality the shell does the right
thing.  Not a blocker, just my parnoia ;-)

> +# Test whether valgrind is available in the test
> +# environment. If not, skip the test.
> +${test_wrapper_env} \
> +${run_program_env} \
> +$rtld --library-path "$library_path" \
> +  /bin/sh -c 'command -v valgrind' || exit 77

Tested on bash, sh, and ksh - although 1003.2-1992 doesn't list
"command" as a built-in.  Not a problem; we don't support systems that
old ;-)

We run this script via $(SHELL), we should use $(SHELL) here too, not /bin/sh

> +${test_wrapper_env} \
> +${run_program_env} \
> +$rtld --library-path "$library_path" \
> +/bin/sh -c "valgrind -q --error-exitcode=1 $test_prog"

Same $(SHELL) here.

A bit of inconsistency with $x vs ${x} vs "$x" but that doesn't affect
functionality here.  However, it looks like we're running /bin/sh with
the just-built glibc, and the test program with the system's glibc?
If you look in $(builddir)/testrun.sh you'll see how we run valgrind to
test the built glibc.

I.e. (in short):

${...} /bin/sh valgrind foo  <- runs /bin/sh with new glibc
/bin/sh ${...} valgrind foo  <- runs valgrind with new glibc
/bin/sh valgrind ${...} foo  <- runs foo with new glibc

Also, if it's your *intent* to test the system valgrind against the
just-built glibc[*], that needs a comment, and should be tested in a
cross-compile environment.

(in general, testing in a cross-compiled case is an excellent way to see
if you got the rtld stuff in the right place ;-)

[*] or both valgrind and the test binary

> diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
> +int
> +main (void)
> +{
> +    setlocale (LC_ALL, "");
> +    bindtextdomain ("translit", "");
> +    textdomain ("translit");
> +
> +  return 0;
> +}

Any reason why these particular calls were chosen, rather than (for
example) a malloc/free pair?  Perhaps a bit of commentary for future
readers about what valgrind is expected to be testing here...