Page MenuHomePhabricator

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

Authored by abrachet on Jan 13 2022, 10:43 PM.

Details

Summary

Implements --update-section which is currently supported for ELF for Mach-O as well

Diff Detail

Event Timeline

abrachet created this revision.Jan 13 2022, 10:43 PM
abrachet requested review of this revision.Jan 13 2022, 10:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 10:43 PM

From what I can gather about Mach-O, it seems like maybe updateSection could just be removeSection + addSection, but I wasn't sure. Maybe someone with more Mach-O experience could say for sure.

Yeah, I'm not really qualified to do this review, as it's fairly format-specific heavy, so I'll defer to others to do it.

thanks for the patch! I'll review it over the weekend

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
343

we probably don't need 2 cases here (specified / unspecified segment name),
for consistency it would be better to always require the full name, e.g. TEXT,text
(otool -d also understands this notation, llvm-objcopy --add-section uses it as well)

423

nit:

std::tie(SectionName, FileName) = Flag.split('=');
abrachet updated this revision to Diff 402279.Jan 22 2022, 7:29 PM

Remove ability to update section without prepending segment name

abrachet marked 2 inline comments as done.Jan 22 2022, 7:30 PM
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
344

immediately called lambda is unnecessary here,
let's split it out into a helper function

425

we need to validate the name (isValidMachOCannonicalName can be of some help here) (+ test case)

abrachet updated this revision to Diff 402293.Jan 22 2022, 10:24 PM
abrachet marked 2 inline comments as done.

small nit, otherwise LGTM

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
320

nit: const Object &O

This revision is now accepted and ready to land.Jan 23 2022, 1:03 AM
This revision was automatically updated to reflect the committed changes.