bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64
Hi Simon, Simon South 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’.
bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64
Simon South 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 si...@simonsouth.net
bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64
Bengt Richter 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 si...@simonsouth.net
bug#50346: core-updates-frozen: strace 5.13 fails "make check" on AArch64
On +2021-09-03 10:00:33 -0400, Simon South wrote: > Bengt Richter 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 > si...@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
Bengt Richter 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 si...@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
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
> si...@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
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
si...@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
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",
0xf7e2cfea, 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",
0xf7e2cfea, 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
si...@simonsouth.net
