GNU bug report logs - #47921
[PATCH] build: Fix elf-dynamic-info-soname.

Previous Next

Package: guix-patches;

Reported by: Dion Mendel <guix <at> dm9.info>

Date: Tue, 20 Apr 2021 20:57:02 UTC

Severity: normal

Tags: patch

Done: Ludovic Courtès <ludo <at> gnu.org>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Tue, 20 Apr 2021 20:57:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Dion Mendel <guix <at> dm9.info>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Tue, 20 Apr 2021 20:57:02 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: guix-patches <at> gnu.org
Subject: [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 03:44:29 +0800
* guix/build/gremlin.scm (elf-dynamic-info-soname): Return the value of
 the dynamic-entry instead of the dynamic-entry record itself.
---
guix/build/gremlin.scm | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/guix/build/gremlin.scm b/guix/build/gremlin.scm
index e8ea66dfb3..4a31c3cfaf 100644
--- a/guix/build/gremlin.scm
+++ b/guix/build/gremlin.scm
@@ -215,7 +215,10 @@ string table if the type is a string."
    (#f #f)
    ((? elf-segment? dynamic)
     (let ((entries (dynamic-entries elf dynamic)))
-       (%elf-dynamic-info (find (matching-entry DT_SONAME) entries)
+       (%elf-dynamic-info (or (and=> (find (matching-entry DT_SONAME)
+                                           entries)
+                                     dynamic-entry-value)
+                              #f)
                          (filter-map (lambda (entry)
                                        (and (= (dynamic-entry-type entry)
                                                DT_NEEDED)
-- 
2.31.0




Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Wed, 21 Apr 2021 02:47:01 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: 47921 <at> debbugs.gnu.org
Subject: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 10:46:50 +0800
[Message part 1 (text/plain, inline)]
Oops, forgot to refactor.  Replacement patch attached.

Rationale:

The elf-dynamic-info-soname function is currently implemented but 
unused.  The current implementation does not work as it omits unwrapping 
the dynamic-entry record.  This patch makes the function work as 
intended.
[0001-build-Fix-elf-dynamic-info-soname.patch (text/x-diff, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Wed, 21 Apr 2021 11:50:01 GMT) Full text and rfc822 format available.

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: Dion Mendel <guix <at> dm9.info>, 47921 <at> debbugs.gnu.org
Subject: Re: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 13:49:33 +0200
[Message part 1 (text/plain, inline)]
Dion Mendel schreef op wo 21-04-2021 om 03:44 [+0800]:
> * guix/build/gremlin.scm (elf-dynamic-info-soname): Return the value of
>   the dynamic-entry instead of the dynamic-entry record itself.

1. IIUC, this change would cause a world-rebuild, so this patch would have
   to applied on core-updates.  The subject line should have been
   [PATCH core-updates]: etcetera.
2. How did you test this patch?
3. What does this patch fix?
4. elf-dynamic-info-soname is a record accessor.  Did you mean elf-dynamic-info?
5. According to the docstring (core-updates, c9a61dff8242612ae8275829a5ee31ff45ff08b1):

  "Return dynamic-link information for ELF as an <elf-dynamic-info> object, or
#f if ELF lacks dynamic-link information."

  So this patch actually _introduces_ a bug.  Or you need to modify the docstring
  as well.

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

Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Wed, 21 Apr 2021 15:47:02 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: 47921 <at> debbugs.gnu.org
Subject: Re: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 23:46:12 +0800
Thanks for the follow up.

>1. IIUC, this change would cause a world-rebuild, so this patch would 
>have to applied on core-updates.  The subject line should have been
>[PATCH core-updates]: etcetera.

Are you sure?  This patch will modify guix.

  guix graph --type=reverse-package guix

Only shows a few packages to be rebuilt.  I am new to guix so I may be 
wrong about this.

>2. How did you test this patch?

Tested in the repl.

Current behaviour:

   (elf-dynamic-info-soname
     (call-with-input-file "/path/to/libz.so.1.2.11"
                           (compose elf-dynamic-info parse-elf
                                    get-bytevector-all)))
   => #<<dynamic-entry> type: 14 value: "libz.so.1" offset: 5764>

There is no way to extract the value as dynamic-entry is private to the 
module.

After the patch:

   (elf-dynamic-info-soname
     (call-with-input-file "/path/to/libz.so.1.2.11"
                           (compose elf-dynamic-info parse-elf
                                    get-bytevector-all)))
   => "libz.so.1"

>3. What does this patch fix?

Module (guix build gremlin) exports several functions to extract 
information from the dynamic section of an elf file.

elf-dynamic-info-soname is one of these functions.  It is not called 
anywhere in guix.  I would like to use it for packaging, but it is 
currently non functioning.

>4. elf-dynamic-info-soname is a record accessor.  Did you mean 
>elf-dynamic-info?

No, I do not mean elf-dynamic-info.

elf-dynamic-info-soname is a record accessor which is currently broken 
because it doesn't unwrap an internal structure, namely <dynamic-entry>.  
All the other accessors unwrap this internal structure.

This patch brings this accessor into line with the others.

>5. According to the docstring (core-updates, c9a61dff8242612ae8275829a5ee31ff45ff08b1):
>
>  "Return dynamic-link information for ELF as an <elf-dynamic-info> object, or
>#f if ELF lacks dynamic-link information."
>
>  So this patch actually _introduces_ a bug.  Or you need to modify the docstring
>  as well.

No.  This patch does not affect elf-dynamic-info.  It fixes one of its 
accessors.  Nothing else is affected.




Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Wed, 21 Apr 2021 18:35:02 GMT) Full text and rfc822 format available.

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: Dion Mendel <guix <at> dm9.info>, 47921 <at> debbugs.gnu.org
Subject: Re: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 20:33:57 +0200
[Message part 1 (text/plain, inline)]
Dion Mendel schreef op wo 21-04-2021 om 23:46 [+0800]:
> Thanks for the follow up.
> 
> > 1. IIUC, this change would cause a world-rebuild, so this patch would 
> > have to applied on core-updates.  The subject line should have been
> > [PATCH core-updates]: etcetera.
> 
> Are you sure?  This patch will modify guix.
> 
>    guix graph --type=reverse-package guix
>
> Only shows a few packages to be rebuilt.  I am new to guix so I may be 
> wrong about this.

If this patch modified something in, say, guix/scripts or gnu/packages,
targetting "master" should be perfectly fine.  But this patch modifies
something under guix/build.  Modules under guix/build can be used from
the build environment, that is, from within the compilation process of
a package.  By modifying guix/build/gremlin.scm, a widely-used module
in package definitions (used indirectly via gnu-build-system IIRC),
practically all packages would be rebuilt.

Unless I'm severely mistaken, you can see this for yourself by:

0. start from a checkout of guix *without* your patch
1. # you probably have its dependencies already, but let's make sure
   ./pre-inst-env guix build hello
2. apply your patch to your local checkout of guix
3. make
4. ./pre-inst-env guix build hello
   # This will probably rebuild GCC, binutils, etc.!

> 2. How did you test this patch?
> 
> Tested in the repl.
> 
> Current behaviour:
> 
>     (elf-dynamic-info-soname
>       (call-with-input-file "/path/to/libz.so.1.2.11"
>                             (compose elf-dynamic-info parse-elf
>                                      get-bytevector-all)))
>     => #<<dynamic-entry> type: 14 value: "libz.so.1" offset: 5764>
> 
> There is no way to extract the value as dynamic-entry is private to the 
> module.
> 
> After the patch:
> 
>     (elf-dynamic-info-soname
>       (call-with-input-file "/path/to/libz.so.1.2.11"
>                             (compose elf-dynamic-info parse-elf
>                                      get-bytevector-all)))
>     => "libz.so.1"

So you didn't try to build any package with this patch?

I would recommend to make sure this patch doesn't break any packages.
While you can't test all packages (unless you have a *really big* build
farm), I would at least suggest running "./pre-inst-env guix build hello".
If that takes too long to complete, don't worry, just interrupt it and let
us now you couldn't test it completely.

To make sure this new functionality does not break in the future, please
write an unit test (in tests/gremlin.scm).

> > 3. What does this patch fix?
> 
> Module (guix build gremlin) exports several functions to extract 
> information from the dynamic section of an elf file.
> 
> elf-dynamic-info-soname is one of these functions.  It is not called 
> anywhere in guix.  I would like to use it for packaging, but it is 
> currently non functioning.

Your patch doesn't modify elf-dynamic-info-soname.  It modifies elf-dynamic-info.

> > 4. elf-dynamic-info-soname is a record accessor.  Did you mean 
> > elf-dynamic-info?

In case I wasn't clear, I was referring to the commit message.  In the commit
message, you say you modified elf-dynamic-info-soname, but you actually modified
elf-dynamic-info.
 
> No, I do not mean elf-dynamic-info.
See two comments above.

> elf-dynamic-info-soname is a record accessor which is currently broken 
Record accessors are correct by construction.  Perhaps you meant
that the "soname" field is initialised incorrectly by elf-dynamic-info?

> because it doesn't unwrap an internal structure, namely <dynamic-entry>.  
> All the other accessors unwrap this internal structure.
I think you can predict my response about accessors here (-:.

I'll interpret this as ‘in all other fields, the internal structure is unwrapped’.
That's a good point!  Your patch seems good to me, but the commit message doesn't
and it has a lack of testing.

> This patch brings this accessor into line with the others.

You didn't modify the elf-dynamic-info-soname, you modified elf-dynamic-info.
See comments above.

> > 5. According to the docstring (core-updates, c9a61dff8242612ae8275829a5ee31ff45ff08b1):
> > 
> >  "Return dynamic-link information for ELF as an <elf-dynamic-info> object, or
> >  #f if ELF lacks dynamic-link information."
> > 
> >  So this patch actually _introduces_ a bug.  Or you need to modify the docstring
> >  as well.

On second thought, the (revised) patch actually seems correct, and my comment here
doesn't make much sense.

> No.  This patch does not affect elf-dynamic-info.  It fixes one of its 
> accessors.

You modified elf-dynamic-info, so this patch does affect it.
elf-dynamic-info is a procedure, so it cannot have any accessors (unless you play
weird tricks with Guile's undocumented ‘applicable structs’).  The record is
<elf-dynamic-info>. <elf-dynamic-info> != elf-dynamic-info.

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

Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Thu, 22 Apr 2021 02:56:01 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: 47921 <at> debbugs.gnu.org
Subject: retitle
Date: Thu, 22 Apr 2021 10:55:24 +0800
retitle 47921 [PATCH core-updates] build: Fix elf-dynamic-info-soname.




Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Thu, 22 Apr 2021 03:14:02 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: 47921 <at> debbugs.gnu.org
Subject: Re: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Thu, 22 Apr 2021 11:13:50 +0800
[Message part 1 (text/plain, inline)]
Thanks for your explanations.  It's cleared up my understanding.

I've tried to retitled to target core-updates, but I'm not sure if it 
applied.

I've updated the patch to include a unit test.

I am currently testing by rebuilding as you suggest.

  ./pre-inst-env guix build hello

Yes this is building the world so I will report back when it completes.

>The record is <elf-dynamic-info>. <elf-dynamic-info> != elf-dynamic-info.

Thank you.  Yes I was confused regarding this.  I've updated the commit 
changelog accordingly.
[0001-guix-build-gremlin.scm-elf-dynamic-info-Correctly-se.patch (text/x-diff, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#47921; Package guix-patches. (Thu, 22 Apr 2021 03:27:02 GMT) Full text and rfc822 format available.

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

From: Dion Mendel <guix <at> dm9.info>
To: 47921 <at> debbugs.gnu.org
Subject: Include correct commit message
Date: Thu, 22 Apr 2021 11:26:05 +0800
[Message part 1 (text/plain, inline)]
Correct changelog message.
[0001-build-Fix-elf-dynamic-info-soname.patch (text/x-diff, attachment)]

Reply sent to Ludovic Courtès <ludo <at> gnu.org>:
You have taken responsibility. (Thu, 22 Apr 2021 08:21:02 GMT) Full text and rfc822 format available.

Notification sent to Dion Mendel <guix <at> dm9.info>:
bug acknowledged by developer. (Thu, 22 Apr 2021 08:21:02 GMT) Full text and rfc822 format available.

Message #31 received at 47921-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Dion Mendel <guix <at> dm9.info>
Cc: 47921-done <at> debbugs.gnu.org
Subject: Re: bug#47921: [PATCH] build: Fix elf-dynamic-info-soname.
Date: Thu, 22 Apr 2021 10:20:16 +0200
Hi Dion,

Dion Mendel <guix <at> dm9.info> skribis:

>>From bfc69ea726e0f5c1955e629e92af377ffb90c2c5 Mon Sep 17 00:00:00 2001
> From: Dion Mendel <guix <at> dm9.info>
> Date: Thu, 22 Apr 2021 10:32:35 +0800
> Subject: [PATCH] * guix/build/gremlin.scm (elf-dynamic-info): Correctly set
>  the value of   soname in <elf-dynamic-info>.
>
> ---
>  guix/build/gremlin.scm |  4 +++-
>  tests/gremlin.scm      | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)

Awesome.  I tweaked the commit message and applied it.

Thank you, and thanks Maxime for the review!

Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 20 May 2021 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 314 days ago.

Previous Next


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