Page MenuHomePhabricator

[llvm-objcopy][MachO] Ignore LC_LINKER_OPTION when redefining symbols
ClosedPublic

Authored by keith on Jan 28 2022, 8:08 PM.

Details

Summary

Previously you would get this error:

error: unsupported load command (cmd=0x2d)

If the binary you were redefining the symbols of contained a
LC_LINKER_OPTION load command. This command does not need to be changed
when redefining symbols so we can ignore it like many others.

Diff Detail

Event Timeline

keith created this revision.Jan 28 2022, 8:08 PM
keith requested review of this revision.Jan 28 2022, 8:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2022, 8:08 PM
smeenai accepted this revision.Jan 31 2022, 2:01 PM
smeenai added a subscriber: smeenai.

LGTM

This revision is now accepted and ready to land.Jan 31 2022, 2:01 PM
This revision was landed with ongoing or failed builds.Jan 31 2022, 3:50 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
46

@keith - this test is not a good place for adding various load commands.
I'd recommend to add a separate test (until we have a test that specifically verifies all the copied-over load commands) (and remove this directive from redefine-symbol.s)
(if yaml supports LC_LINKER_OPTION - yaml-based, if it doesn't - assembly-based)
(the test should invoke llvm-objcopy without any extra flags)

keith added inline comments.Feb 7 2022, 9:02 AM
llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
46

Thanks for the feedback! I submitted https://reviews.llvm.org/D119149 which I hope can be a starting point for that, let me know what you think.

For my context how do you choose between yaml vs asm for this kind of test?

llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
46

There are a few considerations / factors here. I'll list some of them below (although there might be more):

  1. in general, for objcopy etc most tests use YAML: (a) human-readable (b) support for both relocatable / non-relocatable binaries (c) more control over various elements of the binary
  2. For Mach-O the support on ObjYAML's side is incomplete
  3. For some situations YAML can be very verbose / inconvenient
  4. If a test uses llvm-mc, then it depends on the MC layer => the test might hypothetically break if there are changes there

(but this happens rarely)

keith added inline comments.Feb 7 2022, 8:03 PM
llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
46

Thank you!