Page MenuHomePhabricator

[lld/mac] Implement support for .weak_def_can_be_hidden
ClosedPublic

Authored by thakis on Apr 22 2021, 9:37 AM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rGa38ebed2581c: [lld/mac] Implement support for .weak_def_can_be_hidden
Summary

I first had a more invasive patch for this (D101069), but while trying
to get that polished for review I realized that lld's current symbol
merging semantics mean that only a very small code change is needed.
So this goes with the smaller patch for now.

This has no effect on projects that build with -fvisibility=hidden
(e.g. chromium), since these see .private_extern symbols instead.

It does have an effect on projects that build with -fvisibility-inlines-hidden
(e.g. llvm) in -O2 builds, where LLVM's GlobalOpt pass will promote most inline
functions from .weak_definition to .weak_def_can_be_hidden.

Before this patch:

% ls -l out/gn/bin/clang out/gn/lib/libclang.dylib
-rwxr-xr-x  1 thakis  staff  113059936 Apr 22 11:51 out/gn/bin/clang
-rwxr-xr-x  1 thakis  staff   86370064 Apr 22 11:51 out/gn/lib/libclang.dylib
% out/gn/bin/llvm-objdump --macho --weak-bind out/gn/bin/clang | wc -l
    8291
% out/gn/bin/llvm-objdump --macho --weak-bind out/gn/lib/libclang.dylib | wc -l
    5698

With this patch:

% ls -l out/gn/bin/clang out/gn/lib/libclang.dylib
-rwxr-xr-x  1 thakis  staff  111721096 Apr 22 11:55 out/gn/bin/clang
-rwxr-xr-x  1 thakis  staff   85291208 Apr 22 11:55 out/gn/lib/libclang.dylib
thakis@MBP llvm-project % out/gn/bin/llvm-objdump --macho --weak-bind out/gn/bin/clang | wc -l
     725
thakis@MBP llvm-project % out/gn/bin/llvm-objdump --macho --weak-bind out/gn/lib/libclang.dylib | wc -l
     542

Linking clang becomes a tiny bit faster with this patch:

x 100    0.67263818    0.77847815    0.69430709    0.69877208   0.017715892
+ 100    0.67209601    0.73323393    0.68600798    0.68917346   0.012824377
Difference at 95.0% confidence
        -0.00959861 +/- 0.00428661
        -1.37364% +/- 0.613449%
        (Student's t, pooled s = 0.0154648)

This only happens if lld with the patch and lld without the patch are both
linked with an lld with the patch or both linked with an lld without the patch
(...or with ld64). I accidentally linked the lld with the patch with an lld
without the patch and the other way round at first. In that setup, no
difference is found. That makese sense, since having fewer weak imports will
make the linked output a bit faster too. So not only does this make linking
binaries such as clang a bit faster (since fewer exports need to be written to
the export trie by lld), the linked output binary binary is also a bit faster
(since dyld needs to process fewer dynamic imports).

This also happens to fix the one check-clang failure when using lld as host
linker, but mostly for silly reasons: See crbug.com/1183336, mostly comment 26.
The real bug here is that c-index-test links all of LLVM both statically and
dynamically, which is an ODR violation. Things just happen to work with this
patch.

So after this patch, check-clang, check-lld, check-llvm all pass with lld as
host linker :)

Diff Detail

Event Timeline

thakis created this revision.Apr 22 2021, 9:37 AM
Herald added a project: Restricted Project. · View Herald Transcript
thakis requested review of this revision.Apr 22 2021, 9:37 AM
int3 accepted this revision.Apr 22 2021, 5:35 PM

love the comments!

lld/MachO/InputFiles.cpp
450
457–459

nit: I assume you aligned the paragraph this way so that the various references to isWeakDefCanBeHidden would line up vertically for easy comparison. To make it clearer, how about changing "and isWeakDefCanBeHidden" to "but isWeakDefCanBeHidden" so it's clearly identical to line 457?

460

Did I understand it correctly?

461

I think the "weak and" is redundant?

lld/test/MachO/weak-def-can-be-hidden.s
20–25

(same for other non-filecheck comments)

I'm wondering if it would be clearer to have this context in InputFiles.cpp 🤔but either seems fine

57

s/arbitary/arbitrary/

but also: maybe "you have to remove the HEADERS check" would be clearer? I had to think for a second to make the link between adding a suffix and ensuring that a -NOT check wouldn't match

This revision is now accepted and ready to land.Apr 22 2021, 5:35 PM
thakis marked 6 inline comments as done.Apr 22 2021, 7:52 PM

Thanks!

lld/MachO/InputFiles.cpp
460

Yes :)

lld/test/MachO/weak-def-can-be-hidden.s
57

There are 3 HEADERS check spread over the file. Adding a suffix is faster :)

thakis marked 2 inline comments as done.Apr 22 2021, 7:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 7:56 PM