Page MenuHomePhabricator

[llvm-objcopy][MachO] Implement --redefine-sym and --redefine-syms
ClosedPublic

Authored by MaskRay on Nov 13 2019, 4:07 PM.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 13 2019, 4:07 PM
jhenderson added inline comments.Nov 14 2019, 2:05 AM
llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
4

Any reason you're not using yaml2obj for this?

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
60–64

It looks like there's currently no way to remove individual symbols by name in llvm-objcopy for Mach-O, but I wanted to make sure that this is the right order, i.e. symbols are renamed before they are checked for removal, to make sure things don't break in the future.

For example does objcopy --redefine-sym foo=bar -N foo test.o leave a symbol called bar?

MaskRay updated this revision to Diff 229334.Nov 14 2019, 9:46 AM
MaskRay marked an inline comment as done.

Add a test that checks --redefine-sym executes before --strip-symbol/-N.

-N is currently a no-op. This prevents us from making a mistake when -N is implemented.

MaskRay updated this revision to Diff 229337.Nov 14 2019, 9:54 AM

Improve --strip-symbol tests

MaskRay marked an inline comment as done.Nov 14 2019, 9:54 AM
MaskRay added inline comments.
llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s
4

A YAML test file would be verbose:

--- !mach-o
FileHeader:
  magic:           0xFEEDFACF
  cputype:         0x01000007
  cpusubtype:      0x00000003
  filetype:        0x00000001
  ncmds:           3
  sizeofcmds:      336
  flags:           0x00000000
  reserved:        0x00000000
LoadCommands:
  - cmd:             LC_SEGMENT_64
    cmdsize:         232
    segname:         ''
    vmaddr:          0
    vmsize:          0
    fileoff:         368
    filesize:        0
    maxprot:         7
    initprot:        7
    nsects:          2
    flags:           0
    Sections:
      - sectname:        __text
        segname:         __TEXT
        addr:            0x0000000000000000
        size:            0
        offset:          0x00000170
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x80000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
        content:         ''
      - sectname:        __const
        segname:         __TEXT
        addr:            0x0000000000000000
        size:            0
        offset:          0x00000170
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x00000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
        content:         ''
  - cmd:             LC_SYMTAB
    cmdsize:         24
    symoff:          368
    nsyms:           2
    stroff:          400
    strsize:         12
  - cmd:             LC_DYSYMTAB
    cmdsize:         80
    ilocalsym:       0
    nlocalsym:       0
    iextdefsym:      0
    nextdefsym:      2
    iundefsym:       2
    nundefsym:       0
    tocoff:          0
    ntoc:            0
    modtaboff:       0
    nmodtab:         0
    extrefsymoff:    0
    nextrefsyms:     0
    indirectsymoff:  0
    nindirectsyms:   0
    extreloff:       0
    nextrel:         0
    locreloff:       0
    nlocrel:         0
LinkEditData:
  NameList:
    - n_strx:          1
      n_type:          0x0F
      n_sect:          2
      n_desc:          0
      n_value:         0
    - n_strx:          6
      n_type:          0x0F
      n_sect:          1
      n_desc:          0
      n_value:         0
  StringTable:
    - ''
    - _foo
    - _func
...

Most fields of MachOYAML::Section (see lib/ObjectYAML/MachOYAML.cpp:276) are required. I feel that replicating what we do for ELF may not be very good for MachO. We can probably change the assembly test to YAML when the YAML files used by yaml2obj/obj2yaml become more succinct.

jhenderson accepted this revision.Nov 15 2019, 2:07 AM

This looks good to me, but not being a Mach-O expert, I can't say for certain that there isn't anything else to be aware of, so perhaps wait a bit to give @seiya or @alexshap or another Mach-O expert a chance to chime in, unless you're confident that there is nothing else to worry about.

This revision is now accepted and ready to land.Nov 15 2019, 2:07 AM
seiya accepted this revision.Nov 15 2019, 7:45 AM
This revision was automatically updated to reflect the committed changes.