Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 37030 Build 37029: arc lint + arc unit
Event Timeline
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 |
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". |
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? | |
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 |
llvm/tools/llvm-objcopy/MachO/Object.h | ||
---|---|---|
76–81 | Replaced with Optional and added a test case for /dev/null. |
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.
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 | ||
44–48 | This and the above block are very similar. You might want to consider pulling them into some kind of function to reduce the duplication. |
LGTM, from a style and testing point of view, but a Mach-O expert should confirm they are happy too.
I've added one minor comment, but otherwise this looks good to me, thanks
llvm/tools/llvm-objcopy/MachO/Object.cpp | ||
---|---|---|
59 | static |
Remove the "if" before llvm-objcopy