BFD unhappy about a shared library build with `-z pack-relative-relocs`

[marxin]

Started with 14fdad2 where my gentoo docker testing stopped working for ncurses package (/usr/lib64/libreadline.so.8.3 comes from the package). If I build the package, and then try using it with BFD, it complains with:

$ cat /tmp/tinfo.c
#include <stdio.h>
#include <term.h>
#include <curses.h>

int main(void) {
    int err = 0;

    if (setupterm(NULL, 1, &err) != OK) {
        fprintf(stderr, "setupterm failed, err=%d\n", err);
        return 1;
    }

    int cols = tigetnum("cols");
    printf("Terminal columns: %d\n", cols);

    return 0;
}
$ cp /usr/sbin/*-pc-linux-gnu-ld.bfd /usr/sbin/ld && gcc /tmp/tinfo.c -ltinfo && ./a.out
/usr/x86_64-pc-linux-gnu/binutils-bin/2.46.0/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/15/../../../../lib64/libtinfo.so: undefined reference to `GLIBC_ABI_DT_RELR@GLIBC_ABI_DT_RELR'
collect2: error: ld returned 1 exit status
$ readelf -sW /usr/lib/gcc/x86_64-pc-linux-gnu/15/../../../../lib64/libtinfo.so | grep ABI_DT
    24: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  UND GLIBC_ABI_DT_RELR@GLIBC_ABI_DT_RELR (12)

while the same snippet runs correctly if linked by Wild:

$ cargo b -r && cp target/release/wild /usr/sbin/ld
$ gcc /tmp/tinfo.c -ltinfo && ./a.out
Terminal columns: 163

@mati865 can you please take a look? Do you have enough information for the reproducer?

[mati865]

Guess this answers @davidlattimore question from #1701 (comment).

FYI, Gentoo and Arch Linux add -z pack-relative-relocs to the default linker flags when building packages.

I can look tomorrow to replace the mechanism that imports the symbol, with similar one as I previously made to only import the version.

[davidlattimore]

I recall that your original mechanism involved special handling of GLIBC_ABI_DT_RELR in both layout and elf_writer. It'd be good if we could make more use the existing symbol version handling - i.e. change only layout to mark that symbol version as required.

Alternatively, it might work to just mark the undefined reference as weak. Out of curiosity, does liibtinfo.so have a DT_NEEDED entry for libc.so and if it does, does libc.so not define that symbol? It's kind of surprising that we'd emit the undefined symbol without it being resolved at link time.

[davidlattimore]

I wanted to get a better understanding of this, so I wrote a test for it - #1819. Feel free to merge the PR if you want to go that way for testing, or disregard if you've got your own plan for testing.

[marxin]

Out of curiosity, does liibtinfo.so have a DT_NEEDED entry for libc.so and if it does, does libc.so not define that symbol? It's kind of surprising that we'd emit the undefined symbol without it being resolved at link time.

# readelf -d /usr/lib/gcc/x86_64-pc-linux-gnu/15/../../../../lib64/libtinfo.so | grep NEEDED
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
# readelf -sW /usr/lib64/libc.so.6 | grep DT_RELR
   844: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_ABI_DT_RELR

[mati865]

Marking it as weak could be a good idea. I haven't yet looked into it, so you can give it a go.

[mati865]

Marking the symbol weak via this hack works:

Subject: [PATCH] hack: try weak symbol
---
Index: libwild/src/elf_writer.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/libwild/src/elf_writer.rs b/libwild/src/elf_writer.rs
--- a/libwild/src/elf_writer.rs	(revision 3dd0a22d8f59bc9307f6dd5067b08cdf95dad87d)
+++ b/libwild/src/elf_writer.rs	(revision 471a3132ecc1aa65436e15c48a6460f55b3d2d7c)
@@ -5125,7 +5125,15 @@
                 // Note, we copy st_info, but not st_other since we don't want to copy the
                 // visibility. We want to emit the symbol with default visibility, otherwise the
                 // runtime loader may ignore dynamic relocations that reference the symbol.
-                entry.st_info = symbol.st_info();
+                if layout
+                    .symbol_db
+                    .symbol_name(symbol_id)
+                    .is_ok_and(|v| v.bytes() == b"GLIBC_ABI_DT_RELR")
+                {
+                    entry.set_st_info(object::elf::STB_WEAK, symbol.st_type());
+                } else {
+                    entry.st_info = symbol.st_info();
+                }
 
                 if let Some(versym) = table_writer.version_writer.versym.as_mut() {
                     copy_symbol_version(
Index: wild/tests/sources/elf/pack-relative-relocs-shared/pack-relative-relocs-shared.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/wild/tests/sources/elf/pack-relative-relocs-shared/pack-relative-relocs-shared.c b/wild/tests/sources/elf/pack-relative-relocs-shared/pack-relative-relocs-shared.c
--- a/wild/tests/sources/elf/pack-relative-relocs-shared/pack-relative-relocs-shared.c	(revision 3dd0a22d8f59bc9307f6dd5067b08cdf95dad87d)
+++ b/wild/tests/sources/elf/pack-relative-relocs-shared/pack-relative-relocs-shared.c	(revision 471a3132ecc1aa65436e15c48a6460f55b3d2d7c)
@@ -4,8 +4,10 @@
 //#LinkArgs:-Wl,-z,now,-z,pack-relative-relocs
 //#Shared:pack-relative-relocs-shared-1.c
 //#DiffIgnore:section.rodata
-// TODO: Enable linking with ld when fixing #1817
+//#DiffIgnore:section.got.plt.entsize
+//#DiffIgnore:section.gnu.version_r.alignment
 //#SkipLinker:ld
+//#EnableLinker:lld
 
 int foo(void);

I haven't yet figured out the way to do it cleanly.

[davidlattimore]

Yeah, it doesn't look like there's currently any especially clean way to make it weak.

If we're linking against a shared object that defines a global symbol and all references to that global symbol are weak, then the undefined symbol we produce is actually supposed to be weak, even though shared object defined the symbol as global. That's not something we currently do. If we fix that, then that'd likely make it easier to make the reference to GLIBC_ABI_DT_RELR weak. I guess in the short term, it's probably easier to find a way to force-reference the version rather than the symbol. That also has the advantage of matching what GNU ld does.

[mati865]

Fixing that underlying issue without very broad changes would probably require a new value flag, akin to DOWNGRADE_TO_LOCAL. Which wouldn't be a particularly nice solution.

[davidlattimore]

Yeah, we'd almost certainly need a new ValueFlag. I agree that wouldn't be a nice solution if it were used just for the purpose of making the GLIBC_ABI_DT_RELR ref weak, but I think it'd be a fine solution for solving the more general problem that dynamic symbols with only weak refs should be weak.

[mati865]

Weak handling appears to be a broader issue than "just" not emitting weak symbol if all references are weak.
If I remove out.entry.is_weak = false; from asm_diff.rs and run the tests, among the failures there is shared-priority where the situation is opposite. We have a strong reference to a weak symbol in SO and Wild emits output with weak reference:

❯ readelf -WDs wild/tests/build/elf/x86_64/shared-priority/shared-first-archive-not-loaded/shared-priority.c.*ld | rg 'File|foo'
File: wild/tests/build/elf/x86_64/shared-priority/shared-first-archive-not-loaded/shared-priority.c.ld
     2: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND foo
File: wild/tests/build/elf/x86_64/shared-priority/shared-first-archive-not-loaded/shared-priority.c.wild
     1: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND foo

The dependency:

❯ readelf -WDs wild/tests/build/elf/x86_64/shared-priority/shared-only/shared-priority-1.c.*ld.so | rg 'File|foo'
File: wild/tests/build/elf/x86_64/shared-priority/shared-only/shared-priority-1.c.ld.so
     2: 0000000000001000    11 FUNC    WEAK   PROTECTED    5 foo
File: wild/tests/build/elf/x86_64/shared-priority/shared-only/shared-priority-1.c.wild.so
     1: 0000000000001730    11 FUNC    WEAK   PROTECTED    7 foo

So, when we find candidate symbol in SO, we always just copy original st_bind. Made #1825 as dirty PoC for handling weak symbols. Probably terribly wrong but passes the tests.
I'll create an alternative version that only imports the version to quickly solve this issue.