Add valgrind smoke test

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

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
  

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

* DJ Delorie via Libc-alpha:

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

Is it a requirement that $(SHELL) exists in the target environment?  Not
sure about that.  Our containers only have /bin/sh, for example.

Thanks,
Florian
  

Right, test_wrapper might ssh to a different machine.

Even then, a subtle difference between the host and build ABI could
cause problems (and has, in the past, which is why our test-container
rules are so complex), so we may not be able to run $(SHELL) or /bin/sh
via ld.so, even for a "native" build.
  

Hi DJ,

On Tue, 2021-12-07 at 15:32 -0500, DJ Delorie wrote:
> > +# 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

Can we rely on SHELL being defined here or should we pass that to the
test script too?

> > +${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.
> [*] or both valgrind and the test binary

The intent is to test the valgrind available in the test environment
against the just-built glibc (that is both valgrind and the test binary
valgrind-test, which are really the same process)

So we want option 2 from your list. What exactly is ${...} in this
case?

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

I never did a cross-compiled case. Is there a description of that
process?

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

See also my other comment in 
https://sourceware.org/pipermail/libc-alpha/2021-December/133714.html

This tests not just a simple malloc/free pair, but also various
string/path comparisons, dlopen, etc. which in the past have seen
issues with valgrind.

Thanks,

Mark
  

* Mark Wielaard:

> On Tue, 2021-12-07 at 15:32 -0500, DJ Delorie wrote:
>> > +# 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
>
> Can we rely on SHELL being defined here or should we pass that to the
> test script too?

SHELL isn't right for the test.  It runs in a different environment.
Hard-coding /bin/sh is the right thing to do (like we do in the system
function, so it must already exist for many tests to work).

Thanks,
Florian
  

Mark Wielaard <mark@klomp.org> writes:
>> 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
>
> The intent is to test the valgrind available in the test environment
> against the just-built glibc (that is both valgrind and the test binary
> valgrind-test, which are really the same process)

More like a mix:

/bin/sh ${...} valgrind ${...} foo

The ${...} is all the variables needed to run in the just-built
environment, so ${test_wrapper_env} ${run_program_env} $rtld
--library-path "$library_path"

You need to include all that muck for each binary you want to run in the
just-built environment; it's not inherited.

Although you likely don't want the test_wrapper_env for the "foo" as it
might include a nested ssh, it's complicated and requires a bit of trial
and error.  You definitely need the rest else the test program will use
the target's system libc.

>> (in general, testing in a cross-compiled case is an excellent way to
>> see
>> if you got the rtld stuff in the right place ;-)
>
> I never did a cross-compiled case. Is there a description of that
> process?

You can use build-many-glibcs to do a cross build but I don't know if
that supports running the testsuite fully.

Typically you'd have a remote machine you can ssh to, which nfs mounts
your build to the same path, then set a variable to wrap all the tests
in a script that ssh's.

https://sourceware.org/glibc/wiki/Testing/Testsuite#Testing_with_a_cross-compiler

Look at glibc's scripts/cross-test-ssh.sh for instructions

> See also my other comment in 
> https://sourceware.org/pipermail/libc-alpha/2021-December/133714.html
>
> This tests not just a simple malloc/free pair, but also various
> string/path comparisons, dlopen, etc. which in the past have seen
> issues with valgrind.

Ok.
  

On Fri, 2021-12-10 at 14:15 -0500, DJ Delorie wrote:
> Mark Wielaard <mark@klomp.org> writes:
> > > 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
> > 
> > The intent is to test the valgrind available in the test environment
> > against the just-built glibc (that is both valgrind and the test binary
> > valgrind-test, which are really the same process)
> 
> More like a mix:
> 
> /bin/sh ${...} valgrind ${...} foo
> 
> The ${...} is all the variables needed to run in the just-built
> environment, so ${test_wrapper_env} ${run_program_env} $rtld
> --library-path "$library_path"
> 
> You need to include all that muck for each binary you want to run in the
> just-built environment; it's not inherited.
> 
> Although you likely don't want the test_wrapper_env for the "foo" as it
> might include a nested ssh, it's complicated and requires a bit of trial
> and error.  You definitely need the rest else the test program will use
> the target's system libc.

OK. So we would like to invoke the shell /bin/sh as is, so that is just
the system shell? Then we want to use that to invoke valgrind in the
test environment (with or without the new ld.so which doesn't really
matter because...) we then invoke valgrind on the newly build ld.so
with the new library_path, which then invokes our test binary.

One question is why /bin/sh itself doesn't need to run inside/using the
test environment. Doesn't it make things easier if the /bin/sh itself
is executing inside the test environment or does that complicate
things?

But either way should work, although running valgrind on ld.so directly
isn't really supported on some arches (e.g. ppc64 because of some odd
relocation issues). But if we can figure out what the system ld.so is
then we can use that in the first invocation to see if it works and
SKIP, exit 77 when it doesn't.

What is the correct way to extract the system ld.so?

So the tst-valgrind-smoke.sh would have the test whether valgrind is
available as is. Then an extra test to see if valgrind running on the
system ld.so actually works in the test environment. And finally the
actual test using the new ld.so and test library path.

# Test valgrind is available in the test environment
${test_wrapper_env} ${run_program_env} \
  $rtld --library-path "$library_path" \
  /bin/sh -c 'command -v valgrind' || exit 77

# Test valgrind works with the system ld.so in the test environment
# XXX how to get/set ${system_rtld}???
/bin/sh -c \
  "${test_wrapper_env} ${run_program_env} \
   valgrind -q --error-exitcode=1 \
   ${system_rtld} ${test_prog}" || exit 77

# Run the actual test under valgrind and the new ld.so
/bin/sh -c \
  "${test_wrapper_env} ${run_program_env} \
   valgrind -q --error-exitcode=1 \
   ${rtld} --library-path ${library_path} ${test_prog}"

> > > (in general, testing in a cross-compiled case is an excellent way to
> > > see if you got the rtld stuff in the right place ;-)
> > 
> > I never did a cross-compiled case. Is there a description of that
> > process?
> 
> You can use build-many-glibcs to do a cross build but I don't know if
> that supports running the testsuite fully.

Do you have an example how to invoke this script?
It starts with "This script takes as arguments a directory name
(containing a src subdirectory with sources of the relevant toolchain
components)". What are the relevant toolchain components?

> Typically you'd have a remote machine you can ssh to, which nfs mounts
> your build to the same path, then set a variable to wrap all the tests
> in a script that ssh's.
> 
> https://sourceware.org/glibc/wiki/Testing/Testsuite#Testing_with_a_cross-compiler
> 
> Look at glibc's scripts/cross-test-ssh.sh for instructions

OK, so we need multiple machines that can nfs mount each others source
and/or build dirs. Is there a setup like that in the gcc compile farm?

Thanks,

Mark