[PATCH] Add valgrind smoke test

[PATCH] Add valgrind smoke test

[Alexandra Hájková]

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.
---
 Makefile     |  4 +++
 configure    | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure.ac |  1 +
 elf/Makefile |  8 +++++
 4 files changed, 106 insertions(+)

diff --git a/Makefile b/Makefile
index 50f99ca611..a9ae15d65e 100644
--- a/Makefile
+++ b/Makefile
@@ -146,6 +146,7 @@ GCONV_PATH="$${builddir}/iconvdata"
 usage () {
   echo "usage: $$0 [--tool=strace] PROGRAM [ARGUMENTS...]" 2>&1
   echo "       $$0 --tool=valgrind PROGRAM [ARGUMENTS...]" 2>&1
+  echo "       $$0 --tool=valgrind-test PROGRAM [ARGUMENTS...]" 2>&1
   exit 1
 }
 
@@ -181,6 +182,9 @@ case "$$toolname" in
   valgrind)
     exec env $(run-program-env) valgrind $(test-via-rtld-prefix) $${1+"$$@"}
     ;;
+  valgrind-test)
+    exec env $(run-program-env) valgrind -q --error-exitcode=1 $(test-via-rtld-prefix) $${1+"$$@"}
+    ;;
   container)
     exec env $(run-program-env) $(test-via-rtld-prefix) \
       $(common-objdir)/support/test-container \
diff --git a/configure b/configure
index 5dde2ba355..769341ef05 100755
--- a/configure
+++ b/configure
@@ -690,6 +690,7 @@ sysheaders
 ac_ct_CXX
 CXXFLAGS
 CXX
+VALGRIND
 READELF
 CPP
 cross_compiling
@@ -2922,6 +2923,98 @@ else
   READELF="$ac_cv_prog_READELF"
 fi
 
+if test -n "$ac_tool_prefix"; then
+  # Extract the first word of "${ac_tool_prefix}valgrind", so it can be a program name with args.
+set dummy ${ac_tool_prefix}valgrind; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
+$as_echo_n "checking for $ac_word... " >&6; }
+if ${ac_cv_prog_VALGRIND+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  if test -n "$VALGRIND"; then
+  ac_cv_prog_VALGRIND="$VALGRIND" # Let the user override the test.
+else
+as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
+for as_dir in $PATH
+do
+  IFS=$as_save_IFS
+  test -z "$as_dir" && as_dir=.
+    for ac_exec_ext in '' $ac_executable_extensions; do
+  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
+    ac_cv_prog_VALGRIND="${ac_tool_prefix}valgrind"
+    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
+    break 2
+  fi
+done
+  done
+IFS=$as_save_IFS
+
+fi
+fi
+VALGRIND=$ac_cv_prog_VALGRIND
+if test -n "$VALGRIND"; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $VALGRIND" >&5
+$as_echo "$VALGRIND" >&6; }
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+fi
+
+
+fi
+if test -z "$ac_cv_prog_VALGRIND"; then
+  ac_ct_VALGRIND=$VALGRIND
+  # Extract the first word of "valgrind", so it can be a program name with args.
+set dummy valgrind; ac_word=$2
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
+$as_echo_n "checking for $ac_word... " >&6; }
+if ${ac_cv_prog_ac_ct_VALGRIND+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  if test -n "$ac_ct_VALGRIND"; then
+  ac_cv_prog_ac_ct_VALGRIND="$ac_ct_VALGRIND" # Let the user override the test.
+else
+as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
+for as_dir in $PATH
+do
+  IFS=$as_save_IFS
+  test -z "$as_dir" && as_dir=.
+    for ac_exec_ext in '' $ac_executable_extensions; do
+  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
+    ac_cv_prog_ac_ct_VALGRIND="valgrind"
+    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
+    break 2
+  fi
+done
+  done
+IFS=$as_save_IFS
+
+fi
+fi
+ac_ct_VALGRIND=$ac_cv_prog_ac_ct_VALGRIND
+if test -n "$ac_ct_VALGRIND"; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_ct_VALGRIND" >&5
+$as_echo "$ac_ct_VALGRIND" >&6; }
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+fi
+
+  if test "x$ac_ct_VALGRIND" = x; then
+    VALGRIND="false"
+  else
+    case $cross_compiling:$ac_tool_warned in
+yes:)
+{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: using cross tools not prefixed with host triplet" >&5
+$as_echo "$as_me: WARNING: using cross tools not prefixed with host triplet" >&2;}
+ac_tool_warned=yes ;;
+esac
+    VALGRIND=$ac_ct_VALGRIND
+  fi
+else
+  VALGRIND="$ac_cv_prog_VALGRIND"
+fi
+
 
 # We need the C++ compiler only for testing.
 ac_ext=cpp
diff --git a/configure.ac b/configure.ac
index 19051b8ee0..fef797527b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -52,6 +52,7 @@ fi
 AC_SUBST(cross_compiling)
 AC_PROG_CPP
 AC_CHECK_TOOL(READELF, readelf, false)
+AC_CHECK_TOOL([VALGRIND], [valgrind], [false])
 
 # We need the C++ compiler only for testing.
 AC_PROG_CXX
diff --git a/elf/Makefile b/elf/Makefile
index 834ec858a8..5b72cc76f9 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -49,6 +49,14 @@ ifeq (yesyes,$(build-shared)$(run-built-tests))
 tests-special += $(objpfx)list-tunables.out
 endif
 
+# Run smoke tests with valgrind to verify dynamic loader
+ifneq ($(VALGRIND),false)
+tests-special += $(objpfx)valgrind-smoke-test.out
+$(objpfx)valgrind-smoke-test.out: $(objpfx)ld.so
+	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true > $@
+	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true --help >> $@
+endif
+
 # Make sure that the compiler does not insert any library calls in tunables
 # code paths.
 ifeq (yes,$(have-loop-to-function))
-- 
2.26.3

Re: [PATCH] Add valgrind smoke test

[Carlos O'Donell]

On 5/24/21 8:15 AM, Alexandra Hájková via Libc-alpha 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.

Thanks for proposing this. We have been running similar valgrind smoke tests
in Fedora Rawhide and it is very beneficial.

> ---
>  Makefile     |  4 +++
>  configure    | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure.ac |  1 +
>  elf/Makefile |  8 +++++
>  4 files changed, 106 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 50f99ca611..a9ae15d65e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -146,6 +146,7 @@ GCONV_PATH="$${builddir}/iconvdata"
>  usage () {
>    echo "usage: $$0 [--tool=strace] PROGRAM [ARGUMENTS...]" 2>&1
>    echo "       $$0 --tool=valgrind PROGRAM [ARGUMENTS...]" 2>&1
> +  echo "       $$0 --tool=valgrind-test PROGRAM [ARGUMENTS...]" 2>&1

OK. Add valgrind-test to testrun.sh.

>    exit 1
>  }
>  
> @@ -181,6 +182,9 @@ case "$$toolname" in
>    valgrind)
>      exec env $(run-program-env) valgrind $(test-via-rtld-prefix) $${1+"$$@"}
>      ;;
> +  valgrind-test)
> +    exec env $(run-program-env) valgrind -q --error-exitcode=1 $(test-via-rtld-prefix) $${1+"$$@"}
> +    ;;

OK.

>    container)
>      exec env $(run-program-env) $(test-via-rtld-prefix) \
>        $(common-objdir)/support/test-container \
> diff --git a/configure b/configure
> index 5dde2ba355..769341ef05 100755
> --- a/configure
> +++ b/configure

OK.

> --- a/configure.ac
> +++ b/configure.ac
> @@ -52,6 +52,7 @@ fi
>  AC_SUBST(cross_compiling)
>  AC_PROG_CPP
>  AC_CHECK_TOOL(READELF, readelf, false)
> +AC_CHECK_TOOL([VALGRIND], [valgrind], [false])

OK.

>  
>  # We need the C++ compiler only for testing.
>  AC_PROG_CXX
> diff --git a/elf/Makefile b/elf/Makefile
> index 834ec858a8..5b72cc76f9 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -49,6 +49,14 @@ ifeq (yesyes,$(build-shared)$(run-built-tests))
>  tests-special += $(objpfx)list-tunables.out
>  endif
>  
> +# Run smoke tests with valgrind to verify dynamic loader
> +ifneq ($(VALGRIND),false)
> +tests-special += $(objpfx)valgrind-smoke-test.out
> +$(objpfx)valgrind-smoke-test.out: $(objpfx)ld.so
> +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true > $@
> +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true --help >> $@
> +endif
Does this work for cross-compiling?

Do you correctly detect the target valgrind and does the test wrapper allow the
correct execution of valgrind on the target system?

Please see scripts/cross-test-ssh.sh.

> +
>  # Make sure that the compiler does not insert any library calls in tunables
>  # code paths.
>  ifeq (yes,$(have-loop-to-function))
> 


-- 
Cheers,
Carlos.

Re: [PATCH] Add valgrind smoke test

[Joseph Myers]

On Mon, 24 May 2021, Carlos O'Donell via Libc-alpha wrote:

> > +# Run smoke tests with valgrind to verify dynamic loader
> > +ifneq ($(VALGRIND),false)
> > +tests-special += $(objpfx)valgrind-smoke-test.out
> > +$(objpfx)valgrind-smoke-test.out: $(objpfx)ld.so
> > +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true > $@
> > +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true --help >> $@
> > +endif
> Does this work for cross-compiling?
> 
> Do you correctly detect the target valgrind and does the test wrapper 
> allow the correct execution of valgrind on the target system?

My expectation is that <host>-valgrind won't exist - but AC_CHECK_TOOL is 
documented as falling back to the tool without a host prefix if a prefixed 
version doesn't exist, which is a problem.  Logically, this test should 
run if valgrind is available on the test system (which would need to be 
determined when the tests are run, test-wrapper isn't available when glibc 
is built), but the test-wrapper interface doesn't strictly support running 
arbitrary installed programs like that.  So you might need a test program 
(run on the test system via test-wrapper) that itself checks for valgrind 
being available in the PATH and runs it or returns UNSUPPORTED if not 
available.  That is, something like nptl/tst-pthread-gdb-attach.c, which 
handles checking for whether gdb is available.

There are problems even in native-like cases when no test wrapper is being 
used.  What if glibc is being built and tested for 32-bit x86 but 
/usr/bin/true is a 64-bit binary?  A native build doesn't mean that any 
particular installed program uses the same ABI as the glibc being built.  
It's not safe to run /usr/bin/true with the newly built glibc at all (and 
some systems have /bin/true instead of /usr/bin/true).  (That issue could 
presumably be addressed by building a test program against the newly built 
glibc and running that in place of /usr/bin/true. So you end up needing to 
build two test programs against the new glibc, one to check for valgrind 
and run it if available, and the other to run under valgrind.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add valgrind smoke test

[Florian Weimer]

* Joseph Myers:

> On Mon, 24 May 2021, Carlos O'Donell via Libc-alpha wrote:
>
>> > +# Run smoke tests with valgrind to verify dynamic loader
>> > +ifneq ($(VALGRIND),false)
>> > +tests-special += $(objpfx)valgrind-smoke-test.out
>> > +$(objpfx)valgrind-smoke-test.out: $(objpfx)ld.so
>> > +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true > $@
>> > +	$(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true --help >> $@
>> > +endif
>> Does this work for cross-compiling?
>> 
>> Do you correctly detect the target valgrind and does the test wrapper 
>> allow the correct execution of valgrind on the target system?
>
> My expectation is that <host>-valgrind won't exist - but AC_CHECK_TOOL is 
> documented as falling back to the tool without a host prefix if a prefixed 
> version doesn't exist, which is a problem.  Logically, this test should 
> run if valgrind is available on the test system (which would need to be 
> determined when the tests are run, test-wrapper isn't available when glibc 
> is built), but the test-wrapper interface doesn't strictly support running 
> arbitrary installed programs like that.  So you might need a test program 
> (run on the test system via test-wrapper) that itself checks for valgrind 
> being available in the PATH and runs it or returns UNSUPPORTED if not 
> available.  That is, something like nptl/tst-pthread-gdb-attach.c, which 
> handles checking for whether gdb is available.

Would it be okay to run these tests only if $(run-built-tests) is yes,
$(cross-compiling) is no, and configure had found a valgrind binary?

I think even a minimal test which is does not work for cross-compilation
would be valuable.

Thanks,
Florian

Re: [PATCH] Add valgrind smoke test

[Joseph Myers]

On Mon, 28 Jun 2021, Florian Weimer via Libc-alpha wrote:

> Would it be okay to run these tests only if $(run-built-tests) is yes,
> $(cross-compiling) is no, and configure had found a valgrind binary?

We've got rid of the vast bulk of $(cross-compiling) conditionals that 
actually change what's built, tested or installed; I don't like the idea 
of adding more, when we already have an example of a test that deals with 
checking whether a given command (gdb in that case) is available before 
running it.

(And you can't avoid the issue of /usr/bin/true possibly not existing or 
having the wrong ABI in any case.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Fwd: [PATCH] Add valgrind smoke test

[Alexandra Petlanova Hajkova]

---------- Forwarded message ---------
From: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
Date: Fri, Jul 2, 2021 at 3:17 PM
Subject: Re: [PATCH] Add valgrind smoke test
To: Joseph Myers <joseph@codesourcery.com>




On Mon, May 24, 2021 at 9:29 PM Joseph Myers <joseph@codesourcery.com>
wrote:

> On Mon, 24 May 2021, Carlos O'Donell via Libc-alpha wrote:
>
> > > +# Run smoke tests with valgrind to verify dynamic loader
> > > +ifneq ($(VALGRIND),false)
> > > +tests-special += $(objpfx)valgrind-smoke-test.out
> > > +$(objpfx)valgrind-smoke-test.out: $(objpfx)ld.so
> > > +   $(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true > $@
> > > +   $(common-objpfx)testrun.sh --tool=valgrind-test /usr/bin/true
> --help >> $@
> > > +endif
> > Does this work for cross-compiling?
> >
> > Do you correctly detect the target valgrind and does the test wrapper
> > allow the correct execution of valgrind on the target system?
>
> My expectation is that <host>-valgrind won't exist - but AC_CHECK_TOOL is
> documented as falling back to the tool without a host prefix if a prefixed
> version doesn't exist, which is a problem.  Logically, this test should
> run if valgrind is available on the test system (which would need to be
> determined when the tests are run, test-wrapper isn't available when glibc
> is built), but the test-wrapper interface doesn't strictly support running
> arbitrary installed programs like that.  So you might need a test program
> (run on the test system via test-wrapper) that itself checks for valgrind
> being available in the PATH and runs it or returns UNSUPPORTED if not
> available.  That is, something like nptl/tst-pthread-gdb-attach.c, which
> handles checking for whether gdb is available.
>

 So I should not be using
+AC_CHECK_TOOL([VALGRIND], [valgrind], [false])
at all in my case?
I consider this patch rejected then and will try to come with something like
nptl/tst-pthread-gdb-attach.c - for example elf/tst-valgrind-smoke.c.

Thank you for the review,
Alexandra

Re: Fwd: [PATCH] Add valgrind smoke test

[Joseph Myers]

On Fri, 2 Jul 2021, Alexandra Petlanova Hajkova via Libc-alpha wrote:

>  So I should not be using
> +AC_CHECK_TOOL([VALGRIND], [valgrind], [false])
> at all in my case?

Indeed.

-- 
Joseph S. Myers
joseph@codesourcery.com