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

[oontvoo]

Summary

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

[int3]

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

199

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]

remove spurious const/const_cast

[oontvoo]

adding tests

[oontvoo]

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

[int3]

can we have a test? :)

[oontvoo]

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]

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]

added test file

[oontvoo]

🔔 🔔 🔔 (friendly)!

[int3]

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

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

1 ↗

(On Diff #388506)

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

5 ↗

(On Diff #388506)

[oontvoo]

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

1 ↗

(On Diff #388506)

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]

removed double space.

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

5 ↗

(On Diff #388506)

Thanks!

lld/MachO/UnwindInfoSection.cpp

if (auto *s = r.referent.dyn_cast<Symbol *>()) {
         // inconsistent with how symbol resolution usually work
         //
         // 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.
         //
         // (See discussions/alternatives already considered on D107533)
         if (!defined->isExternal())
           if (const Symbol *sym = symtab->find(defined->getName()))
+            if (sym->kind() != Symbol::LazyKind)
               r.referent = s = const_cast<Symbol *>(sym);
       }
       if (auto *undefined = dyn_cast<Undefined>(s)) {
         treatUndefinedSymbol(*undefined);
         // treatUndefinedSymbol() can replace s with a DylibSymbol; re-check.
         if (isa<Undefined>(s))
           continue;
       }