Add valgrind smoke test

Message ID 20210524121532.1374966-1-ahajkova@redhat.com
State New
Headers show
Series
  • Add valgrind smoke test
Related show

Commit Message

Florian Weimer via Libc-alpha May 24, 2021, 12:15 p.m.
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(+)

-- 
2.26.3

Comments

Florian Weimer via Libc-alpha May 24, 2021, 2:28 p.m. | #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))

> 



-- 
Cheers,
Carlos.
Joseph Myers May 24, 2021, 7:28 p.m. | #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.)

-- 
Joseph S. Myers
joseph@codesourcery.com
Florian Weimer via Libc-alpha June 28, 2021, 8:29 a.m. | #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. | #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.)

-- 
Joseph S. Myers
joseph@codesourcery.com
Florian Weimer via Libc-alpha July 2, 2021, 1:18 p.m. | #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. | #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.

-- 
Joseph S. Myers
joseph@codesourcery.com

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