Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
Herald added a subscriber: llvm-commits. ยท View Herald Transcript

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
44

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

60

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

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
60

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
60

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
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.

60

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.

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
45

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.

54โ€“55

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

60
62
62

Probably want to move these out.

139

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
45

True.

54โ€“55

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

60

I just got rid of that function.

62

True.

62

I got rid of that function.

139

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
44

outdated comment

139โ€“151

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?

148โ€“158

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
9โ€“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
60

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

139โ€“151

Done.

148โ€“158

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
61

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.

72

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
151
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
61

Done.

72

Done.

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

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
46

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
46

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.