The layout algorithm for relocatable objects and for executable are somewhat different. This patch implements the latter one based on the algorithm in LLD (MachOFileLayout).
Details
Details
- Reviewers
alexander-shaposhnikov rupprecht jhenderson - Commits
- rG12bd490427d7: Recommit "[llvm-objcopy][MachO] Implement a layout algorithm for executables"
rL369301: Recommit "[llvm-objcopy][MachO] Implement a layout algorithm for executables"
rGdee9546b8f83: [llvm-objcopy][MachO] Implement a layout algorithm for executables
rL369231: [llvm-objcopy][MachO] Implement a layout algorithm for executables
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Just a couple of minor comments from me. I don't know enough about Mach-O to be able to review the majority of this.
| llvm/test/tools/llvm-objcopy/MachO/basic-executable-copy.test | ||
|---|---|---|
| 1 ↗ | (On Diff #212669) | Add a comment to the top of this test saying what the purpose of the test is. |
| 3 ↗ | (On Diff #212669) | --file-headers is the canonical option, I believe (we provide --file-header as an alias, but we should probably try to only use the real one here). |
| 5 ↗ | (On Diff #212669) | There's a lot going on in this YAML. Is it the minimal it can be to show all the behaviour, or can it be reduced further? |
| llvm/test/tools/llvm-objcopy/MachO/basic-shared-library-copy.test | ||
| 1 ↗ | (On Diff #212669) | Same comments as previous test. |
Comment Actions
- Remove basic-shared-library-copy.test because shared libraries support is not sufficient.
| llvm/test/tools/llvm-objcopy/MachO/basic-executable-copy.test | ||
|---|---|---|
| 5 ↗ | (On Diff #212669) | It's too long but I think we should keep this verbose YAML to test all executable/shared library-specific load commands like LC_LOAD_DYLINKER. |
Comment Actions
Removed "shared libraries support " from the title and description. I noticed that shared libraries support is not sufficient. I'll submit a separate patch for this at some time.
| llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp | ||
|---|---|---|
| 274 ↗ | (On Diff #213579) | I mean here. |
| llvm/test/tools/llvm-objcopy/MachO/basic-executable-copy.test | ||
|---|---|---|
| 1–2 ↗ | (On Diff #213579) | Replace the second "This test" with "It uses" some part -> some parts |
Comment Actions
Right, my comments have been addressed, but somebody (e.g. @alexshap) with Mach-O knowledge will need to review the rest.
Comment Actions
in general i like the approach, i have added a few minor comments and one question (maybe I'm missing smth).
| llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp | ||
|---|---|---|
| 20 ↗ | (On Diff #213739) | const auto &MLC |
| 104 ↗ | (On Diff #213739) | const bool |
| 151 ↗ | (On Diff #213739) | khm, maybe I'm missing smth, but why do we have Sec.Addr + Sec.Size here ? |
Comment Actions
- Added const.
| llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp | ||
|---|---|---|
| 151 ↗ | (On Diff #213739) | We need to recompute the VMSize in case a new section is added at the end of the segment. Note that in an object file, it seems that Sec.Addr is offset from the beginning of the first section data. Why we need to use std::max() instead of simply assigning Sec.Addr + Sec.Size is that sections are not necessarily sorted by their vm addresses Here's an example: Sections [
Section {
Index: 0
Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
Address: 0x0
Size: 0xC
Offset: 472
...
}
Section {
Index: 1
Name: __bss (5F 5F 62 73 73 00 00 00 00 00 00 00 00 00 00 00)
Segment: __DATA (5F 5F 44 41 54 41 00 00 00 00 00 00 00 00 00 00)
Address: 0x50
Size: 0x4
Offset: 0
...
}
Section {
Index: 2
Name: __eh_frame (5F 5F 65 68 5F 66 72 61 6D 65 00 00 00 00 00 00)
Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
Address: 0x10
Size: 0x40
Offset: 488
...
}
]
Segment {
Cmd: LC_SEGMENT_64
Name:
Size: 312
vmaddr: 0x0
vmsize: 0x54
...
}In this object file, __bss is located at the end of the segment so the vmsize should be (__bss' vmaddr) + (__bss' vmsize), namely, 0x54. However, since __eh_frame comes after the __bss in the section list, we need to determine the end of the segment by std::max. FYI, this part comes from lib/MC/MachObjectWriter.cpp. |