This is an archive of the discontinued LLVM Phabricator instance.

Differential D101786

[llvm-objdump] Exclude __mh_*_header symbols during MachO disassembly
ClosedPublic

Authored by gkm on May 3 2021, 1:55 PM.

Details

Reviewers
jhenderson
MaskRay
alexander-shaposhnikov
int3
Commits
rG5a439015393e: [llvm-objdump] Exclude __mh_*_header symbols during MachO disassembly
Summary

__mh_(execute|dylib|dylinker|bundle|preload|object)_header are special symbols whose values hold the VMA of the Mach header to support introspection. They are attached to the first section in __TEXT, even though their addresses are outside __TEXT, and they do not refer to code.

It is normally harmless, but when the first section of __TEXT has no other symbols, __mh_*_header is considered by the disassembler when determing function boundaries. Since __mh_*_header refers to an address outside __TEXT, the boundary determination fails and disassembly quits.

Since __TEXT,__text normally has symbols, this bug is obscured. Experiments placing __stubs and __stub_helper first exposed the bug, since neither has symbols.

Diff Detail

Event Timeline

gkm created this revision.May 3 2021, 1:55 PM
Herald added a reviewer: jhenderson. · View Herald TranscriptMay 3 2021, 1:55 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added subscribers: rupprecht, kristof.beyls. · View Herald Transcript
gkm requested review of this revision.May 3 2021, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 1:55 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
smeenai added a reviewer: alexander-shaposhnikov.May 3 2021, 3:43 PM
Harbormaster completed remote builds in B102386: Diff 342539.May 3 2021, 4:39 PM
MaskRay added inline comments.May 4 2021, 12:51 PM
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
2

Use yaml2obj to create the test file. A prebuilt binary is difficult to inspect/modify.

gkm updated this revision to Diff 343877.May 8 2021, 6:17 PM
  • revise according to review feedback
Harbormaster completed remote builds in B103350: Diff 343877.May 8 2021, 6:51 PM
int3 added a subscriber: int3.May 11 2021, 9:47 AM
int3 added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1082

there are a bunch of other __mh_*_header symbols (you can see them in the LLD source)... should we ignore them all?

int3 added inline comments.May 11 2021, 9:53 AM
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
77

I believe LC_UNIXTHREAD is emitted when linking for older target versions. If you pass a modern macOS version to the ld64 you should get LC_MAIN instead. Not that it really matters, but LC_MAIN is generally more compact to encode, especially in yaml -- it doesn't have all those PayloadBytes.

gkm marked 2 inline comments as done.May 11 2021, 1:44 PM
gkm added inline comments.
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
77

LC_UNIXTHREAD is required for static linking regardless of version, and there is no way to override that. YAML output for a dynamic link is larger.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1082

Yes, all should be ignored. They serve the same purpose of header introspection and do not offer any useful information about code boundaries to the disassembler.

int3 added inline comments.May 11 2021, 1:50 PM
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
77

Ah I see. I went to look at my recent diff that added a yaml test, and it seems like yaml2obj doesn't require either load command: rG9260760235261a5cd150b15a3499f7988da65a02

Not sure if objdump needs it to work, but the guidance I've gotten in the past wrt yaml inputs is to make them as small as possible, even if the resulting binary doesn't execute.

gkm marked 2 inline comments as done.May 11 2021, 2:38 PM
gkm added inline comments.
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
77

I manually edited the YAML file to remove PayloadBytes for LC_UNIXTHREAD. I couldn't remove the entire command without getting a non-obvious error from yaml2obj. Is manual editing how we're supposed to trim these files?

int3 added inline comments.May 11 2021, 2:52 PM
llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test
77

Is manual editing how we're supposed to trim these files?

yeah that's what I've done.

LC_UUID should be removable too.

gkm updated this revision to Diff 344590.May 11 2021, 4:36 PM
  • revise according to review feedback
Harbormaster completed remote builds in B103879: Diff 344590.May 11 2021, 5:36 PM
gkm updated this revision to Diff 344619.May 11 2021, 5:50 PM
  • trim a few more load commands from the test YAML
gkm marked 2 inline comments as done.May 11 2021, 5:51 PM
int3 accepted this revision.May 11 2021, 6:33 PM

lgtm

This revision is now accepted and ready to land.May 11 2021, 6:33 PM
int3 added a comment.May 11 2021, 6:53 PM

Maybe update the commit title since we now support all the __mh_* symbols

Harbormaster completed remote builds in B103906: Diff 344619.May 11 2021, 7:11 PM
gkm updated this revision to Diff 344804.May 12 2021, 6:38 AM
gkm retitled this revision from [llvm-objdump] Exclude __mh_execute_header symbol during MachO disassembly to [llvm-objdump] Exclude __mh_*_header symbols during MachO disassembly.
gkm edited the summary of this revision. (Show Details)
  • update title & summary
This revision was landed with ongoing or failed builds.May 12 2021, 6:39 AM
Closed by commit rG5a439015393e: [llvm-objdump] Exclude __mh_*_header symbols during MachO disassembly (authored by gkm). · Explain Why
This revision was automatically updated to reflect the committed changes.
gkm added a commit: rG5a439015393e: [llvm-objdump] Exclude __mh_*_header symbols during MachO disassembly.
Harbormaster completed remote builds in B104043: Diff 344804.May 12 2021, 7:31 AM

Revision Contents

PathSize
llvm/
test/
tools/
llvm-objdump/
MachO/
92 lines
tools/
llvm-objdump/
11 lines

Diff 344805

llvm/test/tools/llvm-objdump/MachO/no-text-symbols-disassembly.test

  • This file was added.
## Verify that we see dissassembler output even when there are no
## symbols in __TEXT,__text.
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Use yaml2obj to create the test file. A prebuilt binary is difficult to inspect/modify.

MaskRay: Use yaml2obj to create the test file. A prebuilt binary is difficult to inspect/modify.
# RUN: yaml2obj %s -o %t.o
# RUN: llvm-objdump -d %t.o | FileCheck %s
--- !mach-o
FileHeader:
magic: 0xFEEDFACF
cputype: 0x01000007
cpusubtype: 0x00000003
filetype: 0x00000002
ncmds: 4
sizeofcmds: 544
flags: 0x00000001
reserved: 0x00000000
LoadCommands:
- cmd: LC_SEGMENT_64
cmdsize: 72
segname: __PAGEZERO
vmaddr: 0
vmsize: 4294967296
fileoff: 0
filesize: 0
maxprot: 0
initprot: 0
nsects: 0
flags: 0
- cmd: LC_SEGMENT_64
cmdsize: 152
segname: __TEXT
vmaddr: 4294967296
vmsize: 4096
fileoff: 0
filesize: 4096
maxprot: 5
initprot: 5
nsects: 1
flags: 0
Sections:
- sectname: __text
segname: __TEXT
addr: 0x0000000100000FF7
size: 9
offset: 0x00000FF7
align: 0
reloff: 0x00000000
nreloc: 0
flags: 0x80000400
reserved1: 0x00000000
reserved2: 0x00000000
reserved3: 0x00000000
- cmd: LC_SEGMENT_64
cmdsize: 72
segname: __LINKEDIT
vmaddr: 4294971392
vmsize: 4096
fileoff: 4096
filesize: 64
maxprot: 1
initprot: 1
nsects: 0
flags: 0
- cmd: LC_SYMTAB
cmdsize: 24
symoff: 4096
nsyms: 2
stroff: 4128
strsize: 32
LinkEditData:
NameList:
- n_strx: 2
n_type: 0x03
n_sect: 1
n_desc: 16
n_value: 4294967296
- n_strx: 22
int3Unsubmitted
Done
ReplyInline Actions

I believe LC_UNIXTHREAD is emitted when linking for older target versions. If you pass a modern macOS version to the ld64 you should get LC_MAIN instead. Not that it really matters, but LC_MAIN is generally more compact to encode, especially in yaml -- it doesn't have all those PayloadBytes.

int3: I believe `LC_UNIXTHREAD` is emitted when linking for older target versions. If you pass a…
gkmAuthorUnsubmitted
Done
ReplyInline Actions

LC_UNIXTHREAD is required for static linking regardless of version, and there is no way to override that. YAML output for a dynamic link is larger.

gkm: `LC_UNIXTHREAD` is required for static linking regardless of version, and there is no way to…
int3Unsubmitted
Done
ReplyInline Actions

Ah I see. I went to look at my recent diff that added a yaml test, and it seems like yaml2obj doesn't require either load command: rG9260760235261a5cd150b15a3499f7988da65a02

Not sure if objdump needs it to work, but the guidance I've gotten in the past wrt yaml inputs is to make them as small as possible, even if the resulting binary doesn't execute.

int3: Ah I see. I went to look at my recent diff that added a yaml test, and it seems like yaml2obj…
gkmAuthorUnsubmitted
Done
ReplyInline Actions

I manually edited the YAML file to remove PayloadBytes for LC_UNIXTHREAD. I couldn't remove the entire command without getting a non-obvious error from yaml2obj. Is manual editing how we're supposed to trim these files?

gkm: I manually edited the YAML file to remove `PayloadBytes` for `LC_UNIXTHREAD`. I couldn't remove…
int3Unsubmitted
Done
ReplyInline Actions

Is manual editing how we're supposed to trim these files?

yeah that's what I've done.

LC_UUID should be removable too.

int3: > Is manual editing how we're supposed to trim these files? yeah that's what I've done.
n_type: 0x01
n_sect: 0
n_desc: 0
n_value: 0
StringTable:
- ' '
- __mh_execute_header
- start
- ''
- ''
- ''
- ''
...
CHECK: Disassembly of section __TEXT,__text:

llvm/tools/llvm-objdump/llvm-objdump.cpp

Show First 20 Lines • Show All 1,070 Lines • ▼ Show 20 Lines if (!NameOrErr) {
continue; continue;
} }
if (NameOrErr->empty() && !(Obj->isXCOFF() && SymbolDescription)) if (NameOrErr->empty() && !(Obj->isXCOFF() && SymbolDescription))
continue; continue;
if (Obj->isELF() && getElfSymbolType(Obj, Symbol) == ELF::STT_SECTION) if (Obj->isELF() && getElfSymbolType(Obj, Symbol) == ELF::STT_SECTION)
continue; continue;
if (MachO) {
// __mh_(execute|dylib|dylinker|bundle|preload|object)_header are special
// symbols that support MachO header introspection. They do not bind to
// code locations and are irrelevant for disassembly.
int3Unsubmitted
Done
ReplyInline Actions

there are a bunch of other __mh_*_header symbols (you can see them in the LLD source)... should we ignore them all?

int3: there are a bunch of other `__mh_*_header` symbols (you can see them in the LLD source)...
gkmAuthorUnsubmitted
Done
ReplyInline Actions

Yes, all should be ignored. They serve the same purpose of header introspection and do not offer any useful information about code boundaries to the disassembler.

gkm: Yes, all should be ignored. They serve the same purpose of header introspection and do not…
if (NameOrErr->startswith("__mh_") && NameOrErr->endswith("_header"))
continue;
// Don't ask a Mach-O STAB symbol for its section unless you know that // Don't ask a Mach-O STAB symbol for its section unless you know that
// STAB symbol's section field refers to a valid section index. Otherwise // STAB symbol's section field refers to a valid section index. Otherwise
// the symbol may error trying to load a section that does not exist. // the symbol may error trying to load a section that does not exist.
if (MachO) {
DataRefImpl SymDRI = Symbol.getRawDataRefImpl(); DataRefImpl SymDRI = Symbol.getRawDataRefImpl();
uint8_t NType = (MachO->is64Bit() ? uint8_t NType = (MachO->is64Bit() ?
MachO->getSymbol64TableEntry(SymDRI).n_type: MachO->getSymbol64TableEntry(SymDRI).n_type:
MachO->getSymbolTableEntry(SymDRI).n_type); MachO->getSymbolTableEntry(SymDRI).n_type);
if (NType & MachO::N_STAB) if (NType & MachO::N_STAB)
continue; continue;
} }
▲ Show 20 Lines • Show All 1,519 Lines • Show Last 20 Lines