Page MenuHomePhabricator

[llvm-objcopy][MachO] Implement --add-section
Needs ReviewPublic

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

Details

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.

117

Nit: no new line at EOF.

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

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

80

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.

96

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
50–52

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
80

Oops, I didn't noticed that. Thanks!

llvm/tools/llvm-objcopy/MachO/Object.cpp
50–52

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
76–81

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