This is an archive of the discontinued LLVM Phabricator instance.

lld: elf: discard more specific .gnu.linkonce section
ClosedPublic

Authored by nickdesaulniers on Jan 26 2019, 1:43 PM.

Details

Summary

lld discards .gnu.linonce.* sections work around a bug in glibc.
https://sourceware.org/bugzilla/show_bug.cgi?id=20543

Unfortunately, the Linux kernel uses a section named
.gnu.linkonce.this_module to store infomation about kernel modules. The
kernel reads data from this section when loading kernel modules, and
errors if it fails to find this section. The current behavior of lld
discards this section when kernel modules are linked, so kernel modules
linked with lld are unloadable by the linux kernel.

The Linux kernel should use a comdat section instead of .gnu.linkonce.
The minimum version of binutils supported by the kernel supports comdat
sections. The kernel is also not relying on the old linkonce behavior;
it seems to have chosen a name that contains a deprecated GNU feature.

Changing the section name now in the kernel would require all kernel
modules to be recompiled to make use of the new section name. Instead,
rather than discarding .gnu.linkonce.*, let's discard the more specific
section name to continue working around the glibc issue while supporting
linking Linux kernel modules.

Link: https://github.com/ClangBuiltLinux/linux/issues/329

Diff Detail

Repository
rL LLVM

Event Timeline

nickdesaulniers marked an inline comment as done.Jan 26 2019, 1:50 PM
nickdesaulniers added inline comments.
lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)
pcc added a comment.Jan 26 2019, 3:05 PM

This needs a test. I imagine that the test lld/test/ELF/comdat-linkonce.s needs to be adjusted for the new behaviour. If you would like to test the Linux kernel behaviour, please create a new test.

lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)

At least on my machine, the only instances of this present in object files and static libraries shipped with glibc involve bx, and when dynamically linking the only problematic definition is in crti.o. As far as I can tell this file has always either used bx or hasn't had the problem at all; Android's glibc2.17 sysroot uses bx, and its glibc2.15 sysroot doesn't have the definition at all. You can check the history of sysdeps/i386/crti.S in glibc to see that it has always used bx.

There's another problem, which is that __x86 used to be __i686 until 2015 or so in some cases: https://github.com/bminor/glibc/commit/9f9f27248bf464b465fd4f05112a5b479503e83a
And it doesn't seem too unreasonable for folks to want to link using a sysroot with an old glibc to ensure that their binaries are compatible with at least that version of glibc.

So we might need to check both names. (Sigh.)

nickdesaulniers marked an inline comment as done.Jan 26 2019, 3:31 PM
nickdesaulniers added inline comments.
lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)

Yes; we should check for both __X86 and __i686. The macro looks like it can use any register, so probably should be using startswith rather than strict equality?
https://github.com/bminor/glibc/commit/9f9f27248bf464b465fd4f05112a5b479503e83a#diff-a36a1edd47385f5b0060d7430013106fR33

pcc added inline comments.Jan 26 2019, 3:42 PM
lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)

Although the macro can take any register, in lld we only need to be able to handle the uses that appear in the object files that are actually read when linking an executable, which in this case is only bx.

nickdesaulniers marked an inline comment as done.Jan 26 2019, 4:36 PM
nickdesaulniers added inline comments.
lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)

is this a job for std::regex_match? (Would we like to have two bugs in place of one (as the joke goes)? :-P)

nickdesaulniers marked an inline comment as not done.Jan 26 2019, 4:47 PM
pcc added inline comments.Jan 26 2019, 5:49 PM
lld/ELF/InputFiles.cpp
738 ↗(On Diff #183728)

I mean that I think that this just needs to be Name == ".gnu.linkonce.t.__x86.get_pc_thunk.bx" || Name == ".gnu.linkonce.t.__i686.get_pc_thunk.bx".

  • only check .bx
nickdesaulniers marked an inline comment as done.Jan 26 2019, 6:41 PM
  • actually check .bx
  • add back __i686 test case
pcc accepted this revision.Jan 26 2019, 6:44 PM

LGTM

lld/test/ELF/comdat-linkonce.s
7 ↗(On Diff #183739)

Maybe test the __i686 one here as well?

This revision is now accepted and ready to land.Jan 26 2019, 6:44 PM
pcc added inline comments.Jan 26 2019, 6:46 PM
lld/test/ELF/comdat-linkonce.s
14 ↗(On Diff #183741)

Does the assembler accept this? (defining symbol twice)

nickdesaulniers marked 2 inline comments as done.Jan 26 2019, 6:46 PM

Thanks for code review!

lld/test/ELF/comdat-linkonce.s
7 ↗(On Diff #183739)

yes; sorry, accidentally removed it. It's back now.

nickdesaulniers marked an inline comment as done.
  • fix unit test I should not have removed
This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Jan 26 2019, 6:57 PM
lld/trunk/test/ELF/comdat-linkonce.s
3

I think you need to add a definition of def to Inputs/comdat.s for this test to make sense.

pcc added inline comments.Mar 25 2019, 10:38 AM
lld/trunk/test/ELF/comdat-linkonce.s
3

Was this ever done?

Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 10:38 AM