[core-updates-frozen] Bug in binutils 1.37

[core-updates-frozen] Bug in binutils 1.37

[Guillaume Le Vaillant]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

Hi,

There's a bug in binutils 1.37, which we are using on the
core-updates-frozen branch.

It's a file descriptor leak that can lead to 'malformed archive' errors
when linking libraries. We get this problem at least when building
qtwekbit and qtwebengine. A workaround allows us to compile qtwebkit
(see [1]), but it doesn't work for qtwebengine.

The bug was discussed at [2] and upstream has a patch to fix it at [3].
However, adding this patch to our binutils rebuilds the world.
I'm currently trying to build things with the patched binutils.
If everything works, should I push this fix on core-updates-frozen, or
does someone have an idea that would lead to less rebuilds?

P.S.: The patch I'm trying is in attachment.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gnu-binutils-Fix-file-decriptor-leak.patch --]
[-- Type: text/x-patch, Size: 9651 bytes --]

From d90e95640f0c2bd3271e370516e51fac56929be7 Mon Sep 17 00:00:00 2001
From: Guillaume Le Vaillant <glv@posteo.net>
Date: Mon, 6 Sep 2021 17:32:38 +0200
Subject: [PATCH] gnu: binutils: Fix file decriptor leak.

* gnu/packages/patches/binutils-1.37-file-descriptor-leak.patch: New file.
* gnu/packages/local.mk (dist_patch_DATA): Add it.
* gnu/packages/base.scm (binutils)[source]: Use it.
---
 gnu/local.mk                                  |   1 +
 gnu/packages/base.scm                         |  17 +-
 .../binutils-1.37-file-descriptor-leak.patch  | 231 ++++++++++++++++++
 3 files changed, 241 insertions(+), 8 deletions(-)
 create mode 100644 gnu/packages/patches/binutils-1.37-file-descriptor-leak.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 8c41b5b676..5814587ef2 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -884,6 +884,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/behave-skip-a-couple-of-tests.patch	\
   %D%/packages/patches/beignet-correct-file-names.patch		\
   %D%/packages/patches/bidiv-update-fribidi.patch		\
+  %D%/packages/patches/binutils-1.37-file-descriptor-leak.patch	\
   %D%/packages/patches/binutils-boot-2.20.1a.patch		\
   %D%/packages/patches/binutils-loongson-workaround.patch	\
   %D%/packages/patches/binutils-mingw-w64-timestamp.patch	\
diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm
index b9841a5cef..f5486c6aae 100644
--- a/gnu/packages/base.scm
+++ b/gnu/packages/base.scm
@@ -510,14 +510,15 @@ change.  GNU make offers many powerful extensions over the standard utility.")
   (package
    (name "binutils")
    (version "2.37")
-   (source (origin
-            (method url-fetch)
-            (uri (string-append "mirror://gnu/binutils/binutils-"
-                                version ".tar.bz2"))
-            (sha256
-             (base32
-              "1m3b2rdfv1dmdpd0bzg1hy7i8a2qng53szc6livyi3nh6101mz37"))
-            (patches (search-patches "binutils-loongson-workaround.patch"))))
+   (source
+    (origin
+      (method url-fetch)
+      (uri (string-append "mirror://gnu/binutils/binutils-"
+                          version ".tar.bz2"))
+      (sha256
+       (base32 "1m3b2rdfv1dmdpd0bzg1hy7i8a2qng53szc6livyi3nh6101mz37"))
+      (patches (search-patches "binutils-loongson-workaround.patch"
+                               "binutils-1.37-file-descriptor-leak.patch"))))
    (build-system gnu-build-system)
 
    ;; TODO: Add dependency on zlib + those for Gold.
diff --git a/gnu/packages/patches/binutils-1.37-file-descriptor-leak.patch b/gnu/packages/patches/binutils-1.37-file-descriptor-leak.patch
new file mode 100644
index 0000000000..1fd3d3d9b7
--- /dev/null
+++ b/gnu/packages/patches/binutils-1.37-file-descriptor-leak.patch
@@ -0,0 +1,231 @@
+From 1c611b40e6bfc8029bff7696814330b5bc0ee5c0 Mon Sep 17 00:00:00 2001
+From: "H.J. Lu" <hjl.tools@gmail.com>
+Date: Mon, 26 Jul 2021 05:59:55 -0700
+Subject: [PATCH] bfd: Close the file descriptor if there is no archive fd
+
+Close the file descriptor if there is no archive plugin file descriptor
+to avoid running out of file descriptors on thin archives with many
+archive members.
+
+bfd/
+
+	PR ld/28138
+	* plugin.c (bfd_plugin_close_file_descriptor): Close the file
+	descriptor there is no archive plugin file descriptor.
+
+ld/
+
+	PR ld/28138
+	* testsuite/ld-plugin/lto.exp: Run tmpdir/pr28138 only for
+	native build.
+
+	PR ld/28138
+	* testsuite/ld-plugin/lto.exp: Run ld/28138 tests.
+	* testsuite/ld-plugin/pr28138.c: New file.
+	* testsuite/ld-plugin/pr28138-1.c: Likewise.
+	* testsuite/ld-plugin/pr28138-2.c: Likewise.
+	* testsuite/ld-plugin/pr28138-3.c: Likewise.
+	* testsuite/ld-plugin/pr28138-4.c: Likewise.
+	* testsuite/ld-plugin/pr28138-5.c: Likewise.
+	* testsuite/ld-plugin/pr28138-6.c: Likewise.
+	* testsuite/ld-plugin/pr28138-7.c: Likewise.
+
+(cherry picked from commit 5a98fb7513b559e20dfebdbaa2a471afda3b4742)
+(cherry picked from commit 7dc37e1e1209c80e0bab784df6b6bac335e836f2)
+---
+ bfd/plugin.c                       |  8 +++++++
+ ld/testsuite/ld-plugin/lto.exp     | 34 ++++++++++++++++++++++++++++++
+ ld/testsuite/ld-plugin/pr28138-1.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-2.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-3.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-4.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-5.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-6.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138-7.c |  6 ++++++
+ ld/testsuite/ld-plugin/pr28138.c   | 20 ++++++++++++++++++
+ 10 files changed, 104 insertions(+)
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-1.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-2.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-3.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-4.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-5.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-6.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138-7.c
+ create mode 100644 ld/testsuite/ld-plugin/pr28138.c
+
+diff --git a/bfd/plugin.c b/bfd/plugin.c
+index 6cfa2b66470..3bab8febe88 100644
+--- a/bfd/plugin.c
++++ b/bfd/plugin.c
+@@ -291,6 +291,14 @@ bfd_plugin_close_file_descriptor (bfd *abfd, int fd)
+ 	     && !bfd_is_thin_archive (abfd->my_archive))
+ 	abfd = abfd->my_archive;
+ 
++      /* Close the file descriptor if there is no archive plugin file
++	 descriptor.  */
++      if (abfd->archive_plugin_fd == -1)
++	{
++	  close (fd);
++	  return;
++	}
++
+       abfd->archive_plugin_fd_open_count--;
+       /* Dup the archive plugin file descriptor for later use, which
+ 	 will be closed by _bfd_archive_close_and_cleanup.  */
+diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
+index def69e43ab3..999d911ce6a 100644
+--- a/ld/testsuite/ld-plugin/lto.exp
++++ b/ld/testsuite/ld-plugin/lto.exp
+@@ -687,6 +687,40 @@ if { [is_elf_format] && [check_lto_shared_available] } {
+     }
+ }
+ 
++run_cc_link_tests [list \
++    [list \
++	"Build pr28138.a" \
++	"-T" "" \
++	{pr28138-1.c pr28138-2.c pr28138-3.c pr28138-4.c pr28138-5.c \
++	 pr28138-6.c pr28138-7.c} {} "pr28138.a" \
++    ] \
++    [list \
++	"Build pr28138.o" \
++	"" "" \
++	{pr28138.c} {} \
++    ] \
++]
++
++set exec_output [run_host_cmd "sh" \
++			      "-c \"ulimit -n 20; \
++			      $CC -Btmpdir/ld -o tmpdir/pr28138 \
++			      tmpdir/pr28138.o tmpdir/pr28138.a\""]
++set exec_output [prune_warnings $exec_output]
++if [string match "" $exec_output] then {
++    if { [isnative] } {
++	set exec_output [run_host_cmd "tmpdir/pr28138" ""]
++	if [string match "PASS" $exec_output] then {
++	    pass "PR ld/28138"
++	} else {
++	    fail "PR ld/28138"
++	}
++    } else {
++	pass "PR ld/28138"
++    }
++} else {
++    fail "PR ld/28138"
++}
++
+ set testname "Build liblto-11.a"
+ remote_file host delete "tmpdir/liblto-11.a"
+ set catch_output [run_host_cmd "$ar" "rc $plug_opt tmpdir/liblto-11.a tmpdir/lto-11a.o tmpdir/lto-11b.o tmpdir/lto-11c.o"]
+diff --git a/ld/testsuite/ld-plugin/pr28138-1.c b/ld/testsuite/ld-plugin/pr28138-1.c
+new file mode 100644
+index 00000000000..51d119e1642
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-1.c
+@@ -0,0 +1,6 @@
++extern int a0(void);
++int
++a1(void)
++{
++  return 1 + a0();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-2.c b/ld/testsuite/ld-plugin/pr28138-2.c
+new file mode 100644
+index 00000000000..1120cd797e9
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-2.c
+@@ -0,0 +1,6 @@
++extern int a1(void);
++int
++a2(void)
++{
++  return 1 + a1();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-3.c b/ld/testsuite/ld-plugin/pr28138-3.c
+new file mode 100644
+index 00000000000..ec464947ee6
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-3.c
+@@ -0,0 +1,6 @@
++extern int a2(void);
++int
++a3(void)
++{
++  return 1 + a2();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-4.c b/ld/testsuite/ld-plugin/pr28138-4.c
+new file mode 100644
+index 00000000000..475701b2c5c
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-4.c
+@@ -0,0 +1,6 @@
++extern int a3(void);
++int
++a4(void)
++{
++  return 1 + a3();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-5.c b/ld/testsuite/ld-plugin/pr28138-5.c
+new file mode 100644
+index 00000000000..e24f86c363e
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-5.c
+@@ -0,0 +1,6 @@
++extern int a4(void);
++int
++a5(void)
++{
++  return 1 + a4();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-6.c b/ld/testsuite/ld-plugin/pr28138-6.c
+new file mode 100644
+index 00000000000..b5b938bdb21
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-6.c
+@@ -0,0 +1,6 @@
++extern int a5(void);
++int
++a6(void)
++{
++  return 1 + a5();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138-7.c b/ld/testsuite/ld-plugin/pr28138-7.c
+new file mode 100644
+index 00000000000..4ef75bf0f0c
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138-7.c
+@@ -0,0 +1,6 @@
++extern int a6(void);
++int
++a7(void)
++{
++  return 1 + a6();
++}
+diff --git a/ld/testsuite/ld-plugin/pr28138.c b/ld/testsuite/ld-plugin/pr28138.c
+new file mode 100644
+index 00000000000..68252c9f382
+--- /dev/null
++++ b/ld/testsuite/ld-plugin/pr28138.c
+@@ -0,0 +1,20 @@
++#include <stdio.h>
++
++extern int a7(void);
++
++int
++a0(void)
++{
++  return 0;
++}
++
++int
++main()
++{
++  if (a7() == 7)
++    {
++      printf ("PASS\n");
++      return 0;
++    }
++  return 1;
++}
+-- 
+2.27.0
-- 
2.33.0


[-- Attachment #1.3: Type: text/plain, Size: 201 bytes --]


[1] https://issues.guix.gnu.org/50014
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=28138
[3] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1c611b40e6bfc8029bff7696814330b5bc0ee5c0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: [core-updates-frozen] Bug in binutils 1.37

[Leo Famulari]

On Mon, Sep 06, 2021 at 03:39:52PM +0000, Guillaume Le Vaillant wrote:
> There's a bug in binutils 1.37, which we are using on the
> core-updates-frozen branch.

2.37, right? :)

> It's a file descriptor leak that can lead to 'malformed archive' errors
> when linking libraries. We get this problem at least when building
> qtwekbit and qtwebengine. A workaround allows us to compile qtwebkit
> (see [1]), but it doesn't work for qtwebengine.
> 
> The bug was discussed at [2] and upstream has a patch to fix it at [3].
> However, adding this patch to our binutils rebuilds the world.
> I'm currently trying to build things with the patched binutils.
> If everything works, should I push this fix on core-updates-frozen, or
> does someone have an idea that would lead to less rebuilds?

There are a few options:

1) Apply the patch in the normal way and rebuild the world. If the
changes in the patch are limited to fixing this bug, then we can be
confident that changing binutils will not break other packages, which is
the main goal behind "freezing" the core-updates branch. Do you think
that expectation is reasonable? Otherwise, the branch is frozen except
for bug fixes; we'd like to avoid rebuilding the world but it's not a
problem if we have to.
2) Create a binutils-fixed package and only use it for qtwebkit and
qtwebengine
3) Try to work around the bug in the qtwebkit and qtwebengine packages

2 and 3 are not great because maybe the bug affects other packages in
some situations. Do you know if it manifests deterministically?

Re: [core-updates-frozen] Bug in binutils 2.37

[Guillaume Le Vaillant]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

Leo Famulari <leo@famulari.name> skribis:

> On Mon, Sep 06, 2021 at 03:39:52PM +0000, Guillaume Le Vaillant wrote:
>> There's a bug in binutils 1.37, which we are using on the
>> core-updates-frozen branch.
>
> 2.37, right? :)
>

Indeed. :)


>> It's a file descriptor leak that can lead to 'malformed archive' errors
>> when linking libraries. We get this problem at least when building
>> qtwekbit and qtwebengine. A workaround allows us to compile qtwebkit
>> (see [1]), but it doesn't work for qtwebengine.
>> 
>> The bug was discussed at [2] and upstream has a patch to fix it at [3].
>> However, adding this patch to our binutils rebuilds the world.
>> I'm currently trying to build things with the patched binutils.
>> If everything works, should I push this fix on core-updates-frozen, or
>> does someone have an idea that would lead to less rebuilds?
>
> There are a few options:
>
> 1) Apply the patch in the normal way and rebuild the world. If the
> changes in the patch are limited to fixing this bug, then we can be
> confident that changing binutils will not break other packages, which is
> the main goal behind "freezing" the core-updates branch. Do you think
> that expectation is reasonable? Otherwise, the branch is frozen except
> for bug fixes; we'd like to avoid rebuilding the world but it's not a
> problem if we have to.
> 2) Create a binutils-fixed package and only use it for qtwebkit and
> qtwebengine
> 3) Try to work around the bug in the qtwebkit and qtwebengine packages
>
> 2 and 3 are not great because maybe the bug affects other packages in
> some situations. Do you know if it manifests deterministically?

The problem happens when linking a library with a lot of members (I
don't know exactly how many), which is the case at least for qtwebkit
and qtwebengine. Users creating such libraries in their projects will
also have the problem.

Increasing the maximum number of open file descriptors seems not be
a very robust workaround. Setting it at 4096 for qtwebkit works,
but I tried 4096, 8192 and 16384 for qtwebengine and it didn't work.

I have built many packages with the patched binutils and I didn't get
any new failure so far. I'm not yet at qtwebkit and qtwebengine though
(compiling the 20 versions of rust took a while).
So when I get there, and if the patch really solves the issue, pushing
it looks like the best solution.

Do you know if there are other world-rebuilding pending fixes that could
go in at the same time?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

Re: [core-updates-frozen] Bug in binutils 2.37

[Guillaume Le Vaillant]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

Guillaume Le Vaillant <glv@posteo.net> skribis:

> Leo Famulari <leo@famulari.name> skribis:
>
>> On Mon, Sep 06, 2021 at 03:39:52PM +0000, Guillaume Le Vaillant wrote:
>>> There's a bug in binutils 1.37, which we are using on the
>>> core-updates-frozen branch.
>>
>> 2.37, right? :)
>>
>
> Indeed. :)
>
>
>>> It's a file descriptor leak that can lead to 'malformed archive' errors
>>> when linking libraries. We get this problem at least when building
>>> qtwekbit and qtwebengine. A workaround allows us to compile qtwebkit
>>> (see [1]), but it doesn't work for qtwebengine.
>>> 
>>> The bug was discussed at [2] and upstream has a patch to fix it at [3].
>>> However, adding this patch to our binutils rebuilds the world.
>>> I'm currently trying to build things with the patched binutils.
>>> If everything works, should I push this fix on core-updates-frozen, or
>>> does someone have an idea that would lead to less rebuilds?
>>
>> There are a few options:
>>
>> 1) Apply the patch in the normal way and rebuild the world. If the
>> changes in the patch are limited to fixing this bug, then we can be
>> confident that changing binutils will not break other packages, which is
>> the main goal behind "freezing" the core-updates branch. Do you think
>> that expectation is reasonable? Otherwise, the branch is frozen except
>> for bug fixes; we'd like to avoid rebuilding the world but it's not a
>> problem if we have to.
>> 2) Create a binutils-fixed package and only use it for qtwebkit and
>> qtwebengine
>> 3) Try to work around the bug in the qtwebkit and qtwebengine packages
>>
>> 2 and 3 are not great because maybe the bug affects other packages in
>> some situations. Do you know if it manifests deterministically?
>
> The problem happens when linking a library with a lot of members (I
> don't know exactly how many), which is the case at least for qtwebkit
> and qtwebengine. Users creating such libraries in their projects will
> also have the problem.
>
> Increasing the maximum number of open file descriptors seems not be
> a very robust workaround. Setting it at 4096 for qtwebkit works,
> but I tried 4096, 8192 and 16384 for qtwebengine and it didn't work.
>
> I have built many packages with the patched binutils and I didn't get
> any new failure so far. I'm not yet at qtwebkit and qtwebengine though
> (compiling the 20 versions of rust took a while).
> So when I get there, and if the patch really solves the issue, pushing
> it looks like the best solution.
>
> Do you know if there are other world-rebuilding pending fixes that could
> go in at the same time?

I confirm that the patch fixed the compilation issues for qtwebkit and
qtwebengine, and it didn't cause new build failures, so I pushed it as
de8e2a699c0219f5ea86f6bbfff4d5ee35104738.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]