diff mbox series

Add valgrind smoke test

Message ID 20210524121532.1374966-1-ahajkova@redhat.com
State Changes Requested
Headers show
Series Add valgrind smoke test | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Alexandra Hájková May 24, 2021, 12:15 p.m. UTC
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(+)

Comments

Carlos O'Donell May 24, 2021, 2:28 p.m. UTC | #1
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))
>
Joseph Myers May 24, 2021, 7:28 p.m. UTC | #2
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.)
Florian Weimer June 28, 2021, 8:29 a.m. UTC | #3
* 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
Joseph Myers June 28, 2021, 6:33 p.m. UTC | #4
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.)
Alexandra Petlanova Hajkova July 2, 2021, 1:18 p.m. UTC | #5
---------- 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
Joseph Myers July 5, 2021, 8:28 p.m. UTC | #6
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.
diff mbox series

Patch

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