bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Simon South]

strace 5.13 in core-updates-frozen appears to build fine but fails its
"readlinkat" test for me on AArch64 (real hardware; a ROCK64).  This is
the only test that fails.

From the build directory, strace-5.13/tests/readlinkat.dir/exp contains

  readlinkat(AT_FDCWD, "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x6c\x69\x6e\x6b", 0xfffff7e2cfea, 22) = -1 ENOENT (No such file or directory)
  readlinkat(AT_FDCWD, "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x6c\x69\x6e\x6b", "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x74\x61\x72\x67\x65\x74", 22) = 22
  +++ exited with 0 +++

but strace-5.13/tests/readlinkat.dir/out shows

  readlinkat(AT_FDCWD, "\x2f\x70\x72\x6f\x63\x2f\x73\x65\x6c\x66\x2f\x65\x78\x65", "\x2f\x74\x6d\x70\x2f\x67\x75\x69\x78\x2d\x62\x75\x69\x6c\x64\x2d\x73\x74\x72\x61\x63\x65\x2d\x35\x2e\x31\x33\x2e\x64\x72\x76\x2d"..., 4096) = 62
  readlinkat(AT_FDCWD, "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x6c\x69\x6e\x6b", 0xfffff7e2cfea, 22) = -1 ENOENT (No such file or directory)
  readlinkat(AT_FDCWD, "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x6c\x69\x6e\x6b", "\x74\x65\x73\x74\x2e\x72\x65\x61\x64\x6c\x69\x6e\x6b\x61\x74\x2e\x74\x61\x72\x67\x65\x74", 22) = 22
  +++ exited with 0 +++

The only difference is an additional line at the start of the generated
output.  I see this in the strace package definition
(gnu/packages/linux.scm:2256):

  ;; XXX: This test fails because an extra readlink call is made
  ;; by the glibc when using the ld.so cache.
  (("readlink.gen.test[^:]") " ")

Perhaps the same is true for readlinkat on AArch64?

-- 
Simon South
simon@simonsouth.net

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Simon South]

Patching strace to add a "--trace-path" parameter to the two tests'
definitions as in the patch below seems to fix this issue on both
AArch64 and x86-64, and is less drastic than disabling the tests
altogether.

The changes limit strace's output during testing to only calls affecting
files in the test directory itself, effectively filtering out the
'readlink("/proc/self/exe")' call from glibc that is throwing the tests
currently.  You can see a number of other places in gen_tests.in where
this is done, presumably for similar reasons.

Does this seem reasonable?

-- 
Simon South
simon@simonsouth.net


diff --git a/tests/gen_tests.in b/tests/gen_tests.in
index 8b4e2e9..cc3ca63 100644
--- a/tests/gen_tests.in
+++ b/tests/gen_tests.in
@@ -623,8 +623,8 @@ quotactl-xfs-v	-v -e trace=quotactl
 read-write	-a15 -eread=0,5 -ewrite=1,4 -e trace=read,write -P read-write-tmpfile -P /dev/zero -P /dev/null
 readahead	-a1
 readdir	-a16
-readlink	-xx
-readlinkat	-xx
+readlink	-xx --trace-path=test.readlink.link
+readlinkat	-xx --trace-path=test.readlinkat.link
 reboot		-s 256
 recv-MSG_TRUNC	-a26 -e trace=recv
 recvfrom	-a35

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Bengt Richter]

Hi,

On +2021-09-02 18:41:12 -0400, Simon South wrote:
> Patching strace to add a "--trace-path" parameter to the two tests'
> definitions as in the patch below seems to fix this issue on both
> AArch64 and x86-64, and is less drastic than disabling the tests
> altogether.
> 
> The changes limit strace's output during testing to only calls affecting
> files in the test directory itself, effectively filtering out the
> 'readlink("/proc/self/exe")' call from glibc that is throwing the tests
> currently.  You can see a number of other places in gen_tests.in where
> this is done, presumably for similar reasons.
> 
> Does this seem reasonable?
>

I worry about disabling tests in too general a way, since it creates
a hiding place which conceivably someone really clever may be able exploit.

So I wonder whether what you are doing is making it possible to
configure tests to have (narrow) context-sensitive expectations
(e.g., in this case making the test handle the error as an expected one,
as opposed to being made ignorant of it), or building in a static and
probably too general configuration.

A proper configurability, ISTM, would be preferable to any other form
of more general filtering.

Sorry if this is just noise from a lurker with insufficient knowledge
of the issue and discussions. If so please ignore ;/

of13> definitions as in the patch below seems to fix this issue on both                                                                                          


> -- 
> Simon South
> simon@simonsouth.net
> 
> 
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index 8b4e2e9..cc3ca63 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -623,8 +623,8 @@ quotactl-xfs-v	-v -e trace=quotactl
>  read-write	-a15 -eread=0,5 -ewrite=1,4 -e trace=read,write -P read-write-tmpfile -P /dev/zero -P /dev/null
>  readahead	-a1
>  readdir	-a16
> -readlink	-xx
> -readlinkat	-xx
> +readlink	-xx --trace-path=test.readlink.link
> +readlinkat	-xx --trace-path=test.readlinkat.link
>  reboot		-s 256
>  recv-MSG_TRUNC	-a26 -e trace=recv
>  recvfrom	-a35
> 
> 
> 

-- 
Regards,
Bengt Richter

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Simon South]

Bengt Richter <bokr@bokr.com> writes:
> A proper configurability, ISTM, would be preferable to any other form
> of more general filtering.

I agree with you on the need to be cautious around modifying test cases
but I'm not sure I follow you otherwise.  What would "proper
configurability" look like in this case?

The change I'm proposing here narrows the two test cases so they test
only what appears to have been intended, i.e. that strace can capture
readlink(at) system calls and that it reports them in the format
expected by the developers.  It does not affect other test cases or the
test suite as a whole.

The obvious alternative would be to modify the test cases' expected
output to match what is actually generated, but this could have the side
effect of tying the package to Linux and perhaps to specific versions of
glibc.

That said I'm still not sure why this additional syscall is being made
in the first place, only that it appears to originate from glibc's
"_dl_get_origin" function[0].  If I build strace from source "manually"
the tests complete fine without modification.  I presume the extra call
has to do with the fact Guix builds strace inside a container; does
anyone know how this could be affecting the way programs are loaded?

-- 
Simon South
simon@simonsouth.net

[0] See e.g. glibc's sysdeps/unix/sysv/linux/dl-origin.c for the x86-64
    case, as well as "_dl_non_dynamic_init" in elf/dl-support.c, which
    seems to be the only place it is called.

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Bengt Richter]

On +2021-09-03 10:00:33 -0400, Simon South wrote:
> Bengt Richter <bokr@bokr.com> writes:
> > A proper configurability, ISTM, would be preferable to any other form
> > of more general filtering.
> 
> I agree with you on the need to be cautious around modifying test cases
> but I'm not sure I follow you otherwise.  What would "proper
> configurability" look like in this case?
> 
> The change I'm proposing here narrows the two test cases so they test
> only what appears to have been intended, i.e. that strace can capture
> readlink(at) system calls and that it reports them in the format
> expected by the developers.  It does not affect other test cases or the
> test suite as a whole.
> 
> The obvious alternative would be to modify the test cases' expected
> output to match what is actually generated, but this could have the side
> effect of tying the package to Linux and perhaps to specific versions of
> glibc.

Well, that would be the point :)
I.e., to move the customizations into .conf files and out of test-suite sources.

> 
> That said I'm still not sure why this additional syscall is being made
> in the first place, only that it appears to originate from glibc's
> "_dl_get_origin" function[0].  If I build strace from source "manually"
> the tests complete fine without modification.  I presume the extra call
> has to do with the fact Guix builds strace inside a container; does
> anyone know how this could be affecting the way programs are loaded?
>

I did not realize there were mods to strace itself affecting this.
I thought it was about somehow filtering its output to fool tests, sorry.

With strace variants maybe "proper configurability"
should have to consider strace versions also, to tune test expectations ;/

> -- 
> Simon South
> simon@simonsouth.net
> 
> [0] See e.g. glibc's sysdeps/unix/sysv/linux/dl-origin.c for the x86-64
>     case, as well as "_dl_non_dynamic_init" in elf/dl-support.c, which
>     seems to be the only place it is called.

-- 
Regards,
Bengt Richter

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Simon South]

Bengt Richter <bokr@bokr.com> writes:
> Well, that would be the point :)
> I.e., to move the customizations into .conf files and out of
> test-suite sources.

Given the limited flexibility of the test harness I'm not sure there's a
solution that doesn't involve modifying the source, but on reflection
you're right: We actually _do_ expect the test results to differ from
what is provided in the source bundle, so a better solution would be to
update the tests to reflect this rather than to change how they're run.
I'll put together a patch that does that.

Incidentally, the reason for the test failures appears to be the
additional call to _dl_get_origin() added to glibc by the
"glib-dl-cache" patch in commit 52564e9, which produces an additional
readlink/readlinkat syscall strace's test driver isn't expecting.  But
this additional call _is_ expected on Guix systems, so the test cases
ought to be modified to match.

-- 
Simon South
simon@simonsouth.net

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Simon South]

Simon South <simon@simonsouth.net> writes:
> But this additional call _is_ expected on Guix systems, so the test
> cases ought to be modified to match.

Perhaps, but having looked into this it's complicated because

- The expected output from the tests is not contained in the source
  bundle in a separate, easy-to-patch file but is actually generated by
  the C code under test as it runs;

- Even if that weren't true, only one test must be patched for both to
  succeed, and the choice depends on the target architecture so there
  wouldn't be a single patch that could work in all cases; and

- The additional output that needs to be generated by the C code
  actually embeds part of a store path, meaning this would need to be
  determined by the code at runtime (possibly yielding even more
  "readlink" calls that would need to be accounted for) in addition to
  truncating and formatting the output to match what strace itself
  produces...

It's too much.  I'm going to follow up with a patch that basically
applies the diff above in a tidy manner, and I think that will be the
best solution.  It is a very limited change that does not alter the
purpose of the tests; does not allow them to pass where they would
normally fail; and will work equally well on all systems, even if a
completely different glibc package is introduced.  Certainly it is an
improvement over simply disabling both tests.

-- 
Simon South
simon@simonsouth.net

bug#50346: [PATCH core-updates-frozen] gnu: strace: Allow readlink, readlinkat tests to pass.

[Simon South]

Modify the invocation of strace's "readlink" and "readlinkat" tests to prevent
them from failing due to an additional system call made by Guix's patched
version of glibc.

* gnu/packages/linux.scm (strace)[source]: Add patch.
[arguments]<#:phases>: Do not disable the "readlink" test now that it can
succeed.
* gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
---
 gnu/local.mk                                  |  1 +
 gnu/packages/linux.scm                        |  8 ++--
 ...strace-fix-readlink-readlinkat-tests.patch | 46 +++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)
 create mode 100644 gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index bb22e29caa..f9c8956568 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1547,6 +1547,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/sdl-pango-sans-serif.patch		\
   %D%/packages/patches/smalltalk-multiplication-overflow.patch	\
   %D%/packages/patches/sqlite-hurd.patch			\
+  %D%/packages/patches/strace-fix-readlink-readlinkat-tests.patch	\
   %D%/packages/patches/sunxi-tools-remove-sys-io.patch	\
   %D%/packages/patches/patchutils-test-perms.patch		\
   %D%/packages/patches/patch-hurd-path-max.patch		\
diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 63f0e4108c..99b7ce7066 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -2240,7 +2240,9 @@ Zerofree requires the file system to be unmounted or mounted read-only.")
                                  "/strace-" version ".tar.xz"))
              (sha256
               (base32
-               "0mmns22bjjvakxj29si0x4dcylcgy26llpcimkb0llcxif439k2s"))))
+               "0mmns22bjjvakxj29si0x4dcylcgy26llpcimkb0llcxif439k2s"))
+             (patches
+              (search-patches "strace-fix-readlink-readlinkat-tests.patch"))))
     (build-system gnu-build-system)
     (arguments
      '(#:phases
@@ -2253,10 +2255,6 @@ Zerofree requires the file system to be unmounted or mounted read-only.")
          (add-after 'unpack 'disable-failing-tests
            (lambda _
              (substitute* "tests/Makefile.in"
-               ;; XXX: This test fails because an extra readlink call is made
-               ;; by the glibc when using the ld.so cache.
-               (("readlink.gen.test[^:]") " ")
-
                ;; XXX: These hang forever even if the test time-out is
                ;; extended.
                (("^\tstrace-DD?D?\\.test \\\\.*") "")
diff --git a/gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch b/gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch
new file mode 100644
index 0000000000..dd5ee98703
--- /dev/null
+++ b/gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch
@@ -0,0 +1,46 @@
+Prevent strace's "readlink" and "readlinkat" tests from failing due to the
+additional system call made by glibc with the patch "glibc-dl-cache.patch"
+applied (introduced in commit 52564e9).
+
+These changes cause strace to report during these tests only system calls on
+files contained in the test directory, effectively filtering out the
+additional readlink/readlinkat call on "/proc/self/exe" and allowing the tests
+to complete as normal.
+
+diff --git a/tests/gen_tests.in b/tests/gen_tests.in
+index 8b4e2e9..cc3ca63 100644
+--- a/tests/gen_tests.in
++++ b/tests/gen_tests.in
+@@ -623,8 +623,8 @@ quotactl-xfs-v	-v -e trace=quotactl
+ read-write	-a15 -eread=0,5 -ewrite=1,4 -e trace=read,write -P read-write-tmpfile -P /dev/zero -P /dev/null
+ readahead	-a1
+ readdir	-a16
+-readlink	-xx
+-readlinkat	-xx
++readlink	-xx --trace-path=test.readlink.link
++readlinkat	-xx --trace-path=test.readlinkat.link
+ reboot		-s 256
+ recv-MSG_TRUNC	-a26 -e trace=recv
+ recvfrom	-a35
+diff --git a/tests/readlink.gen.test b/tests/readlink.gen.test
+index 4263234..418691b 100755
+--- a/tests/readlink.gen.test
++++ b/tests/readlink.gen.test
+@@ -1,4 +1,4 @@
+ #!/bin/sh -efu
+-# Generated by ./tests/gen_tests.sh from ./tests/gen_tests.in (readlink -xx ); do not edit.
++# Generated by ./tests/gen_tests.sh from ./tests/gen_tests.in (readlink -xx --trace-path=test.readlink.link); do not edit.
+ . "${srcdir=.}/init.sh"
+-run_strace_match_diff -xx 
++run_strace_match_diff -xx --trace-path=test.readlink.link
+diff --git a/tests/readlinkat.gen.test b/tests/readlinkat.gen.test
+index d7de993..a48d590 100755
+--- a/tests/readlinkat.gen.test
++++ b/tests/readlinkat.gen.test
+@@ -1,4 +1,4 @@
+ #!/bin/sh -efu
+-# Generated by ./tests/gen_tests.sh from ./tests/gen_tests.in (readlinkat -xx ); do not edit.
++# Generated by ./tests/gen_tests.sh from ./tests/gen_tests.in (readlinkat -xx --trace-path=test.readlinkat.link); do not edit.
+ . "${srcdir=.}/init.sh"
+-run_strace_match_diff -xx 
++run_strace_match_diff -xx --trace-path=test.readlinkat.link
-- 
2.25.2

bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64

[Ludovic Courtès]

Hi Simon,

Simon South <simon@simonsouth.net> skribis:

> Modify the invocation of strace's "readlink" and "readlinkat" tests to prevent
> them from failing due to an additional system call made by Guix's patched
> version of glibc.
>
> * gnu/packages/linux.scm (strace)[source]: Add patch.
> [arguments]<#:phases>: Do not disable the "readlink" test now that it can
> succeed.
> * gnu/packages/patches/strace-fix-readlink-readlinkat-tests.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.

Good catch!  Pushed with a shortened patch file name as
b0eaa4f2d73cd7746a41d1f970b95243f2098458.

Thanks,
Ludo’.