Page MenuHomePhabricator

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

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

Details

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 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
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
229

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

oontvoo requested review of this revision.Mon, Nov 15, 11:50 AM

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

int3 added a comment.Wed, Nov 17, 12:15 PM

can we have a test? :)

oontvoo added a comment.EditedThu, Nov 18, 6:12 PM

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.Thu, Nov 18, 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.Fri, Nov 19, 7:57 AM

added test file

🔔 🔔 🔔 (friendly)!

int3 added a comment.Mon, Nov 22, 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
2

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

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

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.Mon, Nov 22, 11:10 AM
oontvoo marked an inline comment as done.

removed double space.

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

Thanks!

This revision was landed with ongoing or failed builds.Mon, Nov 22, 11:11 AM
This revision was automatically updated to reflect the committed changes.