This is an archive of the discontinued LLVM Phabricator instance.

[lld][Macho] Include dead-stripped symbols in mapfile
ClosedPublic

Authored by Roger on Nov 29 2021, 12:29 PM.

Details

Summary

ld64 outputs dead stripped symbols when using the -dead-strip flag. This change mimics that behavior for lld.

ld64's -dead_strip flag outputs:

$ ld -map map basics.o -o out -dead_strip -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib -lSystem
$ cat map
# Path: out
# Arch: x86_64
# Object files:
[  0] linker synthesized
[  1] basics.o
# Sections:
# Address       Size            Segment Section
0x100003F97     0x00000021      __TEXT  __text
0x100003FB8     0x00000048      __TEXT  __unwind_info
0x100004000     0x00000008      __DATA_CONST    __got
0x100008000     0x00000010      __DATA  __ref_section
0x100008010     0x00000001      __DATA  __common
# Symbols:
# Address       Size            File  Name
0x100003F97     0x00000006      [  1] _ref_local
0x100003F9D     0x00000001      [  1] _ref_private_extern
0x100003F9E     0x0000000C      [  1] _main
0x100003FAA     0x00000006      [  1] _no_dead_strip_globl
0x100003FB0     0x00000001      [  1] _ref_from_no_dead_strip_globl
0x100003FB1     0x00000006      [  1] _no_dead_strip_local
0x100003FB7     0x00000001      [  1] _ref_from_no_dead_strip_local
0x100003FB8     0x00000048      [  0] compact unwind info
0x100004000     0x00000008      [  0] non-lazy-pointer-to-local: _ref_com
0x100008000     0x00000008      [  1] _ref_data
0x100008008     0x00000008      [  1] l_ref_data
0x100008010     0x00000001      [  1] _ref_com

# Dead Stripped Symbols:
#               Size            File  Name
<<dead>>        0x00000006      [  1] _unref_extern
<<dead>>        0x00000001      [  1] _unref_local
<<dead>>        0x00000007      [  1] _unref_private_extern
<<dead>>        0x00000001      [  1] _ref_private_extern_u
<<dead>>        0x00000008      [  1] _unref_data
<<dead>>        0x00000008      [  1] l_unref_data
<<dead>>        0x00000001      [  1] _unref_com

Diff Detail

Event Timeline

Roger created this revision.Nov 29 2021, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 12:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Roger updated this revision to Diff 394932.Dec 16 2021, 11:14 AM

Small fixes.

Roger updated this revision to Diff 394956.Dec 16 2021, 12:14 PM

small fix.

Roger updated this revision to Diff 395005.Dec 16 2021, 2:50 PM

Compilation fix.

Roger updated this revision to Diff 396050.Dec 23 2021, 10:29 AM

test passes

Roger edited the summary of this revision. (Show Details)Jan 1 2022, 2:12 PM
Roger updated this revision to Diff 396899.Jan 1 2022, 5:49 PM

Update.

Roger edited the summary of this revision. (Show Details)Jan 1 2022, 5:50 PM
Roger edited the summary of this revision. (Show Details)
Roger retitled this revision from [lld][Macho] Provide option to print dead-stripped symbols. to [lld][Macho] Include dead-stripped symbols in mapfile.Jan 1 2022, 6:02 PM
Roger edited the summary of this revision. (Show Details)
Roger published this revision for review.Jan 1 2022, 6:04 PM

I expect this will need some advice but I think it's ready for its initial review.

Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2022, 6:04 PM

The output from ld64 doesn't look like what lld produces. Also the command you linked above doesn't pass in dead_strip. Is ld64 always outputting dead stripped symbols regardless of the flag or is the summary a typo?

lld/MachO/MapFile.cpp
43

Why do we need to propagate Live to the lambda? Does ld64 order dead stripped symbols by the name only?

44

nit: Variable name should be camel case. Also, I think dumpLive or something around that seems to be better fit than just Live.

int3 added inline comments.Jan 5 2022, 1:46 AM
lld/MachO/MapFile.cpp
44

+1 for camel case. Also, rather than calling getSymbols twice, each time iterating over all symbols, it would be more efficient to have the function output two vectors, one for live and one for dead symbols.

Roger updated this revision to Diff 397947.Jan 6 2022, 11:34 AM

Squashing separate commits.

Roger edited the summary of this revision. (Show Details)Jan 6 2022, 12:04 PM
Roger added a comment.Jan 6 2022, 12:12 PM

The output from ld64 doesn't look like what lld produces.

You're right that the new dead symbol section is not exactly the same as what ld64 outputs. look lld/test/MachO/dead-strip.s. I followed the precedence set by lld's section for live symbols and basically replicated that for dead symbols. Do you think this needs to be changed?

Also the command you linked above doesn't pass in dead_strip. Is ld64 always outputting dead stripped symbols regardless of the flag or is the summary a typo?

Yeah, it was a typo. I fixed the summary.

lld/MachO/MapFile.cpp
43

The ordering for live symbols is first by virtual address and then by name. As I understand, dead-stripped symbols don't have virtual addresses so we can just order by name.

Do you think this needs to be changed?

I don't think the core logic needs to be changed. It makes more sense to have LLD's output be a subset of ld64 (for now), which hints at the fact that our dead strip isn't on par with ld64 (or something else is going on).

lld/MachO/MapFile.cpp
43

Do we need to have this ordered? I guess you're trying to not fork the code so passing it into the lambda makes sense, but in general, we could skip the sort for dead stripped symbols.

Roger updated this revision to Diff 398709.Jan 10 2022, 11:36 AM

Rewrite diff to be more declarative.

Roger marked 3 inline comments as done.Jan 10 2022, 11:57 AM
Roger added inline comments.
lld/MachO/MapFile.cpp
43

I guess we don't need to have it ordered. It doesn't look like ld64 orders it. I can take out the ordering if that is better but I don't really know. I've refactored the code a bit but I don't know if that changes anything.

44

@thevinster I decided to encapsulate the "live-ness" of the symbols in their container so the word "Live" now describes the container rather than configure the behavior of the function that dumps symbols. I think the word "Live" is now more appropriate.

@int3 Done.

Roger updated this revision to Diff 398714.Jan 10 2022, 11:57 AM
Roger marked 2 inline comments as done.

Small rename.

mpark added a subscriber: mpark.Jan 10 2022, 1:27 PM
mpark added inline comments.
lld/MachO/MapFile.cpp
44

Maybe just Symbols? There's not much vector about this class aside from an implementation detail. Specifically, you can't use this like a vector at all.

44

Probably want to move these out.

44–45

Seems like you could just make the correct decision here rather than having conditionalInsert do a conditional check.

59
61
177

Seems silly to check the liveness of the container for each element.
It'd be different if you could write C++17 and turn this into an if constexpr,
but otherwise I'd say at least pull this out of the loop such that you only
check this once? Yes the loop would be duplicated but 🤷‍♂️

Roger updated this revision to Diff 398728.Jan 10 2022, 1:35 PM

Rewrite code in a more declarative style.

Roger updated this revision to Diff 398741.Jan 10 2022, 2:20 PM

Addressed mpark's comments.

lld/MachO/MapFile.cpp
44

True.

44

True.

44–45

True. I was trying out pushing conditionals into data structures but this may be taking it too far, yeah.

59

I just got rid of that function.

61

I got rid of that function.

177

I really wanted to make this into an if constexpr. I'll change this to just be two loops.

Roger marked 6 inline comments as done.Jan 10 2022, 2:22 PM
Roger updated this revision to Diff 398746.Jan 10 2022, 2:36 PM

Update comments.

Roger updated this revision to Diff 402580.Jan 24 2022, 10:07 AM

Reverted from declarative style to a more imperative style.

Roger updated this revision to Diff 402583.Jan 24 2022, 10:15 AM

Update assert

int3 added inline comments.Jan 26 2022, 2:17 PM
lld/MachO/MapFile.cpp
43

outdated comment

177–189

with the if being hoisted out, I'm not sure this writeSymbols function is really factoring out a useful amount of code. How about inlining it?

178–181

I was wondering if this should be !deadSymbols.empty(), but it seems like config->deadStrip is the right one as ld64 will emit the "Dead Stripped Symbols" line as long as -dead_strip is passed. Can we have a test that checks for the case where no symbols get stripped?

lld/test/MachO/dead-strip.s
10

outdated comment

Roger updated this revision to Diff 403773.Jan 27 2022, 1:08 PM

Address comments

Roger updated this revision to Diff 403806.Jan 27 2022, 2:32 PM
Roger marked 3 inline comments as done.

Add test with no dead symbols.

Roger marked 2 inline comments as done.Jan 27 2022, 2:32 PM
Roger added inline comments.
lld/MachO/MapFile.cpp
43

The code has changed enough where the original question no longer applies.

177–189

Done.

178–181

I was also thinking about that. I figured it would be better to print the "Dead Stripped Symbols:" header even if there are no dead stripped symbols to explicitly show that there are none. Basing it off the configuration felt like it would make the code would behave with fewest surprises.

int3 accepted this revision.Jan 27 2022, 2:55 PM

Nice!

lld/MachO/MapFile.cpp
1

would be good to brew install clang-format :)

lld/test/MachO/dead-strip.s
59

can we call this file something other than map? That way we're not overwriting the map emitted by the earlier command on line 11. That makes debugging the test easier since we can look in the output folder and see the preserved outputs from each command.

70

this could just be NODEADSYMBOLS-EMPTY:, then you wouldn't need the "END OF MAP"

This revision is now accepted and ready to land.Jan 27 2022, 2:55 PM
thevinster accepted this revision.Jan 27 2022, 3:09 PM
thevinster added inline comments.
lld/MachO/MapFile.cpp
181
Roger updated this revision to Diff 404022.Jan 28 2022, 7:46 AM
Roger marked 4 inline comments as done.

Address comments

Roger added inline comments.Jan 28 2022, 7:47 AM
lld/test/MachO/dead-strip.s
59

Done.

70

Done.

oontvoo added inline comments.
lld/MachO/MapFile.cpp
45

It seems the deadSymbols is only ever used if config->deadStrip.
Can this function take a flag (eg., includeDead) and not collect or sort dead symbols when the flag is not set?

int3 added inline comments.Jan 28 2022, 9:38 AM
lld/MachO/MapFile.cpp
45

when the flag's not set, the deadSymbols vector will be empty anyway, so sorting will be a no-op.

oontvoo added inline comments.Jan 28 2022, 9:49 AM
lld/MachO/MapFile.cpp
45

Ah, ok! :)

This revision was landed with ongoing or failed builds.Jan 28 2022, 10:57 AM
This revision was automatically updated to reflect the committed changes.