Page MenuHomePhabricator

[llvm-objcopy][MachO] Implement -Obinary
Needs ReviewPublic

Authored by seiya on Aug 19 2019, 1:55 AM.

Details

Event Timeline

seiya created this revision.Aug 19 2019, 1:55 AM
jhenderson added inline comments.Aug 19 2019, 6:28 AM
llvm/test/tools/llvm-objcopy/MachO/binary-output.test
2

How about "Shows that for binary output, llvm-objcopy extracts ..."?

4

Remove gap -> It removes any gap

6

Preserve gaps -> It preserves any gaps

seiya updated this revision to Diff 215993.Aug 19 2019, 2:47 PM
seiya marked 3 inline comments as done.

Fixed review comments.

grimar added a subscriber: grimar.Aug 20 2019, 1:03 AM
grimar added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
503

LC -> Cmd?

518

This can be:

// Remove ....
for (Section *Sec : OrderedSections)
  Sec->Addr -= OrderedSections[0]->Addr;
531

Does it make sense to remove this in favor of speed?
I.e. instead of filling the whole buffer with zeroes you can fill only gaps.
This might be better for large binaries.

llvm/tools/llvm-objcopy/MachO/MachOWriter.h
72

What do you think of using probably more traditional/clear Obj and Buf names?

jhenderson added inline comments.Aug 20 2019, 2:45 AM
llvm/test/tools/llvm-objcopy/MachO/binary-output.test
3

MachO -> Mach-O (I believe)

rupprecht added inline comments.Aug 22 2019, 1:57 PM
llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
524–527

ELF/binary-output-empty.test would be a good test to add for this

seiya updated this revision to Diff 217294.Mon, Aug 26, 7:25 PM
seiya marked 9 inline comments as done.

Addressed review comments.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
503

LC is the common abbreviation for LoadCommand under llvm-objcopy/MachO.

518

I don't think it works. This for loop subtracts the original value of the first section's address (OrderedSections[0]->Addr). In the code you suggested, in the first iteration, Sec points to OrderedSections[0] and as a result OrderedSections[0]->Addr becomes 0 in remaining iterations.

llvm/tools/llvm-objcopy/MachO/MachOWriter.h
72

I'd like to keep them because O and B are widely used under llvm-objcopy/MachO. That said, Obj and Buf looks better to me. I'll submit another NFC patch to rename them.

seiya marked an inline comment as done.Mon, Aug 26, 7:27 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
531

Updated the patch to fill only gaps instead of the whole buffer.

seiya updated this revision to Diff 217295.Mon, Aug 26, 7:28 PM
seiya marked an inline comment as done.

Addressed a review comment.

Harbormaster completed remote builds in B37327: Diff 217295.
jhenderson added inline comments.Tue, Aug 27, 2:32 AM
llvm/test/tools/llvm-objcopy/MachO/binary-output.test
35

Nit: delete extra blank line here.