Re: [PATCH] Add valgrind smoke test

[Mark Wielaard <mark@klomp.org>]

Hi,

On Mon, 2021-12-06 at 15:40 +0100, Alexandra Hájková wrote:
> 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>

Let me add some comments on the different parts to help a real review.

> ---
>  elf/Makefile              |  7 +++++++
>  elf/tst-valgrind-smoke.sh | 38 ++++++++++++++++++++++++++++++++++++++
>  elf/valgrind-test.c       | 31 +++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+)
>  create mode 100644 elf/tst-valgrind-smoke.sh
>  create mode 100644 elf/valgrind-test.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index ef36008673..14aab3624a 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -232,6 +232,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-dl-is_dso tst-ro-dynamic \
>  	 tst-audit18 \
>  	 tst-rtld-run-static \
> +	 valgrind-test
>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \

Adding valgrind-test to tests simply makes sure it gets build for use
with tst-valgrind-smoke.sh.

> @@ -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)
> +
>  tests += $(tests-execstack-$(have-z-execstack))
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-leaks1-mem.out \

We want a tst-valgrind-smoke.out file, which will show the test run.
To get tst-valgrind-smoke.out we need to have tst-valgrind-smoke.sh
(which already exists, but we'll also use it as $< in the make rule)
and ld.so build first. Question, do we also want to depend on
$(objpfx)valgrind-tst to make sure that it exists when this rule is
executed, or is having it in tests enough to express that dependency?

The rule simply passes the just build linker, test and run-program
environments and the rpath of the just build libraries, plus the actual
program to run under valgrind to the tst-valgrind-smoke.sh script. The
exit code of which will then be processed by evaluate-test.

> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> new file mode 100644
> index 0000000000..a78d7ff10d
> --- /dev/null
> +++ b/elf/tst-valgrind-smoke.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +# 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/>.
> +
> +set -e
> +
> +rtld=$1
> +test_wrapper_env=$2
> +run_program_env=$3
> +library_path=$4
> +test_prog=$5
> +
> +# 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

First we make sure that valgrind is actually available in the test
environment, if not, like in a cross-build, it will immediately exit
with 77 which produces a SKIP.

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

Then we execute valgrind on the test program (valgrind-test) inside the
test environment using the just build ld.so and libraries. If valgrind
detects any error it exits with exit code 1 (producing a FAIL),
otherwise it will use the exit code (0) of the test_prog.

> diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
> new file mode 100644
> index 0000000000..606c874b68
> --- /dev/null
> +++ b/elf/valgrind-test.c
> @@ -0,0 +1,31 @@
> +/* This is the simple test intended to be called by
> +   tst-valgrind-smoke to perform vagrind 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/>.  */
> +
> +#include <libintl.h>
> +#include <locale.h>
> +
> +int
> +main (void)
> +{
> +    setlocale (LC_ALL, "");
> +    bindtextdomain ("translit", "");
> +    textdomain ("translit");
> +
> +  return 0;
> +}

The program to be run under valgrind in the test environment is fairly
simply, but still exercises some constructs (like string and path
lookups and dlopen) that have seen issues under valgrind in the past.

I hope this valgrind smoke test will be accepted into the glibc
testsuite because it would really help us catch issues with the
valgrind/glibc interactions early.

Cheers,

Mark