This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Handle $ld$hide[$os] symbols.
ClosedPublic

Authored by oontvoo on Dec 14 2021, 6:08 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG4f90e67e2f0f: [lld-macho] Handle $ld$hide[$os] symbols.
Summary

This is needed so that LLD wouldn't bind a symbol to a framework where it's hidden (would result in "missing symbol" runtime crash).

Perf impact seemed minimal:
(linking chromium)

x base.txt
+ with_hide.txt

SYSTEM CPU time: 
    N           Min           Max        Median           Avg        Stddev
x   5          0.88           0.9          0.89         0.892  0.0083666003
+   5          0.87           0.9          0.88         0.882   0.010954451
No difference proven at 95.0% confidence

USER CPU time: 
    N           Min           Max        Median           Avg        Stddev
x   5          3.51          3.54          3.51         3.518   0.013038405
+   5          3.47           3.5           3.5          3.49   0.014142136
Difference at 95.0% confidence
	-0.028 +/- 0.019837
	-0.795907% +/- 0.56387%
	(Student's t, pooled s = 0.0136015)

WALL time: 
    N           Min           Max        Median           Avg        Stddev
x   5          3.91          3.94          3.94          3.93   0.014142136
+   5          3.86           3.9           3.9         3.888   0.017888544
Difference at 95.0% confidence
	-0.042 +/- 0.0235167
	-1.0687% +/- 0.598389%
	(Student's t, pooled s = 0.0161245)

PR/52708

Diff Detail

Event Timeline

oontvoo created this revision.Dec 14 2021, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2021, 6:08 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Dec 14 2021, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2021, 6:08 PM
oontvoo planned changes to this revision.Dec 14 2021, 6:10 PM
oontvoo edited the summary of this revision. (Show Details)Dec 14 2021, 6:13 PM
oontvoo updated this revision to Diff 394464.Dec 14 2021, 9:38 PM

add tests

keith added a subscriber: keith.Dec 15 2021, 1:02 PM
keith added inline comments.
lld/MachO/InputFiles.cpp
1158

nit: looks like this could go above the bool assignments

1388

is == there right comparison here? not entirely sure of the use cases but should a .1 difference still have this logic apply?

oontvoo updated this revision to Diff 394647.Dec 15 2021, 1:12 PM
oontvoo marked an inline comment as done.

review comment + more exp in tests

oontvoo added inline comments.Dec 15 2021, 1:33 PM
lld/MachO/InputFiles.cpp
1388

Yes, == is by design. We want exact version match.

Apparently, apple would list literally all the versions it wants to hide in the tbd (rather than using a min version).
Eg., if you want to hide foo for 10, 11, and 12, then you would need $ld$hide$os10.0$foo, $ld$hide$os11.0$foo, $ld$hide$os12.0$foo

int3 added inline comments.Dec 15 2021, 4:40 PM
lld/MachO/InputFiles.cpp
1252–1256

this would be a much cleaner implementation IMO. could we do a few quick benchmarks to see if it actually makes a perf difference? if not, simpler is better :)

1388

is this really == and not <=?

1388

oops, ignore this comment, forgot to delete it after I saw your reply to keith

int3 added inline comments.Dec 15 2021, 4:47 PM
lld/test/MachO/special-symbol-ld-hidden.s
13

should we check whether $ld$hide has an impact on re-exported symbols too?

25–28

shouldn't this be .quad? I believe .long is for 32-bit values

oontvoo updated this revision to Diff 394968.Dec 16 2021, 1:00 PM

Updated diff: process all the $ld symbols. First.

Benchmarking this on chromium vs the previous diff showed little difference

SYSTEM CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          1.05          1.08          1.05         1.058   0.013038405
+   5          1.06          1.09          1.08         1.074   0.013416408
No difference proven at 95.0% confidence

USER CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          3.57          3.65          3.58         3.596   0.032093613
+   5           3.6           3.7          3.61         3.634   0.044497191
No difference proven at 95.0% confidence

WALL time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          4.14          4.26          4.15         4.176    0.04929503
+   5          4.17          4.31           4.2         4.226   0.061073726
No difference proven at 95.0% confidence

(though I suspect it *might* make a difference with a larger app like Youtube ... will try benchmarking it later today)

Interestingly, the two-pass approach seemed faster 🤔 .
In any case, I guess we're good. :)

SYSTEM CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          1.08          1.19           1.1         1.116   0.046151923
+   5          0.91          1.04          0.93          0.95   0.054313902
Difference at 95.0% confidence
	-0.166 +/- 0.0735032
	-14.8746% +/- 6.58631%
	(Student's t, pooled s = 0.0503984)

USER CPU time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          3.28           3.4          3.36          3.35    0.04472136
+   5          3.13           3.3          3.18         3.192   0.068337398
Difference at 95.0% confidence
	-0.158 +/- 0.0842243
	-4.71642% +/- 2.51416%
	(Student's t, pooled s = 0.0577495)

WALL time: 
x one_pass.txt
+ two_passes.txt
    N           Min           Max        Median           Avg        Stddev
x   5          5.54          5.65          5.62         5.608   0.042071368
+   5          4.79          5.31          4.93         4.966    0.21173096
Difference at 95.0% confidence
	-0.642 +/- 0.222622
	-11.4479% +/- 3.96971%
	(Student's t, pooled s = 0.152643)
oontvoo updated this revision to Diff 395177.Dec 17 2021, 11:23 AM
oontvoo marked an inline comment as done.

added more tests

oontvoo marked an inline comment as done.Dec 17 2021, 11:24 AM
oontvoo added inline comments.
lld/test/MachO/Inputs/libReexportSystem2.tbd
1 ↗(On Diff #395177)

This needs to be a standalone tbd because llvm-lit doesn't seem to understand tbd-v3 any more and I need the re-exports: [filename] for my test.

lld/test/MachO/special-symbol-ld-hidden.s
13

Yes - good point. seemed to have no effect on reexported symbols. (I've checked with ld64)

25–28

it needed to be 32-bit because of the @GOTPCREL

int3 added inline comments.Dec 17 2021, 12:17 PM
lld/MachO/InputFiles.cpp
1263

why not just push back a pointer to symbol? InterfaceSymbol seems redundant

lld/test/MachO/Inputs/libReexportSystem2.tbd
1 ↗(On Diff #395177)

weird, what's the error?

but in any case tbdv4 does support reexports too, see e.g. Inputs/libStubLink.tbd

lld/test/MachO/special-symbol-ld-hidden.s
25–28

oh right. it's weird to use GOTPCREL like this though, since this isn't actually an instruction stream. I think .quad _xxx makes more sense if you just want to add a reference to a symbol

most of our other tests do that too

int3 added a comment.Dec 17 2021, 12:56 PM

Oh yeah could you add the benchmark timings to the commit message? Along with the machine you ran them on

oontvoo updated this revision to Diff 395209.Dec 17 2021, 1:09 PM
oontvoo marked an inline comment as done.

updated diff:

  • store Symbol* instead of inventing a new InterfaceSymbol struct
  • use inline tbd
oontvoo added inline comments.Dec 17 2021, 1:10 PM
lld/test/MachO/special-symbol-ld-hidden.s
25–28

also can't use .quad because objdump will crash with

objdump: error: ': truncated or malformed object (for BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB bad offset, not in section for opcode at: 0xa)

:(

int3 added inline comments.Dec 17 2021, 1:13 PM
lld/test/MachO/special-symbol-ld-hidden.s
25–28

weird. can you look at the other tests that use quad to figure out the difference?

oontvoo edited the summary of this revision. (Show Details)Dec 17 2021, 1:22 PM
oontvoo updated this revision to Diff 395214.Dec 17 2021, 1:27 PM

put the refs explicitly in data section

lld/test/MachO/special-symbol-ld-hidden.s
25–28

Oh, found it! I forgot the .data

int3 accepted this revision.Dec 17 2021, 1:33 PM

Thanks!

This revision is now accepted and ready to land.Dec 17 2021, 1:33 PM
This revision was landed with ongoing or failed builds.Dec 17 2021, 1:40 PM
This revision was automatically updated to reflect the committed changes.