public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Add valgrind smoke test
@ 2021-12-06 14:40 Alexandra Hájková
  2021-12-07 11:56 ` Mark Wielaard
                   ` (4 more replies)
  0 siblings, 5 replies; 28+ messages in thread
From: Alexandra Hájková @ 2021-12-06 14:40 UTC (permalink / raw)
  To: libc-alpha; +Cc: Alexandra Hájková, Mark Wielaard

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

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 \
@@ -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 \
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
+
+${test_wrapper_env} \
+${run_program_env} \
+$rtld --library-path "$library_path" \
+/bin/sh -c "valgrind -q --error-exitcode=1 $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;
+}
-- 
2.26.3


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
@ 2021-12-07 11:56 ` Mark Wielaard
  2021-12-07 20:32 ` DJ Delorie
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Mark Wielaard @ 2021-12-07 11:56 UTC (permalink / raw)
  To: Alexandra Hájková, libc-alpha; +Cc: Alexandra Hájková

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
  2021-12-07 11:56 ` Mark Wielaard
@ 2021-12-07 20:32 ` DJ Delorie
  2021-12-07 20:58   ` Florian Weimer
  2021-12-10 12:56   ` Mark Wielaard
  2021-12-17 18:26 ` Alexandra Hájková
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 28+ messages in thread
From: DJ Delorie @ 2021-12-07 20:32 UTC (permalink / raw)
  To: Alexandra Hájková; +Cc: libc-alpha, mark, ahajkova


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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-07 20:32 ` DJ Delorie
@ 2021-12-07 20:58   ` Florian Weimer
  2021-12-07 21:10     ` DJ Delorie
  2021-12-10 12:56   ` Mark Wielaard
  1 sibling, 1 reply; 28+ messages in thread
From: Florian Weimer @ 2021-12-07 20:58 UTC (permalink / raw)
  To: DJ Delorie via Libc-alpha
  Cc: Alexandra Hájková, DJ Delorie, mark, ahajkova

* 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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-07 20:58   ` Florian Weimer
@ 2021-12-07 21:10     ` DJ Delorie
  0 siblings, 0 replies; 28+ messages in thread
From: DJ Delorie @ 2021-12-07 21:10 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, alexandra.khirnova, mark, ahajkova


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.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-07 20:32 ` DJ Delorie
  2021-12-07 20:58   ` Florian Weimer
@ 2021-12-10 12:56   ` Mark Wielaard
  2021-12-10 13:07     ` Florian Weimer
  2021-12-10 19:15     ` DJ Delorie
  1 sibling, 2 replies; 28+ messages in thread
From: Mark Wielaard @ 2021-12-10 12:56 UTC (permalink / raw)
  To: DJ Delorie, Alexandra Hájková; +Cc: libc-alpha, ahajkova

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-10 12:56   ` Mark Wielaard
@ 2021-12-10 13:07     ` Florian Weimer
  2021-12-10 19:15     ` DJ Delorie
  1 sibling, 0 replies; 28+ messages in thread
From: Florian Weimer @ 2021-12-10 13:07 UTC (permalink / raw)
  To: Mark Wielaard
  Cc: DJ Delorie, Alexandra Hájková, libc-alpha, ahajkova

* 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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-10 12:56   ` Mark Wielaard
  2021-12-10 13:07     ` Florian Weimer
@ 2021-12-10 19:15     ` DJ Delorie
  2021-12-13 12:55       ` Mark Wielaard
  1 sibling, 1 reply; 28+ messages in thread
From: DJ Delorie @ 2021-12-10 19:15 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: alexandra.khirnova, libc-alpha, ahajkova

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.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-10 19:15     ` DJ Delorie
@ 2021-12-13 12:55       ` Mark Wielaard
  0 siblings, 0 replies; 28+ messages in thread
From: Mark Wielaard @ 2021-12-13 12:55 UTC (permalink / raw)
  To: DJ Delorie; +Cc: alexandra.khirnova, libc-alpha, ahajkova

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] Add valgrind smoke test
  2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
  2021-12-07 11:56 ` Mark Wielaard
  2021-12-07 20:32 ` DJ Delorie
@ 2021-12-17 18:26 ` Alexandra Hájková
  2021-12-17 21:07   ` DJ Delorie
  2021-12-20 11:37 ` Alexandra Hájková
  2022-01-12 17:15 ` Alexandra Hájková
  4 siblings, 1 reply; 28+ messages in thread
From: Alexandra Hájková @ 2021-12-17 18:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: Alexandra Hájková, Mark Wielaard

From: Alexandra Hájková <ahajkova@redhat.com>

Check if whether valgrind is available in the test environment.
If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
First, check if algrind works with the system ld.so in the test
environment. Then run the actual test inside the test environment,
using the just build ld.so and new libraries.

Co-authored-by: Mark Wielaard <mark@klomp.org>
---
 elf/Makefile              |  7 ++++++
 elf/tst-valgrind-smoke.sh | 46 +++++++++++++++++++++++++++++++++++++++
 elf/valgrind-test.c       | 44 +++++++++++++++++++++++++++++++++++++
 3 files changed, 97 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 fe42caeb0e..9c9d331311 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -230,6 +230,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
 	 tst-tls20 tst-tls21 tst-dlmopen-dlerror tst-dlmopen-gethostbyname \
 	 tst-dl-is_dso tst-ro-dynamic \
+	 valgrind-test \
 	 tst-audit18 \
 	 tst-rtld-run-static \
 #	 reldep9
@@ -256,6 +257,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 $(objpfx)valgrind-test
+	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(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 \
diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
new file mode 100644
index 0000000000..b51e2907a3
--- /dev/null
+++ b/elf/tst-valgrind-smoke.sh
@@ -0,0 +1,46 @@
+#!/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"
+system_rtld="$2"
+test_wrapper_env="$3"
+run_program_env="$4"
+library_path="$5"
+test_prog="$6"
+
+# Test whether valgrind is available in the test
+# environment. If not, skip the test.
+${test_wrapper_env} ${run_program_env} \
+  /bin/sh -c "command -v valgrind" || exit 77
+
+# Test valgrind works with the system ld.so in the test environment
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${system_rtld} ${test_prog} ${system_rtld}" || exit 77
+
+# Finally the actual test inside the test environment,
+# using the just build ld.so and new libraries to run
+# the smoke test under valgrind.
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
new file mode 100644
index 0000000000..00c8ec1ad3
--- /dev/null
+++ b/elf/valgrind-test.c
@@ -0,0 +1,44 @@
+/* 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>
+#include <limits.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+int
+main (int argc, char **argv)
+{
+  // Do some non-trivial stuff that has been known to trigger
+  // issues under valgrind in the past.
+  // Setting up the locale textdomain makes sure to test some
+  // string/path comparisons, file search and library loading.
+  setlocale (LC_ALL, "");
+  bindtextdomain ("translit", "");
+  textdomain ("translit");
+
+  // Show what we are executing and how...
+  char *me = realpath (argv[0], NULL);
+  printf ("bin: %s\n", me);
+  printf ("ld.so: %s\n", argv[1]);
+  free (me);
+
+  return 0;
+}
-- 
2.26.3


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-17 18:26 ` Alexandra Hájková
@ 2021-12-17 21:07   ` DJ Delorie
  2021-12-20 11:31     ` Alexandra Petlanova Hajkova
  0 siblings, 1 reply; 28+ messages in thread
From: DJ Delorie @ 2021-12-17 21:07 UTC (permalink / raw)
  To: Alexandra Hájková; +Cc: libc-alpha, mark, ahajkova


Alexandra Hjkov via Libc-alpha <libc-alpha@sourceware.org>
writes:
> Check if whether valgrind is available in the test environment.
> If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
> First, check if algrind works with the system ld.so in the test
> environment. Then run the actual test inside the test environment,
> using the just build ld.so and new libraries.
>
> Co-authored-by: Mark Wielaard <mark@klomp.org>

> diff --git a/elf/Makefile b/elf/Makefile
> +	 valgrind-test \

New test.  Ok.

> @@ -256,6 +257,12 @@ tests-special += $(objpfx)tst-audit14-cmp.out $(objpfx)tst-audit15-cmp.out \

> +tests-special += $(objpfx)tst-valgrind-smoke.out
> +$(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
> +	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' '$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; \
> +	$(evaluate-test)

We're passing everything to the script, which is $<, so "see below".
Long lines should be wrapped, but Ok with that change.

> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> +#!/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/>.

Ok.

> +set -e
> +
> +rtld="$1"
> +system_rtld="$2"
> +test_wrapper_env="$3"
> +run_program_env="$4"
> +library_path="$5"
> +test_prog="$6"

rtld is $(objpfx)ld.so
system_rtld is $(rtlddir)/$(rtld-installed-name)
test_wrapper_env is '$(test-wrapper-env)'  (either just "env" or script
   to ssh, plus "env")
run_program_env is '$(run-program-env)'  (just env vars)
library_path is '$(rpath-link)'
test_prog is $(objpfx)valgrind-test
ok

> +# Test whether valgrind is available in the test
> +# environment. If not, skip the test.
> +${test_wrapper_env} ${run_program_env} \
> +  /bin/sh -c "command -v valgrind" || exit 77

Run valgrind with no options; we should only exit if valgrind is
missing.  Ok.

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

runs on target, native valgrind with in-build test with native rtld...

Note that the test_prog built against the new glibc probably won't run
against the old installed glibc, esp if there's a version bump.  You
might need to use something other than the test_prog, like /bin/echo,
for this test.

Running the build/test on an older Fedora version should have picked
this up.

> +# Finally the actual test inside the test environment,
> +# using the just build ld.so and new libraries to run
> +# the smoke test under valgrind.
> +/bin/sh -c \
> +  "${test_wrapper_env} ${run_program_env} \
> +   valgrind -q --error-exitcode=1 \
> +     ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"

runs on target, native valgrind, runs target program with new rtld and
libraries, ok.

> diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
> +/* 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>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +int
> +main (int argc, char **argv)
> +{
> +  // Do some non-trivial stuff that has been known to trigger
> +  // issues under valgrind in the past.
> +  // Setting up the locale textdomain makes sure to test some
> +  // string/path comparisons, file search and library loading.
> +  setlocale (LC_ALL, "");
> +  bindtextdomain ("translit", "");
> +  textdomain ("translit");
> +
> +  // Show what we are executing and how...
> +  char *me = realpath (argv[0], NULL);
> +  printf ("bin: %s\n", me);
> +  printf ("ld.so: %s\n", argv[1]);
> +  free (me);
> +
> +  return 0;
> +}

Ok.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-17 21:07   ` DJ Delorie
@ 2021-12-20 11:31     ` Alexandra Petlanova Hajkova
  0 siblings, 0 replies; 28+ messages in thread
From: Alexandra Petlanova Hajkova @ 2021-12-20 11:31 UTC (permalink / raw)
  To: DJ Delorie; +Cc: Alexandra Hájková, libc-alpha, Mark Wielaard

> > +# Test valgrind works with the system ld.so in the test environment
> > +/bin/sh -c \
> > +  "${test_wrapper_env} ${run_program_env} \
> > +   valgrind -q --error-exitcode=1 \
> > +     ${system_rtld} ${test_prog} ${system_rtld}" || exit 77
>
> runs on target, native valgrind with in-build test with native rtld...
>
> Note that the test_prog built against the new glibc probably won't run
> against the old installed glibc, esp if there's a version bump.  You
> might need to use something other than the test_prog, like /bin/echo,
> for this test.
>
> Running the build/test on an older Fedora version should have picked
> this up.
>

I tried on Fedora 32 and the test ended here with UNSUPPORTED indeed,
we initially tested it on Fedora Rawhide. Using /bin/echo for the next
version.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] Add valgrind smoke test
  2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
                   ` (2 preceding siblings ...)
  2021-12-17 18:26 ` Alexandra Hájková
@ 2021-12-20 11:37 ` Alexandra Hájková
  2022-01-10 12:13   ` Mark Wielaard
  2022-01-10 12:38   ` Adhemerval Zanella
  2022-01-12 17:15 ` Alexandra Hájková
  4 siblings, 2 replies; 28+ messages in thread
From: Alexandra Hájková @ 2021-12-20 11:37 UTC (permalink / raw)
  To: libc-alpha; +Cc: Alexandra Hájková, Mark Wielaard

From: Alexandra Hájková <ahajkova@redhat.com>

Check if whether valgrind is available in the test environment.
If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
First, check if algrind works with the system ld.so in the test
environment. Then run the actual test inside the test environment,
using the just build ld.so and new libraries.

Co-authored-by: Mark Wielaard <mark@klomp.org>
---
 elf/Makefile              |  7 ++++++
 elf/tst-valgrind-smoke.sh | 46 +++++++++++++++++++++++++++++++++++++++
 elf/valgrind-test.c       | 44 +++++++++++++++++++++++++++++++++++++
 3 files changed, 97 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 fe42caeb0e..c2da351c9e 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -230,6 +230,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
 	 tst-tls20 tst-tls21 tst-dlmopen-dlerror tst-dlmopen-gethostbyname \
 	 tst-dl-is_dso tst-ro-dynamic \
+	 valgrind-test \
 	 tst-audit18 \
 	 tst-rtld-run-static \
 #	 reldep9
@@ -256,6 +257,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 $(objpfx)valgrind-test
+	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(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 \
diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
new file mode 100644
index 0000000000..cc32136268
--- /dev/null
+++ b/elf/tst-valgrind-smoke.sh
@@ -0,0 +1,46 @@
+#!/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"
+system_rtld="$2"
+test_wrapper_env="$3"
+run_program_env="$4"
+library_path="$5"
+test_prog="$6"
+
+# Test whether valgrind is available in the test
+# environment. If not, skip the test.
+${test_wrapper_env} ${run_program_env} \
+  /bin/sh -c "command -v valgrind" || exit 77
+
+# Test valgrind works with the system ld.so in the test environment
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${system_rtld} /bin/echo ${system_rtld}" || exit 77
+
+# Finally the actual test inside the test environment,
+# using the just build ld.so and new libraries to run
+# the smoke test under valgrind.
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
new file mode 100644
index 0000000000..00c8ec1ad3
--- /dev/null
+++ b/elf/valgrind-test.c
@@ -0,0 +1,44 @@
+/* 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>
+#include <limits.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+int
+main (int argc, char **argv)
+{
+  // Do some non-trivial stuff that has been known to trigger
+  // issues under valgrind in the past.
+  // Setting up the locale textdomain makes sure to test some
+  // string/path comparisons, file search and library loading.
+  setlocale (LC_ALL, "");
+  bindtextdomain ("translit", "");
+  textdomain ("translit");
+
+  // Show what we are executing and how...
+  char *me = realpath (argv[0], NULL);
+  printf ("bin: %s\n", me);
+  printf ("ld.so: %s\n", argv[1]);
+  free (me);
+
+  return 0;
+}
-- 
2.26.3


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-20 11:37 ` Alexandra Hájková
@ 2022-01-10 12:13   ` Mark Wielaard
  2022-01-10 12:38   ` Adhemerval Zanella
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Wielaard @ 2022-01-10 12:13 UTC (permalink / raw)
  To: Alexandra Hájková, libc-alpha; +Cc: Alexandra Hájková

HI,


On Mon, 2021-12-20 at 12:37 +0100, Alexandra Hájková wrote:
> Check if whether valgrind is available in the test environment.
> If not, skip the test. Run smoke tests with valgrind to verify
> dynamic loader.
> First, check if algrind works with the system ld.so in the test
> environment. Then run the actual test inside the test environment,
> using the just build ld.so and new libraries.
> [...]
> +# Test valgrind works with the system ld.so in the test environment
> +/bin/sh -c \
> +  "${test_wrapper_env} ${run_program_env} \
> +   valgrind -q --error-exitcode=1 \
> +     ${system_rtld} /bin/echo ${system_rtld}" || exit 77

So now you use /bin/echo instead of the newly build testcase.
Looks ok. The main thing is that it tests running the system ld.so
under valgrind works. If it does, then the new ld.so should too.

I think that is correct given DJ's feedback. Although I admit am a
little surprised to be honest. I didn't think the test used any new
symbol (versions), so would also work against the system libc. But
since it was build against the fresh glibc, it does make sense it could
pull in newer symbol versions than are available on the system.

This version looks good to me to get integrated.

Thanks,

Mark

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-12-20 11:37 ` Alexandra Hájková
  2022-01-10 12:13   ` Mark Wielaard
@ 2022-01-10 12:38   ` Adhemerval Zanella
  1 sibling, 0 replies; 28+ messages in thread
From: Adhemerval Zanella @ 2022-01-10 12:38 UTC (permalink / raw)
  To: libc-alpha



On 20/12/2021 08:37, Alexandra Hájková via Libc-alpha wrote:
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
> Check if whether valgrind is available in the test environment.
> If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
> First, check if algrind works with the system ld.so in the test
> environment. Then run the actual test inside the test environment,
> using the just build ld.so and new libraries.
> 
> Co-authored-by: Mark Wielaard <mark@klomp.org>
> ---
>  elf/Makefile              |  7 ++++++
>  elf/tst-valgrind-smoke.sh | 46 +++++++++++++++++++++++++++++++++++++++
>  elf/valgrind-test.c       | 44 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 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 fe42caeb0e..c2da351c9e 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -230,6 +230,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
>  	 tst-tls20 tst-tls21 tst-dlmopen-dlerror tst-dlmopen-gethostbyname \
>  	 tst-dl-is_dso tst-ro-dynamic \
> +	 valgrind-test \
>  	 tst-audit18 \
>  	 tst-rtld-run-static \
>  #	 reldep9
> @@ -256,6 +257,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 $(objpfx)valgrind-test
> +	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(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 \
> diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
> new file mode 100644
> index 0000000000..cc32136268
> --- /dev/null
> +++ b/elf/tst-valgrind-smoke.sh
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +# Valgrind smoke test.
> +# Copyright (C) 2021 Free Software Foundation, Inc.

Update the Copyright date.

> +# 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"
> +system_rtld="$2"
> +test_wrapper_env="$3"
> +run_program_env="$4"
> +library_path="$5"
> +test_prog="$6"
> +
> +# Test whether valgrind is available in the test
> +# environment. If not, skip the test.

I think double space after period should be used on shell comments as well.

> +${test_wrapper_env} ${run_program_env} \
> +  /bin/sh -c "command -v valgrind" || exit 77
> +
> +# Test valgrind works with the system ld.so in the test environment
> +/bin/sh -c \
> +  "${test_wrapper_env} ${run_program_env} \
> +   valgrind -q --error-exitcode=1 \
> +     ${system_rtld} /bin/echo ${system_rtld}" || exit 77
> +
> +# Finally the actual test inside the test environment,
> +# using the just build ld.so and new libraries to run
> +# the smoke test under valgrind.
> +/bin/sh -c \
> +  "${test_wrapper_env} ${run_program_env} \
> +   valgrind -q --error-exitcode=1 \
> +     ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
> diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
> new file mode 100644
> index 0000000000..00c8ec1ad3
> --- /dev/null
> +++ b/elf/valgrind-test.c
> @@ -0,0 +1,44 @@
> +/* 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.

Ditto.

> +   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>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +int
> +main (int argc, char **argv)
> +{
> +  // Do some non-trivial stuff that has been known to trigger
> +  // issues under valgrind in the past.
> +  // Setting up the locale textdomain makes sure to test some
> +  // string/path comparisons, file search and library loading.

Please use '/* ... */' comments.

> +  setlocale (LC_ALL, "");

Use xsetlocale.

> +  bindtextdomain ("translit", "");
> +  textdomain ("translit");

Also check the return here.

> +
> +  // Show what we are executing and how...
> +  char *me = realpath (argv[0], NULL);
> +  printf ("bin: %s\n", me);
> +  printf ("ld.so: %s\n", argv[1]);
> +  free (me);
> +
> +  return 0;
> +}

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] Add valgrind smoke test
  2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
                   ` (3 preceding siblings ...)
  2021-12-20 11:37 ` Alexandra Hájková
@ 2022-01-12 17:15 ` Alexandra Hájková
  2022-01-20 19:35   ` Alexandra Hájková
  2022-01-20 21:29   ` DJ Delorie
  4 siblings, 2 replies; 28+ messages in thread
From: Alexandra Hájková @ 2022-01-12 17:15 UTC (permalink / raw)
  To: libc-alpha; +Cc: Alexandra Hájková, Mark Wielaard

From: Alexandra Hájková <ahajkova@redhat.com>

Check if whether valgrind is available in the test environment.
If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
First, check if algrind works with the system ld.so in the test
environment. Then run the actual test inside the test environment,
using the just build ld.so and new libraries.

Co-authored-by: Mark Wielaard <mark@klomp.org>
---
 elf/Makefile              |  7 ++++++
 elf/tst-valgrind-smoke.sh | 46 ++++++++++++++++++++++++++++++++++++
 elf/valgrind-test.c       | 49 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 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 b86d116be9..77829cf967 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -327,6 +327,7 @@ tests += \
   unload6 \
   unload7 \
   unload8 \
+  valgrind-test \
 #	 reldep9
 tests-cxx = \
   tst-dlopen-nodelete-reloc \
@@ -361,6 +362,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 $(objpfx)valgrind-test
+	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(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 \
diff --git a/elf/tst-valgrind-smoke.sh b/elf/tst-valgrind-smoke.sh
new file mode 100644
index 0000000000..7ce4af6897
--- /dev/null
+++ b/elf/tst-valgrind-smoke.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+# Valgrind smoke test.
+# Copyright (C) 2022 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"
+system_rtld="$2"
+test_wrapper_env="$3"
+run_program_env="$4"
+library_path="$5"
+test_prog="$6"
+
+# Test whether valgrind is available in the test
+# environment.  If not, skip the test.
+${test_wrapper_env} ${run_program_env} \
+  /bin/sh -c "command -v valgrind" || exit 77
+
+# Test valgrind works with the system ld.so in the test environment
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${system_rtld} /bin/echo ${system_rtld}" || exit 77
+
+# Finally the actual test inside the test environment,
+# using the just build ld.so and new libraries to run
+# the smoke test under valgrind.
+/bin/sh -c \
+  "${test_wrapper_env} ${run_program_env} \
+   valgrind -q --error-exitcode=1 \
+     ${rtld} --library-path ${library_path} ${test_prog} ${rtld}"
diff --git a/elf/valgrind-test.c b/elf/valgrind-test.c
new file mode 100644
index 0000000000..71b8a09b35
--- /dev/null
+++ b/elf/valgrind-test.c
@@ -0,0 +1,49 @@
+/* This is the simple test intended to be called by
+   tst-valgrind-smoke to perform vagrind smoke test.
+   Copyright (C) 2022 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 <errno.h>
+#include <libintl.h>
+#include <locale.h>
+#include <limits.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <support/support.h>
+
+int
+main (int argc, char **argv)
+{
+  /* Do some non-trivial stuff that has been known to trigger
+     issues under valgrind in the past.
+     Setting up the locale textdomain makes sure to test some
+     string/path comparisons, file search and library loading. */
+  xsetlocale (LC_ALL, "");
+  if (bindtextdomain ("translit", "") == NULL)
+      return errno;
+  if (textdomain ("translit") == NULL)
+      return errno;
+
+  /* Show what we are executing and how...  */
+  char *me = realpath (argv[0], NULL);
+  printf ("bin: %s\n", me);
+  printf ("ld.so: %s\n", argv[1]);
+  free (me);
+
+  return 0;
+}
-- 
2.26.3


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-12 17:15 ` Alexandra Hájková
@ 2022-01-20 19:35   ` Alexandra Hájková
  2022-01-24 18:34     ` Joseph Myers
  2022-01-20 21:29   ` DJ Delorie
  1 sibling, 1 reply; 28+ messages in thread
From: Alexandra Hájková @ 2022-01-20 19:35 UTC (permalink / raw)
  To: libc-alpha
  Cc: Alexandra Hájková,
	Mark Wielaard, dj, Adhemerval Zanella, Joseph Myers

On Wed, Jan 12, 2022 at 6:15 PM Alexandra Hájková
<alexandra.khirnova@gmail.com> wrote:
>
> From: Alexandra Hájková <ahajkova@redhat.com>
>
> Check if whether valgrind is available in the test environment.
> If not, skip the test. Run smoke tests with valgrind to verify dynamic loader.
> First, check if algrind works with the system ld.so in the test
> environment. Then run the actual test inside the test environment,
> using the just build ld.so and new libraries.
>
> Co-authored-by: Mark Wielaard <mark@klomp.org>

Are there any objections to the new version?

Thank you,

Alexandra

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-12 17:15 ` Alexandra Hájková
  2022-01-20 19:35   ` Alexandra Hájková
@ 2022-01-20 21:29   ` DJ Delorie
  1 sibling, 0 replies; 28+ messages in thread
From: DJ Delorie @ 2022-01-20 21:29 UTC (permalink / raw)
  To: Alexandra Hájková; +Cc: libc-alpha, mark, ahajkova


I think this addresses all my issues.  If anything else pops up we can
fix it once we have a known problematic environment.

Reviewed-by: DJ Delorie <dj@redhat.com>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-20 19:35   ` Alexandra Hájková
@ 2022-01-24 18:34     ` Joseph Myers
  2022-01-26 17:46       ` Joseph Myers
  2022-01-26 17:59       ` Mark Wielaard
  0 siblings, 2 replies; 28+ messages in thread
From: Joseph Myers @ 2022-01-24 18:34 UTC (permalink / raw)
  To: alexandra.khirnova; +Cc: libc-alpha, Mark Wielaard, Alexandra Hájková

I'm seeing

FAIL: elf/tst-valgrind-smoke

for the x86_64 configurations when running build-many-glibcs.py (and 
UNSUPPORTED for other architectures).  No execution tests of the newly 
built glibc should be run at all unless

ifeq ($(run-built-tests),yes)

but I don't see any such conditional around the

tests-special += $(objpfx)tst-valgrind-smoke.out

that causes this test to be run.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-24 18:34     ` Joseph Myers
@ 2022-01-26 17:46       ` Joseph Myers
  2022-01-26 17:59       ` Mark Wielaard
  1 sibling, 0 replies; 28+ messages in thread
From: Joseph Myers @ 2022-01-26 17:46 UTC (permalink / raw)
  To: alexandra.khirnova; +Cc: Mark Wielaard, libc-alpha, Alexandra Hájková

On Mon, 24 Jan 2022, Joseph Myers wrote:

> I'm seeing
> 
> FAIL: elf/tst-valgrind-smoke
> 
> for the x86_64 configurations when running build-many-glibcs.py (and 
> UNSUPPORTED for other architectures).  No execution tests of the newly 
> built glibc should be run at all unless
> 
> ifeq ($(run-built-tests),yes)
> 
> but I don't see any such conditional around the
> 
> tests-special += $(objpfx)tst-valgrind-smoke.out
> 
> that causes this test to be run.

I haven't seen any further comments on or fixes for this.  We're very 
close to a release, so please revert the patch introducing problems if 
it's not going to be fixed in the next day or two.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-24 18:34     ` Joseph Myers
  2022-01-26 17:46       ` Joseph Myers
@ 2022-01-26 17:59       ` Mark Wielaard
  2022-01-26 18:40         ` Joseph Myers
  1 sibling, 1 reply; 28+ messages in thread
From: Mark Wielaard @ 2022-01-26 17:59 UTC (permalink / raw)
  To: Joseph Myers, alexandra.khirnova; +Cc: libc-alpha, Alexandra Hájková

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

Hi Joseph,

On Mon, 2022-01-24 at 18:34 +0000, Joseph Myers wrote:
> I'm seeing
> 
> FAIL: elf/tst-valgrind-smoke
> 
> for the x86_64 configurations when running build-many-glibcs.py (and 
> UNSUPPORTED for other architectures).  No execution tests of the newly 
> built glibc should be run at all unless
> 
> ifeq ($(run-built-tests),yes)
> 
> but I don't see any such conditional around the
> 
> tests-special += $(objpfx)tst-valgrind-smoke.out
> 
> that causes this test to be run.

If your analysis is correct then the attached patch should solve the
issue. Could you try that out?

Thanks,

Mark


[-- Attachment #2: 0001-Guard-tst-valgrind-smoke.out-with-run-built-tests.patch --]
[-- Type: text/x-patch, Size: 871 bytes --]

From cc4798c2bf6acfb9e580fab6e4bad469f600e212 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Wed, 26 Jan 2022 18:57:29 +0100
Subject: [PATCH] Guard tst-valgrind-smoke.out with run-built-tests

Prevent tst-valgrind-smoke from running when run-built-tests is not yes.
---
 elf/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/elf/Makefile b/elf/Makefile
index daafb5cf12..775c755291 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -530,7 +530,9 @@ endif
 endif
 endif
 
+ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-valgrind-smoke.out
+endif
 $(objpfx)tst-valgrind-smoke.out: tst-valgrind-smoke.sh $(objpfx)ld.so $(objpfx)valgrind-test
 	$(SHELL) $< $(objpfx)ld.so  $(rtlddir)/$(rtld-installed-name) '$(test-wrapper-env)' \
 		'$(run-program-env)' '$(rpath-link)' $(objpfx)valgrind-test > $@; $(evaluate-test)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-26 17:59       ` Mark Wielaard
@ 2022-01-26 18:40         ` Joseph Myers
  2022-01-26 19:23           ` Mark Wielaard
  0 siblings, 1 reply; 28+ messages in thread
From: Joseph Myers @ 2022-01-26 18:40 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: alexandra.khirnova, libc-alpha, Alexandra Hájková

On Wed, 26 Jan 2022, Mark Wielaard wrote:

> If your analysis is correct then the attached patch should solve the
> issue. Could you try that out?

I confirm that this fixes the problem.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2022-01-26 18:40         ` Joseph Myers
@ 2022-01-26 19:23           ` Mark Wielaard
  0 siblings, 0 replies; 28+ messages in thread
From: Mark Wielaard @ 2022-01-26 19:23 UTC (permalink / raw)
  To: Joseph Myers; +Cc: alexandra.khirnova, libc-alpha, Alexandra Hájková

On Wed, Jan 26, 2022 at 06:40:10PM +0000, Joseph Myers wrote:
> On Wed, 26 Jan 2022, Mark Wielaard wrote:
> 
> > If your analysis is correct then the attached patch should solve the
> > issue. Could you try that out?
> 
> I confirm that this fixes the problem.

Great. I did a local make && make check and tst-valgrind-smoke.out is
still generated and the testcase still PASSes locally.

Can I push the patch as is to master, or are we that close to a
release that a release manager needs to ack the patch first?

Thanks,

Mark


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-06-28  8:29     ` Florian Weimer
@ 2021-06-28 18:33       ` Joseph Myers
  0 siblings, 0 replies; 28+ messages in thread
From: Joseph Myers @ 2021-06-28 18:33 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Alexandra Hájková

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-05-24 19:28   ` Joseph Myers
@ 2021-06-28  8:29     ` Florian Weimer
  2021-06-28 18:33       ` Joseph Myers
  0 siblings, 1 reply; 28+ messages in thread
From: Florian Weimer @ 2021-06-28  8:29 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Carlos O'Donell, libc-alpha, Alexandra Hájková

* 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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-05-24 14:28 ` Carlos O'Donell
@ 2021-05-24 19:28   ` Joseph Myers
  2021-06-28  8:29     ` Florian Weimer
  0 siblings, 1 reply; 28+ messages in thread
From: Joseph Myers @ 2021-05-24 19:28 UTC (permalink / raw)
  To: Carlos O'Donell
  Cc: Alexandra Hájková, libc-alpha, Alexandra Hájková

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] Add valgrind smoke test
  2021-05-24 12:15 Alexandra Hájková
@ 2021-05-24 14:28 ` Carlos O'Donell
  2021-05-24 19:28   ` Joseph Myers
  0 siblings, 1 reply; 28+ messages in thread
From: Carlos O'Donell @ 2021-05-24 14:28 UTC (permalink / raw)
  To: Alexandra Hájková, libc-alpha; +Cc: Alexandra Hájková

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.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH] Add valgrind smoke test
@ 2021-05-24 12:15 Alexandra Hájková
  2021-05-24 14:28 ` Carlos O'Donell
  0 siblings, 1 reply; 28+ messages in thread
From: Alexandra Hájková @ 2021-05-24 12:15 UTC (permalink / raw)
  To: libc-alpha; +Cc: 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


^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2022-01-26 19:23 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-06 14:40 [PATCH] Add valgrind smoke test Alexandra Hájková
2021-12-07 11:56 ` Mark Wielaard
2021-12-07 20:32 ` DJ Delorie
2021-12-07 20:58   ` Florian Weimer
2021-12-07 21:10     ` DJ Delorie
2021-12-10 12:56   ` Mark Wielaard
2021-12-10 13:07     ` Florian Weimer
2021-12-10 19:15     ` DJ Delorie
2021-12-13 12:55       ` Mark Wielaard
2021-12-17 18:26 ` Alexandra Hájková
2021-12-17 21:07   ` DJ Delorie
2021-12-20 11:31     ` Alexandra Petlanova Hajkova
2021-12-20 11:37 ` Alexandra Hájková
2022-01-10 12:13   ` Mark Wielaard
2022-01-10 12:38   ` Adhemerval Zanella
2022-01-12 17:15 ` Alexandra Hájková
2022-01-20 19:35   ` Alexandra Hájková
2022-01-24 18:34     ` Joseph Myers
2022-01-26 17:46       ` Joseph Myers
2022-01-26 17:59       ` Mark Wielaard
2022-01-26 18:40         ` Joseph Myers
2022-01-26 19:23           ` Mark Wielaard
2022-01-20 21:29   ` DJ Delorie
  -- strict thread matches above, loose matches on Subject: below --
2021-05-24 12:15 Alexandra Hájková
2021-05-24 14:28 ` Carlos O'Donell
2021-05-24 19:28   ` Joseph Myers
2021-06-28  8:29     ` Florian Weimer
2021-06-28 18:33       ` Joseph Myers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).