This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] implement options -map
ClosedPublic

Authored by woshiccm on Mar 9 2021, 11:54 PM.

Details

Reviewers
int3
thakis
Group Reviewers
Restricted Project
Commits
rGed8bff13dcaa: [lld-macho] implement options -map
Summary

Implement command-line options -map

Diff Detail

Event Timeline

woshiccm created this revision.Mar 9 2021, 11:54 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
woshiccm requested review of this revision.Mar 9 2021, 11:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 11:54 PM
woshiccm edited the summary of this revision. (Show Details)Mar 9 2021, 11:57 PM
thakis added a subscriber: thakis.Mar 10 2021, 6:13 AM

Thanks for the patch!

Overall, looks pretty good.

Please clang-format the file (make the comments in the first few lines <= 80 cls first).

Please add a test.

Does ld64 print CommonSymbols, or does this run late enough that they've already been replaced by Defineds? (A good thing to include in the test.)

lld/MachO doesn't use TimeTraceScope anywhere else yet (it should!), so it's a bit weird to have it here. Maybe should add actual time trace support in a patch before this? But I suppose that part is fine as-is as well.

woshiccm edited the summary of this revision. (Show Details)Mar 10 2021, 6:51 PM
woshiccm updated this revision to Diff 329860.Mar 11 2021, 12:26 AM

Fix MapFile format

woshiccm updated this revision to Diff 329861.Mar 11 2021, 12:33 AM

Fix MapFile.cpp format

woshiccm updated this revision to Diff 329863.Mar 11 2021, 12:53 AM

Fix Writer.cpp format

woshiccm added a comment.EditedMar 11 2021, 1:00 AM

Thanks for the patch!

Overall, looks pretty good.

Please clang-format the file (make the comments in the first few lines <= 80 cls first).

Please add a test.

Does ld64 print CommonSymbols, or does this run late enough that they've already been replaced by Defineds? (A good thing to include in the test.)

lld/MachO doesn't use TimeTraceScope anywhere else yet (it should!), so it's a bit weird to have it here. Maybe should add actual time trace support in a patch before this? But I suppose that part is fine as-is as well.

The format fixed. Yes, CommonSymbols have already been replaced by Defineds.
Could we merge this patch first, and then I add test? I am not familiar with writing tests yet, it may take some time.

New code should be covered by a test added in the same change. See https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests and https://llvm.org/docs/CommandGuide/FileCheck.html#tutorial -- and look at the existing tests in lld/test/MachO for many examples, of course :)

thakis requested changes to this revision.Mar 11 2021, 7:35 AM
This revision now requires changes to proceed.Mar 11 2021, 7:35 AM
int3 added a comment.Mar 11 2021, 9:17 AM

Yeah diffs should generally come with corresponding tests. This code is fairly self-contained, so there's little chance of rebase conflicts and no rush to land :)

You can run the existing tests under lld/macho via llvm-lit -vva <filename>. -vva will let you see what commands are being run to get a sense of how things work.

lld/MachO/MapFile.cpp
10
62–71

this is fairly similar to SymtabSection::finalizeContents, with some minor differences. We should check if the logic can be factored out. I can help with that though :) could you leave a TODO for now?

66

we should check for sym != nullptr in this loop (we don't parse symbols in debug sections, so they will be null)

also JFYI, lld::macho:: won't be necessary once D98149 lands

74–75

this function isn't using parallel-for :) leave a TODO?

85

llvm/TextAPI/MachO/Architecture.h defines getArchitectureName()

woshiccm updated this revision to Diff 330149.Mar 11 2021, 10:42 PM

Fix issues

woshiccm updated this revision to Diff 330150.Mar 11 2021, 10:44 PM

Fix issues

woshiccm updated this revision to Diff 330193.Mar 12 2021, 3:01 AM

Add map-file.s

New code should be covered by a test added in the same change. See https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests and https://llvm.org/docs/CommandGuide/FileCheck.html#tutorial -- and look at the existing tests in lld/test/MachO for many examples, of course :)

I added map-file.s for test

lld/MachO/MapFile.cpp
74–75

done

85

done

Yeah diffs should generally come with corresponding tests. This code is fairly self-contained, so there's little chance of rebase conflicts and no rush to land :)

You can run the existing tests under lld/macho via llvm-lit -vva <filename>. -vva will let you see what commands are being run to get a sense of how things work.

I added map-file.s for test

int3 added inline comments.Mar 12 2021, 12:21 PM
lld/MachO/Writer.cpp
939

I think writeMapFile() should be moved below this line -- there's no need to emit the mapfile if we can't emit the binary itself

lld/test/MachO/map-file.s
7

The test seems to be failing, I guess it should be a -map without the =?

also I think it would be nicer to have the output go to %t/map

15

we should test common symbols (.comm) as well, as @thakis mentioned in an earlier comment

29–30

We try not to match against hardcoded addresses because they're a bit of a pain to update. In this case I think it would be better to match this against llvm-objdump's output: something like

RUN: llvm-objdump --syms --section-headers %t/test-map > %t/objdump
RUN: cat %t/objdump %t/map > %t/out
RUN: FileCheck %s < %t/out

Then we can use numeric substitutions to check that the values line up.

woshiccm updated this revision to Diff 330496.Mar 14 2021, 12:52 AM

Fix map-file.s

int3 added a comment.Mar 15 2021, 12:24 PM

almost there!

lld/test/MachO/map-file.s
7

could you move the other files under %t/ as well, i.e. %t/foo.o and %t/test.o?

29–30

this still needs to be addressed

woshiccm updated this revision to Diff 330870.Mar 15 2021, 8:40 PM

Fix map-file.s

int3 added inline comments.Mar 16 2021, 7:15 PM
lld/test/MachO/map-file.s
29–30

So right now the test is just ignoring the value, but it would be better if it checked that it matched the actual addresses in the binary using numeric substitutions. It would be something like this:

# RUN: llvm-objdump --syms --section-headers %t/test-map > %t/objdump
# RUN: cat %t/objdump %t/map > %t/out
# RUN: FileCheck %s < %t/out
...
# CHECK:      Sections:
# CHECK-NEXT: Idx  Name          Size           VMA          Type
# CHECK-NEXT:   0  __text        {{[0-9a-f]+}} [[#%x,TEXT:]] TEXT
...
# CHECK-NEXT: # Sections:
# CHECK-NEXT: # Address  Size      Segment  Section
# CHECK-NEXT: 0x[[#TEXT]]  0x00000001  __TEXT  __text
woshiccm updated this revision to Diff 331154.Mar 16 2021, 9:00 PM

Fix map-file.s

int3 accepted this revision.Mar 16 2021, 9:32 PM
int3 added inline comments.
lld/test/MachO/map-file.s
31–33

you're re-defining the TEXT/BSS/DATA symbols here (which were already defined on lines 25-27). I know they're the same values since these symbols are at the beginnings of their sections, but I think it would be nicer to define separate names for the symbols (e.g. MAIN, NUMBER, FOO).

woshiccm updated this revision to Diff 331166.Mar 16 2021, 11:34 PM

Fix map-file.s

woshiccm updated this revision to Diff 331167.Mar 16 2021, 11:37 PM

Fix map-file.s

woshiccm updated this revision to Diff 331445.Mar 17 2021, 7:44 PM

Fix map-fils.s

woshiccm updated this revision to Diff 331446.Mar 17 2021, 7:58 PM

Fix map-file.s

oontvoo added inline comments.
lld/MachO/Driver.cpp
829

maybe check and warn if the path is empty?

int3 added inline comments.Mar 17 2021, 8:28 PM
lld/MachO/Driver.cpp
829

The empty check is already done in writeMapFile(). And I think the option parser will reject something like -map without an argument, so we don't need to warn (but I haven't verified this)

Harbormaster completed remote builds in B94377: Diff 331445.
woshiccm updated this revision to Diff 331471.Mar 17 2021, 11:30 PM

Rebase diff

This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2021, 7:39 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.