patch rfc - valgrind vs. debuginfod

patch rfc - valgrind vs. debuginfod

[Frank Ch. Eigler]

Hi -

The following patch appears to make valgrind consistently happy,
whether distcheck or check runs.  It siply arranges to make sure that
$VALGRIND_CMD is run without debuginfod client being enabled, even as
the $cmd it runs gets the necessary env var set.

I don't completely understand the connection to the weird symptoms
(32-bit backtraces on 64-bit hosts, missing suppressions?) that we
noticed earlier.

diff --git a/tests/test-subr.sh b/tests/test-subr.sh
index 411e5f288acd..2ea6398c0932 100644
--- a/tests/test-subr.sh
+++ b/tests/test-subr.sh
@@ -83,7 +83,7 @@ testrun()
 built_testrun()
 {
   LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"\
-  $VALGRIND_CMD "$@"
+  env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" "$@"
 }
 
 installed_testrun()
@@ -104,9 +104,9 @@ installed_testrun()
   if [ "${libdir}" != /usr/lib ] && [ "${libdir}" != /usr/lib64 ]; then
     LD_LIBRARY_PATH="${libdir}:${libdir}/elfutils\
 ${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH" \
-    $VALGRIND_CMD $program ${1+"$@"}
+    env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" $program ${1+"$@"}
   else
-    $VALGRIND_CMD $program ${1+"$@"}
+    env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" $program ${1+"$@"}
   fi
 }
 

Re: patch rfc - valgrind vs. debuginfod

[Mark Wielaard]

Hi Frank,

On Wed, 2021-06-16 at 22:34 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> The following patch appears to make valgrind consistently happy,
> whether distcheck or check runs.  It siply arranges to make sure that
> $VALGRIND_CMD is run without debuginfod client being enabled, even as
> the $cmd it runs gets the necessary env var set.

This makes sense, the fedora buildbot worker was upgraded to Fedora 34
over the weekend, which contains valgrind 3.17.0 which has debuginfod
client support and that might interact with the testing...

> I don't completely understand the connection to the weird symptoms
> (32-bit backtraces on 64-bit hosts, missing suppressions?) that we
> noticed earlier.

Yeah, that is a bit of a mystery.

> diff --git a/tests/test-subr.sh b/tests/test-subr.sh
> index 411e5f288acd..2ea6398c0932 100644
> --- a/tests/test-subr.sh
> +++ b/tests/test-subr.sh
> @@ -83,7 +83,7 @@ testrun()
>  built_testrun()
>  {
>    LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"\
> -  $VALGRIND_CMD "$@"
> +  env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" "$@"
>  }
>  
>  installed_testrun()
> @@ -104,9 +104,9 @@ installed_testrun()
>    if [ "${libdir}" != /usr/lib ] && [ "${libdir}" != /usr/lib64 ]; then
>      LD_LIBRARY_PATH="${libdir}:${libdir}/elfutils\
>  ${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH" \
> -    $VALGRIND_CMD $program ${1+"$@"}
> +    env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" $program ${1+"$@"}
>    else
> -    $VALGRIND_CMD $program ${1+"$@"}
> +    env -u DEBUGINFOD_URLS $VALGRIND_CMD env DEBUGINFOD_URLS="$DEBUGINFOD_URLS" $program ${1+"$@"}
>    fi
>  }

I think there are 3 issues with this implementation of the fix.

First, VALGRIND_CMD could be unset (some testcases that introspect
themselves do that), in which case you are unnecessary running two env
wrappers on the testcase (not a big deal though).

Second this is only one of the places VALGRIND_CMD is used (in run*sh
tests), the other place is in tests/test-wrapper.sh for native tests.

Third valgrind by default only runs on the program it is invoked with.
So in this case it only runs on env, but doesn't run on the actual
program env invokes. For that you'll need to invoke valgrind with --
trace-children=yes.

I think the first can be fixed by simply testing whether VALGRIND_CMD
is set before adding the env wrapping.

The second can be fixed by doing the same env -u env DEBUGNFOD_URLS
wrapping in tests/test-wrapper.sh.

For the third issue we should probably add trace-children=yes to
valgrind_cmd in tests/Makefile.am.

Cheers,

Mark

Re: patch rfc - valgrind vs. debuginfod

[Mark Wielaard]

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

Hi Frank,

On irc we discussed some issues with trying to disable debuginfod
client for valgrind while actually using it in the to be tested
executable. The main issue is that valgrind and the test executable are
actually one and the same. So disabling or enabling DEBUGINFOD_URLS for
one also disables or enables it for the other.

So maybe it is just simpler to disable testing under valgrind for those
tests that are testing the debuginfod client cache dir directly.

That is what the attached patch does. It is pity to not have valgrind
run over all the tests, but it is the simplest I could come up with.
And this has given us more headaches than we deserve in the first
place.

What do you think?

Thanks,

Mark

[-- Attachment #2: 0001-run-debuginfod-find.sh-Disable-valgrind-for-debuginf.patch --]
[-- Type: text/x-patch, Size: 1721 bytes --]

From ad3c57cfb68d1ef5f6a1d426578d8005048c251e Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Fri, 2 Jul 2021 18:16:00 +0200
Subject: [PATCH] run-debuginfod-find.sh: Disable valgrind for debuginfod
 client cache tests

valgrind itself might use the debuginfod client. So disable valgrind
when testing the cache.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 tests/ChangeLog              | 5 +++++
 tests/run-debuginfod-find.sh | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/tests/ChangeLog b/tests/ChangeLog
index d8fa97fa..a0e4ec52 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2021-07-02  Mark Wielaard  <mark@klomp.org>
+
+	* run-debuginfo-find.sh: unset VALGRIND_CMD before testing debuginfod
+	client cache.
+
 2021-06-16  Frank Ch. Eigler <fche@redhat.com>
 
 	* run-debuginfod-find.sh: Fix intermittent groom/stale failure,
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 456dc2f8..74a5ceff 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -583,6 +583,11 @@ curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/debuginfo > /dev/null || true
 curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/badtype > /dev/null || true
 (curl -s http://127.0.0.1:$PORT2/metrics | grep 'badtype') && false
 
+# DISABLE VALGRIND checking because valgrind might use debuginfod client
+# requests itself, causing confusion about who put what in the cache.
+# It stays disabled till the end of this test.
+unset VALGRIND_CMD
+
 # Confirm that reused curl connections survive 404 errors.
 # The rm's force an uncached fetch
 rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo .client_cache*/$BUILDID/debuginfo
-- 
2.18.4

Re: patch rfc - valgrind vs. debuginfod

[Frank Ch. Eigler]

Hi -

> On irc we discussed some issues with trying to disable debuginfod
> client for valgrind while actually using it in the to be tested
> executable. The main issue is that valgrind and the test executable are
> actually one and the same. So disabling or enabling DEBUGINFOD_URLS for
> one also disables or enables it for the other.

Yeah, it makes sense to disable this testing.  You're doing it late
enough that -some- valgrinding will still happen, so lgtm.

- FChE