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

[keith]

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.

[smeenai]

LGTM

[alexander-shaposhnikov]

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]

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?

[alexander-shaposhnikov]

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]

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

46

Thank you!

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

 # RUN: llvm-readobj --symbols %t.notstripped | FileCheck %s --check-prefix=NOTSTRIPPED
 # NOTSTRIPPED: Name: _ba
 # NOTSTRIPPED: Name: _func
 
 ## FIXME: _ba should be stripped after --strip-symbol is implemented.
 # RUN: llvm-objcopy --strip-symbol _ba --redefine-sym _foo=_ba %t %t.noba
 # RUN: llvm-readobj --symbols %t.noba | FileCheck %s --check-prefix=NOTSTRIPPED
 
+.linker_option "-lc"
 .globl _func
 _func:
 
 .section __TEXT,__const
 .globl _foo
 _foo:

llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp

for (LoadCommand &LC : O.LoadCommands) {
     case MachO::LC_UUID:
     case MachO::LC_SOURCE_VERSION:
     case MachO::LC_THREAD:
     case MachO::LC_UNIXTHREAD:
     case MachO::LC_SUB_FRAMEWORK:
     case MachO::LC_SUB_UMBRELLA:
     case MachO::LC_SUB_CLIENT:
     case MachO::LC_SUB_LIBRARY:
+    case MachO::LC_LINKER_OPTION:
       // Nothing to update.
       break;
     default:
       // Abort if it's unsupported in order to prevent corrupting the object.
       return createStringError(llvm::errc::not_supported,
                                "unsupported load command (cmd=0x%x)", cmd);
     }
   }