This is an archive of the discontinued LLVM Phabricator instance.

Differential D110040

[lld-macho] Don't replace local personality symbol with LazySymbol
ClosedPublic

Authored by oontvoo on Sep 19 2021, 9:13 AM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG944071eca2c8: [lld-macho] Don't replace local personality symbol with LazySymbol
Summary

Follup-up to D107533, where we replaced local syms with non-local.
It doesn't make sense to replace local symbol with lazy.

Diff Detail

Event Timeline

oontvoo created this revision.Sep 19 2021, 9:13 AM
Herald added a reviewer: gkm. · View Herald TranscriptSep 19 2021, 9:13 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Sep 19 2021, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2021, 9:13 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B124580: Diff 373466.Sep 19 2021, 9:48 AM
oontvoo edited the summary of this revision. (Show Details)Sep 19 2021, 9:57 AM
int3 added a comment.Sep 20 2021, 8:45 AM

Can we have a test? It's kind of subtle... we typically don't bother checking for LazySymbol because any such referenced symbol would typically by fetch()-ed once all the input files are loaded. But in this case we aren't looking up a symbol by relocation, but by name. I think it's worth documenting in a test so that the check doesn't get removed by someone trying to 'clean up' the code in the future.

lld/MachO/UnwindInfoSection.cpp
198

unrelated to this diff, but doesn't find() return a non-const pointer? In which case we should just take a non-const value on line 197 and skip the cast here

oontvoo updated this revision to Diff 373626.Sep 20 2021, 9:38 AM
oontvoo marked an inline comment as done.

remove spurious const/const_cast

oontvoo planned changes to this revision.Sep 20 2021, 9:39 AM

adding tests

Harbormaster completed remote builds in B124690: Diff 373626.Sep 20 2021, 10:08 AM
oontvoo requested review of this revision.Nov 15 2021, 11:50 AM

Turnt out - we still need this hack. putting it back on the review queue. Please have a look! 🙏

oontvoo mentioned this in D114017: [lld-macho][nfc] Factor-out NFC changes from main __eh_frame diff.Nov 16 2021, 4:27 PM
int3 added a comment.Nov 17 2021, 12:15 PM

can we have a test? :)

oontvoo added a comment.EditedNov 18 2021, 6:12 PM
In D110040#3138464, @int3 wrote:

can we have a test? :)

To be honest, I have a repro but I'm not sure there's a good (maintainable) way to put it into a test.
Repro:

// ------------- defined.s
.private_extern _my_personality

.text
.no_dead_strip _my_personality
_my_personality:
.cfi_startproc
.cfi_def_cfa_offset 16
.cfi_endproc
nop
.subsections_via_symbols

// -----------objc.s
.section __TEXT,__text
.global _OBJC_CLASS_$_MyTest

.no_dead_strip _OBJC_CLASS_$_MyTest
_OBJC_CLASS_$_MyTest:
  .cfi_startproc
  .cfi_personality 155, _my_personality
  .cfi_def_cfa_offset 16
  ret
  .cfi_endproc

  ret
.subsections_via_symbols

build/link commands:

llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator  defined.s -o defined.o
llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator  objc.s -o objc.o 
ld -r -o combined.o
ar r pack.a defined.o combined.o

ld64.lld.darwinnew   -dylib  -arch x86_64  -platform_version ios-simulator 12.0.0 15.0  -syslibroot third_party/apple_sdks/xcode_13_0/iphonesimulator  -ObjC  pack.a

Thoughts?

P.S: I mean, can we check in the archive? or can we rely on (llvm-)ar is available during tests?

int3 added a comment.Nov 18 2021, 10:48 PM

yes, llvm-ar is available :) otherwise we wouldn't be able to test any of our archive handling code...

I think we should use yaml2obj + llvm-ar

oontvoo updated this revision to Diff 388506.Nov 19 2021, 7:57 AM

added test file

Harbormaster completed remote builds in B135124: Diff 388506.Nov 19 2021, 8:27 AM
oontvoo added a comment.Nov 20 2021, 9:29 AM

🔔 🔔 🔔 (friendly)!

int3 added a comment.Nov 22 2021, 9:09 AM

Sorry, too many half-baked things in my diff queue, missed this on Friday :)

lld/test/MachO/objc-uses-custom-personality.s
1

the "objc" part seems tangential to the logic actually being tested, no? can we make the test non-objc-specific?

5
int3 accepted this revision.Nov 22 2021, 9:10 AM
This revision is now accepted and ready to land.Nov 22 2021, 9:10 AM
oontvoo added inline comments.Nov 22 2021, 9:15 AM
lld/test/MachO/objc-uses-custom-personality.s
1

Actually I think this is actually quite essential :P because only ObjC archives has the semantic of "force-loading" when -objC is specified.
Regular C++ classes don't have this.

oontvoo updated this revision to Diff 388978.Nov 22 2021, 11:10 AM
oontvoo marked an inline comment as done.

removed double space.

lld/test/MachO/objc-uses-custom-personality.s
5

Thanks!

This revision was landed with ongoing or failed builds.Nov 22 2021, 11:11 AM
Closed by commit rG944071eca2c8: [lld-macho] Don't replace local personality symbol with LazySymbol (authored by oontvoo). · Explain Why
This revision was automatically updated to reflect the committed changes.
oontvoo added a commit: rG944071eca2c8: [lld-macho] Don't replace local personality symbol with LazySymbol.
Harbormaster completed remote builds in B135464: Diff 388978.Nov 22 2021, 5:07 PM
keith mentioned this in D114275: [test][lld-macho] Improve LC_FUNCTION_STARTS test coverage.Jan 28 2022, 5:39 PM

Revision Contents

PathSize
lld/
MachO/
5 lines
test/
MachO/
224 lines

Diff 388506

lld/MachO/UnwindInfoSection.cpp

Show First 20 Lines • Show All 188 Lines • ▼ Show 20 Lines if (auto *s = r.referent.dyn_cast<Symbol *>()) {
// symbols. Note that LD64's behavior is a bit different and it is // symbols. Note that LD64's behavior is a bit different and it is
// inconsistent with how symbol resolution usually work // inconsistent with how symbol resolution usually work
// //
// So we've decided not to follow it. Instead, simply pick the symbol // So we've decided not to follow it. Instead, simply pick the symbol
// with the same name from the symbol table to replace the local one. // with the same name from the symbol table to replace the local one.
// //
// (See discussions/alternatives already considered on D107533) // (See discussions/alternatives already considered on D107533)
if (!defined->isExternal()) if (!defined->isExternal())
if (const Symbol *sym = symtab->find(defined->getName())) if (Symbol *sym = symtab->find(defined->getName()))
r.referent = s = const_cast<Symbol *>(sym); if (sym->kind() != Symbol::LazyKind)
int3Unsubmitted
Done
ReplyInline Actions

unrelated to this diff, but doesn't find() return a non-const pointer? In which case we should just take a non-const value on line 197 and skip the cast here

int3: unrelated to this diff, but doesn't `find()` return a non-`const` pointer? In which case we…
r.referent = s = sym;
} }
if (auto *undefined = dyn_cast<Undefined>(s)) { if (auto *undefined = dyn_cast<Undefined>(s)) {
treatUndefinedSymbol(*undefined); treatUndefinedSymbol(*undefined);
// treatUndefinedSymbol() can replace s with a DylibSymbol; re-check. // treatUndefinedSymbol() can replace s with a DylibSymbol; re-check.
if (isa<Undefined>(s)) if (isa<Undefined>(s))
continue; continue;
} }
▲ Show 20 Lines • Show All 472 Lines • Show Last 20 Lines

lld/test/MachO/objc-uses-custom-personality.s

  • This file was added.
## Tests that lld correctly resolves the custom personality referenced by objc code in an archive.
int3Unsubmitted
Not Done
ReplyInline Actions

the "objc" part seems tangential to the logic actually being tested, no? can we make the test non-objc-specific?

int3: the "objc" part seems tangential to the logic actually being tested, no? can we make the test…
oontvooAuthorUnsubmitted
Done
ReplyInline Actions

Actually I think this is actually quite essential :P because only ObjC archives has the semantic of "force-loading" when -objC is specified.
Regular C++ classes don't have this.

oontvoo: Actually I think this is actually quite essential :P because only ObjC archives has the…
# REQUIRES: x86
# RUN: rm -rf %t; split-file %s %t
# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/defined.s -o %t/defined.o
int3Unsubmitted
Done
ReplyInline Actions
# RUN: rm -rf %t; split-file %s %t
- # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/defined.s -o %t/defined.o
+ # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/defined.s -o %t/defined.o
# RUN: yaml2obj %t/combined.yaml > %t/combined.o
int3:
oontvooAuthorUnsubmitted
Done
ReplyInline Actions

Thanks!

oontvoo: Thanks!
# RUN: yaml2obj %t/combined.yaml > %t/combined.o
# RUN: llvm-ar r %t/pack.a %t/defined.o %t/combined.o
# RUN: %lld -dylib -arch x86_64 -platform_version ios-simulator 12.0.0 15.0 -ObjC %t/pack.a -o %t/a.dylib
# RUN: llvm-objdump --macho --syms %t/a.dylib | FileCheck %s
# CHECK: SYMBOL TABLE:
# CHECK: {{.*}} l F __TEXT,__text _my_personality
#--- defined.s
.private_extern _my_personality
.text
.no_dead_strip _my_personality
_my_personality:
.cfi_startproc
.cfi_def_cfa_offset 16
.cfi_endproc
nop
.subsections_via_symbols
## combined.yaml is produced from combined.o below:
## lvm-mc -filetype=obj -triple=x86_64-apple-iossimulator %t/objc.s -o %t/objc.o
## ld -r -o combined.o defined.o objc.o
## // objc.s:
## .section __TEXT,__text
## .global _OBJC_CLASS_$_MyTest
## .no_dead_strip _OBJC_CLASS_$_MyTest
## _OBJC_CLASS_$_MyTest:
## .cfi_startproc
## .cfi_personality 155, _my_personality
## .cfi_def_cfa_offset 16
## ret
## .cfi_endproc
##
## ret
## .subsections_via_symbols
#--- combined.yaml
--- !mach-o
FileHeader:
magic: 0xFEEDFACF
cputype: 0x1000007
cpusubtype: 0x3
filetype: 0x1
ncmds: 3
sizeofcmds: 352
flags: 0x2000
reserved: 0x0
LoadCommands:
- cmd: LC_SEGMENT_64
cmdsize: 312
segname: ''
vmaddr: 0
vmsize: 152
fileoff: 416
filesize: 152
maxprot: 7
initprot: 7
nsects: 3
flags: 0
Sections:
- sectname: __text
segname: __TEXT
addr: 0x0
size: 3
offset: 0x1A0
align: 0
reloff: 0x0
nreloc: 0
flags: 0x80000400
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
content: 90C3C3
- sectname: __eh_frame
segname: __TEXT
addr: 0x8
size: 80
offset: 0x1A8
align: 3
reloff: 0x238
nreloc: 5
flags: 0x0
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
content: 1400000000000000017A520001781001100C0708900100001800000000000000017A505200017810069B04000000100C070890011800000004000000F8FFFFFFFFFFFFFF0100000000000000000E1000
relocations:
- address: 0x2A
symbolnum: 0
pcrel: true
length: 2
extern: true
type: 4
scattered: false
value: 0
- address: 0x38
symbolnum: 2
pcrel: false
length: 2
extern: true
type: 5
scattered: false
value: 0
- address: 0x38
symbolnum: 3
pcrel: false
length: 2
extern: true
type: 0
scattered: false
value: 0
- address: 0x3C
symbolnum: 3
pcrel: false
length: 3
extern: true
type: 5
scattered: false
value: 0
- address: 0x3C
symbolnum: 4
pcrel: false
length: 3
extern: true
type: 0
scattered: false
value: 0
- sectname: __compact_unwind
segname: __LD
addr: 0x58
size: 64
offset: 0x1F8
align: 3
reloff: 0x260
nreloc: 3
flags: 0x2000000
reserved1: 0x0
reserved2: 0x0
reserved3: 0x0
content: '00000000000000000100000000000202000000000000000000000000000000000000000000000000020000000000020200000000000000000000000000000000'
relocations:
- address: 0x0
symbolnum: 0
pcrel: false
length: 3
extern: true
type: 0
scattered: false
value: 0
- address: 0x20
symbolnum: 4
pcrel: false
length: 3
extern: true
type: 0
scattered: false
value: 0
- address: 0x30
symbolnum: 0
pcrel: false
length: 3
extern: true
type: 0
scattered: false
value: 0
- cmd: LC_SYMTAB
cmdsize: 24
symoff: 632
nsyms: 5
stroff: 712
strsize: 64
- cmd: LC_DATA_IN_CODE
cmdsize: 16
dataoff: 632
datasize: 0
LinkEditData:
NameList:
- n_strx: 23
n_type: 0x1E
n_sect: 1
n_desc: 32
n_value: 0
- n_strx: 39
n_type: 0xE
n_sect: 2
n_desc: 0
n_value: 8
- n_strx: 39
n_type: 0xE
n_sect: 2
n_desc: 0
n_value: 32
- n_strx: 49
n_type: 0xE
n_sect: 2
n_desc: 0
n_value: 60
- n_strx: 2
n_type: 0xF
n_sect: 1
n_desc: 32
n_value: 1
StringTable:
- ' '
- '_OBJC_CLASS_$_MyTest'
- _my_personality
- EH_Frame1
- func.eh
- ''
- ''
- ''
- ''
- ''
- ''
- ''
...