Page MenuHomePhabricator

[lld/mac] Add support for -flat_namespace
ClosedPublic

Authored by thakis on Feb 28 2021, 12:27 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG8174f33dc9bf: [lld/mac] Add support for -flat_namespace
Summary

-flat_namespace makes lld emit binaries that use name lookup that's more in
line with other POSIX systems: Instead of looking up symbols as (dylib,name)
pairs by dyld, they're instead looked up just by name.

-flat_namespace has three effects:

  1. MH_TWOLEVEL and MH_NNOUNDEFS are no longer set in the Mach-O header
  2. All symbols use BIND_SPECIAL_DYLIB_FLAT_LOOKUP as ordinal
  3. When a dylib is added to the link, its dependent dylibs are also added, so that lld can verify that no undefined symbols remain at the end of a link with -flat_namespace. These transitive dylibs are added for symbol resolution, but they are not emitted in LC_LOAD_COMMANDs.

-undefined with -flat_namespace still isn't implemented. Before this change,
it was impossible to hit that combination because -flat_namespace caused a
diagnostic. Now that it no longer does, emit a dedicated temporary diagnostic
when both flags are used.

Diff Detail

Event Timeline

thakis created this revision.Feb 28 2021, 12:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
thakis requested review of this revision.Feb 28 2021, 12:27 PM
int3 accepted this revision.Mar 1 2021, 9:16 AM
int3 added inline comments.
lld/MachO/InputFiles.cpp
704

hm is the :: prefix necessary?

lld/test/MachO/flat-namespace.s
39–40

does llvm-objdump --syms or llvm-readobj print anything better?

lld/test/MachO/header.s
14–16

ultra nit: align the header

This revision is now accepted and ready to land.Mar 1 2021, 9:16 AM
int3 added inline comments.Mar 1 2021, 9:18 AM
lld/MachO/InputFiles.cpp
704

judging from the test failures, I guess we need to account for syslibroot here too

Thanks!

lld/MachO/InputFiles.cpp
704

hm is the :: prefix necessary?

Yes, else ADL makes this try to use lld::macho::loadDyib() (from Driver.h, in DriverUtils.cpp)

704

judging from the test failures, I guess we need to account for syslibroot here too

ah, annoying. will try to do that…

lld/test/MachO/flat-namespace.s
39–40

llvm-objdump --syms doesn't print it. llvm-readobj has it as Flags [ (0xFE00). obj2yaml also prints it as n_desc: 65024.

I think we definitely want to test nm here since that's what end users are most likely to use. Want me to add a readobj/yaml2obj test in addition to what's here, or nah?

lld/test/MachO/header.s
14–16

Done, but it's not aligned in llvm-objdump output :P

int3 added inline comments.Mar 1 2021, 10:49 AM
lld/test/MachO/flat-namespace.s
39–40

A readobj test would be nice :)

readobj test; honor syslibroot

thakis added a comment.EditedMar 1 2021, 12:07 PM

Hm, still fails with

lld: error: unable to locate library '/usr/lib/libSystem.B.dylib' loaded from '/mnt/disks/ssd0/agent/llvm-project/build/tools/lld/test/MachO/Output/flat-namespace.s.tmp/baz.dylib' for -flat_namespace

I think that's because lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.tbd contains /usr/lib/libSystem.B.dylib (note .B) but we don't have a lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.B.tbd. In Real Life, libSystem.dylib symlinks to libSystem.B.dylib. Symlinks are tricky because Windows. Options:

  1. Make a copy of lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.tbd at lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.B.tbd and check it in. Somewhat ugly because two copies of that file checked in, but the file basically never changes.
  2. In the test, make a temp dummy dir, copy lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libSystem.tbd to both %t/mysysroot/libSystem.tbd and %t/mysysroot/libSystem.B.tbd, and use that as -libsysroot. Somewhat ugly because the test doesn't "know" about libsysroot since that comes in via %lld.

Opinions? I weakly lean towards 1.

int3 added a comment.Mar 1 2021, 12:13 PM

How about just removing all references to libSystem.B.dylib and just use libSystem.dylib everywhere? I don't think we have anything in our code or tests that actually cares that it's symlinked

This revision was landed with ongoing or failed builds.Mar 1 2021, 12:26 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 12:26 PM

How about just removing all references to libSystem.B.dylib and just use libSystem.dylib everywhere? I don't think we have anything in our code or tests that actually cares that it's symlinked

Hey, that's a great idea. Landed with that in a separate preparatory commit (3e6b6cee00819d256f30e84aec7ae634e0725534) :)