Page MenuHomePhabricator

[llvm-objcopy][MachO] Implement --add-section
ClosedPublic

Authored by seiya on Aug 15 2019, 2:38 AM.

Diff Detail

Event Timeline

seiya created this revision.Aug 15 2019, 2:38 AM
seiya updated this revision to Diff 215355.Aug 15 2019, 2:39 AM
  • Removed trailing whitespaces.
Harbormaster completed remote builds in B36821: Diff 215355.
jhenderson added inline comments.Aug 15 2019, 3:37 AM
llvm/test/tools/llvm-objcopy/MachO/add-section.test
2

Remove the "if" before llvm-objcopy

8

You also need a test case for what happens if %t.data doesn't exist.

176

Nit: no new line at EOF.

rupprecht added inline comments.Aug 15 2019, 2:53 PM
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
75–83

This boilerplate should be handled in CopyConfig instead of copied in every platform-specific implementation, can you add a FIXME to consolidate it?

87

SecName is actually the pair of <segment>,<section>, so shouldn't SecName be replaced with Pair.second? I think the test case you have shows this.

103

If the segment can't be created, it seems like it's not at all related to the file content, so either the filename here should be the file we're trying to add a segment to, or it should be a regular StringError.

llvm/tools/llvm-objcopy/MachO/Object.cpp
47–49

Is there a utility you could extract to convert from possibly-null-delimited segment/section names to StringRefs? This pattern is used above with the strncpy and also in MachOReader

seiya updated this revision to Diff 215531.Aug 15 2019, 8:03 PM
seiya marked 9 inline comments as done.
  • Addressed review comments.
seiya added inline comments.Aug 15 2019, 8:03 PM
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
87

Oops, I didn't noticed that. Thanks!

llvm/tools/llvm-objcopy/MachO/Object.cpp
47–49

Added extractSegmentName for this. I didn't updated strncpy above because it's for filling the segment name, not extracting a possibly-null-terminated string.

seiya updated this revision to Diff 215812.Aug 18 2019, 9:15 PM

Refactored a bit.

seiya updated this revision to Diff 215813.Aug 18 2019, 9:17 PM
  • Added a newline at the end of file.
Harbormaster completed remote builds in B36926: Diff 215813.
jhenderson added inline comments.Aug 19 2019, 3:27 AM
llvm/test/tools/llvm-objcopy/MachO/add-section.test
16

No -> {{[Nn]}}o

The error message is platform-dependent. On some platforms it's "No" and on others it's "no".

seiya updated this revision to Diff 215990.Aug 19 2019, 2:45 PM
seiya marked an inline comment as done.

Fixed a review comment.

seiya updated this revision to Diff 216121.Aug 20 2019, 6:07 AM

Update CommandGuide.

No more comments from me, thanks!

seiya updated this revision to Diff 216130.Aug 20 2019, 6:24 AM

CommandGuide: Removed remove "the" before "<section>".

Don't think I have any comments besides these two, so LGTM once they're addressed & you get a MachO reviewer to be happy w/ this.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
33

sizeof(Sec.sectname) should be sizeof(Sec.segname) here?
(They're the same size, fortunately)

llvm/tools/llvm-objcopy/MachO/Object.h
72–77

A common use of GNU objcopy is to run with --add-section .note.GNU-stack=/dev/null, which I imagine corresponds to a 0-sized section. I think you might want to wrap OwnedContentData in Optional<>. (Assuming a zero-sized section is valid in Mach-O). Additionally, can you add a test for --add-section w/ /dev/null?

The ELF code doesn't use Optional because it takes a different approach by having subclasses for different types of sections, so OwnedDataSection is different from Section -- I don't know if that makes sense here

seiya updated this revision to Diff 233283.Dec 11 2019, 2:02 AM

Addressed review comments and isolate some changes into D71331.

seiya marked 2 inline comments as done.Dec 11 2019, 2:04 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/MachO/Object.h
72–77

Replaced with Optional and added a test case for /dev/null.

seiya updated this revision to Diff 233285.Dec 11 2019, 2:05 AM
seiya marked an inline comment as done.

Added a newline at EOF.

There seems to be some stuff to do with 64-bit versus normal segments in here, which I don't fully follow due to it being Mach-O, but your test only seems to handle the 64-bit segment type. Should it be extended?

seiya updated this revision to Diff 233495.Dec 11 2019, 9:27 PM

Added 32-bit object support.

seiya added a comment.Dec 11 2019, 9:29 PM

There seems to be some stuff to do with 64-bit versus normal segments in here, which I don't fully follow due to it being Mach-O, but your test only seems to handle the 64-bit segment type. Should it be extended?

I forgot to implement 32-bit support. The latest patch includes 32-bit support and tests for it.

jhenderson added inline comments.Dec 12 2019, 2:05 AM
llvm/test/tools/llvm-objcopy/MachO/add-section.test
5

It probably makes sense to call %t %t.64bit.

5–6

By the way, there seems to be a degree of consensus that -o should be preferred to a shell redirection with yaml2obj. No need to modify old tests, but if you're adding new ones/updating them, it's probably good to make this change too.

20

%t.out1 -> %t.out1.64bit or something along those lines

llvm/tools/llvm-objcopy/MachO/Object.cpp
45–49

This and the above block are very similar. You might want to consider pulling them into some kind of function to reduce the duplication.

seiya updated this revision to Diff 233732.Dec 12 2019, 9:07 PM
seiya marked 5 inline comments as done.Dec 12 2019, 9:12 PM
jhenderson accepted this revision.Dec 13 2019, 2:32 AM

LGTM, from a style and testing point of view, but a Mach-O expert should confirm they are happy too.

This revision is now accepted and ready to land.Dec 13 2019, 2:32 AM
alexshap accepted this revision.Dec 13 2019, 11:46 AM

I've added one minor comment, but otherwise this looks good to me, thanks

llvm/tools/llvm-objcopy/MachO/Object.cpp
60

static

This revision was automatically updated to reflect the committed changes.