GNU bug report logs - #52940
[PATCH] gremlin: Mimic ld.so NEEDED deduplication behavior.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 52940 in the body.
You can then email your comments to 52940 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Chris Marusich <cmmarusich <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] gremlin: Mimic ld.so NEEDED deduplication behavior.
Date: Sat, 01 Jan 2022 15:13:43 -0800

[Message part 1 (text/plain, inline)]

Hi Guix,

I've noticed that test file-needed/recursive in tests/gremlin.scm fails
on master branch on powerpc64le-linux.  It does not fail on
x86_64-linux.  I've attached a patch that attempts to fix the issue.
The primary issue is that it does not deduplicate entries in the same
way as ld.so when there are multiple entries referring to the same
shared object.  The patch changes file-needed/recursive to behave more
like ld.so.

Here is the failing test output, including the test source:

--8<---------------cut here---------------start------------->8---
;;; (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"))
test-name: file-needed/recursive
location: /home/marusich/guix-master/tests/gremlin.scm:70
source:
+ (test-assert
+   "file-needed/recursive"
+   (let* ((needed
+            (file-needed/recursive %guile-executable))
+          (pipe (dynamic-wind
+                  (lambda ()
+                    (setenv "LD_TRACE_LOADED_OBJECTS" "yup"))
+                  (lambda ()
+                    (open-pipe* OPEN_READ %guile-executable))
+                  (lambda () (unsetenv "LD_TRACE_LOADED_OBJECTS")))))
+     (define ldd-rx
+       (make-regexp
+         "^[[:blank:]]+([[:graph:]]+ => )?([[:graph:]]+) .*$"))
+     (define (read-ldd-output port)
+       (let loop ((result '()))
+         (match (read-line port)
+                ((? eof-object?) (reverse result))
+                ((= (cut regexp-exec ldd-rx <>) m)
+                 (if m
+                   (loop (cons (match:substring m 2) result))
+                   (loop result))))))
+     (define ground-truth
+       (remove
+         (cut string-prefix? "linux-vdso.so" <>)
+         (read-ldd-output pipe)))
+     (and (zero? (close-pipe pipe))
+          (lset= string=?
+                 (pk 'truth ground-truth)
+                 (pk 'needed needed)))))
actual-value: #f
result: FAIL
--8<---------------cut here---------------end--------------->8---

For reference, here is the actual dynamic section of of
%guile-executable on this system, as reported by readelf:

--8<---------------cut here---------------start------------->8---
$ readelf -d /gnu/store/gahs2sx5snbfkr9vlcjj5c2kvnlhr0zs-guile-3.0.7/bin/guile

Dynamic section at offset 0xfc60 contains 37 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libguile-3.0.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libgc.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libpthread.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libffi.so.7]
 0x0000000000000001 (NEEDED)             Shared library: [libunistring.so.2]
 0x0000000000000001 (NEEDED)             Shared library: [libcrypt.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so.2]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000001d (RUNPATH)            Library runpath: [/gnu/store/gahs2sx5snbfkr9vlcjj5c2kvnlhr0zs-guile-3.0.7/lib:/gnu/store/7x2cjqbmpgwrgmnb234gsxkmsqs5pj09-libgc-8.0.4/lib:/gnu/store/521riv2sgv0b0s4j0kzz6i52rf9rarh8-libffi-3.3/lib/../lib:/gnu/store/xj20v8lk2wal0z1rla0yx3bjkasbx6mq-libunistring-0.9.10/lib:/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib:/gnu/store/ys7b4gr5nbq8sfnff9ry5blb4bhpx6mq-gcc-7.5.0-lib/lib:/gnu/store/sipyfs2540b48b2sb9j8ypmybja1dvqb-glibc-2.31/lib/../lib:/gnu/store/ys7b4gr5nbq8sfnff9ry5blb4bhpx6mq-gcc-7.5.0-lib/lib/gcc/powerpc64le-unknown-linux-gnu/7.5.0/../../../../lib]
 0x000000000000000c (INIT)               0x10000980
 0x000000000000000d (FINI)               0x10000ef4
 0x0000000000000019 (INIT_ARRAY)         0x1001fc50
 0x000000000000001b (INIT_ARRAYSZ)       8 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x1001fc58
 0x000000000000001c (FINI_ARRAYSZ)       8 (bytes)
 0x0000000000000004 (HASH)               0x10000268
 0x000000006ffffef5 (GNU_HASH)           0x100002c0
 0x0000000000000005 (STRTAB)             0x10000470
 0x0000000000000006 (SYMTAB)             0x100002f0
 0x000000000000000a (STRSZ)              891 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000015 (DEBUG)              0x0
 0x0000000000000003 (PLTGOT)             0x10020000
 0x0000000000000002 (PLTRELSZ)           216 (bytes)
 0x0000000000000014 (PLTREL)             RELA
 0x0000000000000017 (JMPREL)             0x10000880
 0x0000000070000000 (PPC64_GLINK)        0x10000eb0
 0x0000000070000003 (PPC64_OPT)          0x0
 0x0000000000000007 (RELA)               0x10000850
 0x0000000000000008 (RELASZ)             48 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffe (VERNEED)            0x10000810
 0x000000006fffffff (VERNEEDNUM)         2
 0x000000006ffffff0 (VERSYM)             0x100007ec
 0x0000000000000000 (NULL)               0x0
--8<---------------cut here---------------end--------------->8---

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.

Regarding (A), it seems to be an error in the test logic.  The test code
already filters out strings beginning with "linux-vdso.so" from the
"truth" list.:

  (define ground-truth
    (remove (cut string-prefix? "linux-vdso.so" <>)
            (read-ldd-output pipe)))

The intent seems to be to filter out the vdso shared object from the
"truth" list.  However, it fails to do so in this case, since the name
of the vdso shared object is actually "linux-vdso64.so.1".  My patch
fixes this by filtering out strings that begin with "linux-vdso64.so",
too.

Regarding (B), it seems to occur because ld.so deduplicates entries.  I
checked the glibc source code, but I had a hard time figuring out
exactly how exactly the deduplication works.  In any case, based on
ld.so's actual behavior, it seems that ld.so does in fact deduplicate
entries, and file-needed/recursive does not.  This explains the
difference.

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!

--
Chris

PGP: https://savannah.gnu.org/people/viewgpg.php?user_id=106836

[0001-gremlin-Mimic-ld.so-NEEDED-deduplication-behavior.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 52940 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 52940 <at> debbugs.gnu.org
Subject: Re: bug#52940: [PATCH] gremlin: Mimic ld.so NEEDED deduplication
 behavior.
Date: Wed, 05 Jan 2022 20:07:14 +0100

Hi Chris,

Chris Marusich <cmmarusich <at> 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 <at> gnu.org>
> +;;; Copyright © 2022 Chris Marusich <cmmarusich <at> 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’.

Message #13 received at 52940-close <at> debbugs.gnu.org (full text, mbox):

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 52940-close <at> debbugs.gnu.org
Subject: Re: bug#52940: [PATCH] gremlin: Mimic ld.so NEEDED deduplication
 behavior.
Date: Sat, 08 Jan 2022 18:04:11 -0800

[Message part 1 (text/plain, inline)]

Hi Ludo,

Ludovic Courtès <ludo <at> 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 <at> gnu.org>
>> +;;; Copyright © 2022 Chris Marusich <cmmarusich <at> 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.

-- 
Chris

PGP: https://savannah.gnu.org/people/viewgpg.php?user_id=106836

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.