Message ID | 87zgofaw2g.fsf@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#52940] gremlin: Mimic ld.so NEEDED deduplication behavior. | expand |
Hi Chris, Chris Marusich <cmmarusich@gmail.com> skribis: > Here is the failing test output, including the test source: > > ;;; (truth ("linux-vdso64.so.1" "/gnu/store/gahs2sx5snbfkr9vlcjj5c2kvnlhr0zs-guile-3.0.7/lib/libguile-3.0.so.1" "/gnu/store/7x2cjqbmpgwrgmnb234gsxkmsqs5pj09-libgc-8.0.4/lib/libgc.so.1" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libpthread.so.0" "/gnu/store/521riv2sgv0b0s4j0kzz6i52rf9rarh8-libffi-3.3/lib/../lib/libffi.so.7" "/gnu/store/xj20v8lk2wal0z1rla0yx3bjkasbx6mq-libunistring-0.9.10/lib/libunistring.so.2" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libcrypt.so.1" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libdl.so.2" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libm.so.6" "/gnu/store/ys7b4gr5nbq8sfnff9ry5blb4bhpx6mq-gcc-7.5.0-lib/lib/libgcc_s.so.1" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libc.so.6" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/ld64.so.2")) > > ;;; (needed ("/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libc.so.6" "/gnu/store/ys7b4gr5nbq8sfnff9ry5blb4bhpx6mq-gcc-7.5.0-lib/lib/libgcc_s.so.1" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libm.so.6" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libdl.so.2" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libcrypt.so.1" "/gnu/store/xj20v8lk2wal0z1rla0yx3bjkasbx6mq-libunistring-0.9.10/lib/libunistring.so.2" "/gnu/store/521riv2sgv0b0s4j0kzz6i52rf9rarh8-libffi-3.3/lib/../lib/libffi.so.7" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libpthread.so.0" "/gnu/store/7x2cjqbmpgwrgmnb234gsxkmsqs5pj09-libgc-8.0.4/lib/libgc.so.1" "/gnu/store/gahs2sx5snbfkr9vlcjj5c2kvnlhr0zs-guile-3.0.7/lib/libguile-3.0.so.1" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/ld64.so.2" "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/../lib/libc.so.6")) [...] > Note that the RUNPATH above contains an entry for > "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib" followed > later by > "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/../lib". It seems > that ld.so's tracing mechanism is smart enough to avoid printing the > second entry. > > So, the test fails because the "needed" list is not set-equivalent to > the "truth" list. There are two reasons why they are not > set-equivalent: > > A) "truth" contains "linux-vdso64.so.1", but "needed" does not. > > B) "needed" contains > "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/../lib/libc.so.6", > but "truth" does not. However, both contain > "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/libc.so.6", > which refers to the same file. [...] > What is a good solution for (B)? I can think of the following potential > solutions: > > 1) Try to avoid introducing multiple entries referring to the same thing > in the first place. Somehow, somewhere, something is adding the second > entry to the dynamic section of Guile's ELF file. It happens on > powerpc64le-linux but not on x86_64-linux. What code or tool is doing > this? I don't know, but I guess I would start by looking at the > gnu-build-system code. I'm not sure if it's a really problem, though, > so I'm not eager to jump down this rabbit hole just yet. > > 2) Change the test so that it passes even if file-needed/recursive > returns multiple entries referring to the same file. In other words, > accept that the current behavior is OK, even if it means that the > results returned by file-needed/recursive are not always exactly the > same as the results returned by ld.so. > > 3) Try to change file-needed/recursive so that it does not return > multiple entries referring to the same file. In other words, make it > behave more like ld.so. > > I can't think of a reason why the current behavior of > file-needed/recursive is bad, but it was simple enough to make it > deduplicate entries similarly to ld.so. So, my patch implements > solution (3). Hopefully it's good enough! Good catch! We could go fancy and have ‘loop’ in ‘file-needed/recursive’ thread a map of device/inode number pairs to file names; when calling ‘search-path’, we’d check whether the file we found already is in the set, possibly under a different name, and we’d use that name instead of introducing a new one. That’d be more efficient that calling ‘canonicalize-path’, especially O(n³) times roughly. But… given that this is a corner case, that modifying (guix build gremlin) entails a full rebuild, and that there just should be that “/lib/../lib” entry in the first place, I’d lean towards leaving gremlin.scm unchanged. WDYT? However… > diff --git a/tests/gremlin.scm b/tests/gremlin.scm > index 9af899c89a..86757e62b4 100644 > --- a/tests/gremlin.scm > +++ b/tests/gremlin.scm > @@ -1,5 +1,6 @@ > ;;; GNU Guix --- Functional package management for GNU > ;;; Copyright © 2015, 2018, 2020 Ludovic Courtès <ludo@gnu.org> > +;;; Copyright © 2022 Chris Marusich <cmmarusich@gmail.com> > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -92,7 +93,9 @@ > (loop result)))))) > > (define ground-truth > - (remove (cut string-prefix? "linux-vdso.so" <>) > + (remove (lambda (entry) > + (or (string-prefix? "linux-vdso.so" entry) > + (string-prefix? "linux-vdso64.so" entry))) > (read-ldd-output pipe))) > > (and (zero? (close-pipe pipe)) … I think this part should definitely be committed (‘master’ is fine). Thanks, Ludo’.
Hi Ludo, Ludovic Courtès <ludo@gnu.org> writes: > Good catch! We could go fancy and have ‘loop’ in > ‘file-needed/recursive’ thread a map of device/inode number pairs to > file names; when calling ‘search-path’, we’d check whether the file we > found already is in the set, possibly under a different name, and we’d > use that name instead of introducing a new one. That’d be more > efficient that calling ‘canonicalize-path’, especially O(n³) times > roughly. > > But… given that this is a corner case, that modifying (guix build > gremlin) entails a full rebuild, and that there just should be that > “/lib/../lib” entry in the first place, I’d lean towards leaving > gremlin.scm unchanged. > > WDYT? > > However… > >> diff --git a/tests/gremlin.scm b/tests/gremlin.scm >> index 9af899c89a..86757e62b4 100644 >> --- a/tests/gremlin.scm >> +++ b/tests/gremlin.scm >> @@ -1,5 +1,6 @@ >> ;;; GNU Guix --- Functional package management for GNU >> ;;; Copyright © 2015, 2018, 2020 Ludovic Courtès <ludo@gnu.org> >> +;;; Copyright © 2022 Chris Marusich <cmmarusich@gmail.com> >> ;;; >> ;;; This file is part of GNU Guix. >> ;;; >> @@ -92,7 +93,9 @@ >> (loop result)))))) >> >> (define ground-truth >> - (remove (cut string-prefix? "linux-vdso.so" <>) >> + (remove (lambda (entry) >> + (or (string-prefix? "linux-vdso.so" entry) >> + (string-prefix? "linux-vdso64.so" entry))) >> (read-ldd-output pipe))) >> >> (and (zero? (close-pipe pipe)) > > … I think this part should definitely be committed (‘master’ is fine). I agree that the existing behavior is probably fine. It makes me feel better to know that you also think so. With that in mind, I've committed a simpler fix in 6a2050b on master that just changes the test. I've also updated the guix package two times. Now, after running "guix pull", I can once again successfully build the guix package on powerpc64le-linux. Hooray! Thank you for the review! I'll now close this bug.
From 67365d79afc7182aefbacf360941f338aea712b6 Mon Sep 17 00:00:00 2001 From: Chris Marusich <cmmarusich@gmail.com> Date: Sat, 1 Jan 2022 14:17:38 -0800 Subject: [PATCH] gremlin: Mimic ld.so NEEDED deduplication behavior. Together, these two changes fix the file-needed/recursive test, which was failing on powerpc64le-linux. It was not failing on x86_64-linux. The test failure on powerpc64le-linux was caused by two issues. First, file-needed/recursive did not deduplicate entries in the same way as ld.so. The %guile-executable ELF file contains in its RUNPATH both "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib" and "/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/../lib". Although ld.so deduplicates the second entry, file-needed/recursive did not. Second, the vdso shared library name is "linux-vdso64.so.1", but the test incorrectly assumed that the vdso shared library would always begin with "linux-vdso.so". * guix/build/gremlin.scm (file-needed/recursive)[contains-canonical-file?]: New procedure. Use it to deduplicate entries that refer to the same file. * tests/gremlin.scm (file-needed/recursive)[ground-truth]: In addition to strings that begin with "linux-vdso.so", remove strings that begin with "linux-vdso64.so". --- guix/build/gremlin.scm | 12 +++++++++++- tests/gremlin.scm | 5 ++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/guix/build/gremlin.scm b/guix/build/gremlin.scm index 2a74d51dd9..e90e59679b 100644 --- a/guix/build/gremlin.scm +++ b/guix/build/gremlin.scm @@ -1,5 +1,6 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2015, 2018, 2020 Ludovic Courtès <ludo@gnu.org> +;;; Copyright © 2022 Chris Marusich <cmmarusich@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -268,6 +269,10 @@ recursively, and the list of .so file names that could not be found. File names are resolved by searching the RUNPATH of the file that NEEDs them. This is similar to the info returned by the 'ldd' command." + (define (contains-canonical-file? file files) + (any (lambda (entry) + (string=? (canonicalize-path entry) (canonicalize-path file))) + files)) (let loop ((files (list file)) (result '()) (not-found '())) @@ -292,10 +297,15 @@ This is similar to the info returned by the 'ldd' command." (not (libc-library? needed)) needed)) needed resolved)) + ;; Deduplicate entries that refer to the same file. + ;; The actual ld.so tracing behavior is similar and + ;; will de-duplicate entries even if they have + ;; different names but refer to the same file. (needed (remove (lambda (value) (or (not value) ;; XXX: quadratic - (member value result))) + (contains-canonical-file? + value result))) resolved))) (loop (append rest needed) (append needed result) diff --git a/tests/gremlin.scm b/tests/gremlin.scm index 9af899c89a..86757e62b4 100644 --- a/tests/gremlin.scm +++ b/tests/gremlin.scm @@ -1,5 +1,6 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2015, 2018, 2020 Ludovic Courtès <ludo@gnu.org> +;;; Copyright © 2022 Chris Marusich <cmmarusich@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -92,7 +93,9 @@ (loop result)))))) (define ground-truth - (remove (cut string-prefix? "linux-vdso.so" <>) + (remove (lambda (entry) + (or (string-prefix? "linux-vdso.so" entry) + (string-prefix? "linux-vdso64.so" entry))) (read-ldd-output pipe))) (and (zero? (close-pipe pipe)) -- 2.26.3