This is an archive of the discontinued LLVM Phabricator instance.

[lld][macho] Add more skip platform check for libSystem re-exports
ClosedPublic

Authored by bc-lee on Jan 21 2022, 1:22 PM.

Details

Summary

Xcode 13 comes with a mismatched platform in libcompiler_rt.dylib,
so this creates a linker error on mac catalyst.
Fix it by adding it to the skip list.

Diff Detail

Event Timeline

bc-lee created this revision.Jan 21 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 1:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
bc-lee requested review of this revision.Jan 21 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 1:22 PM
int3 accepted this revision.Jan 21 2022, 5:51 PM
int3 added a subscriber: int3.

Hmm, I would've loved to cross-check it with the latest ld64 code, but the source for version 711 hasn't dropped yet. I'll take your word for it :)

This revision is now accepted and ready to land.Jan 21 2022, 5:51 PM

From what I understand, https://reviews.llvm.org/D100913 ( BZ https://llvm.org/bz49799 GH https://github.com/llvm/llvm-project/issues/49143 ) is for iphonesimulator cases. ld64 is also allowlisting those libraries.

This patch is for the catalyst case, where ld64 doesn't have a separate whitelist, but instead fully allows catalyst binary linking while linking with MacOS dynamic libraries.

The ld64 code related to this is https://github.com/apple-opensource/ld64/blob/e28c028b20af187a16a7161d89e91868a450cadc/src/ld/PlatformSupport.cpp#L177

Maybe we could also allow catalyst binary linking with the all MacOS dynamic libraries, but I don't think this is the best solution.

And I don't have a write access for LLVM, can you commit my changes on behalf of it?

MaskRay accepted this revision.Jan 21 2022, 8:22 PM
MaskRay added inline comments.
lld/MachO/InputFiles.cpp
1274

const at a namespace scope is automatically internal linkage (unless some edge-case conditions) so static can be omitted.

bc-lee updated this revision to Diff 402278.Jan 22 2022, 6:25 PM
bc-lee marked an inline comment as done.
bc-lee added inline comments.
lld/MachO/InputFiles.cpp
1274

Thanks!

bc-lee marked an inline comment as done.Feb 4 2022, 1:03 AM

Hi, int3. Can you commit my changes on behalf of it? I don't have a write access for LLVM.

int3 added a comment.Feb 4 2022, 11:06 AM

Yeah I can commit it. What name and email do you want it committed under?

bc-lee added a comment.Feb 4 2022, 1:27 PM

Yeah I can commit it. What name and email do you want it committed under?

Byoungchan Lee <daniel.l@hpcnt.com>. Thank you!

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 24 2022, 5:36 PM

Should we revert this? Enough stuff is mismatched that ld64 (and, after D124336, ld64.lld) ignore platform mismatches when targeting catalyst for implicit reexports. libcompiler_rt.dylib is an implicit reexport of libSystem, so after D124336 this change here shouldn't have any effect.

Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2022, 5:36 PM
Herald added a subscriber: StephenFan. · View Herald Transcript

@thakis It's okay to revert this. This patch was just a workaround. It's also a good idea to leave constexpr as it is.

Great, I made D124354 for that. Please stamp :)