This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Fix an issue where Objective-C symbols where not force-loaded due to LC_LINKER_OPTION
Needs RevisionPublic

Authored by tt on Jun 15 2022, 11:31 PM.

Details

Reviewers
smeenai
ributzka
int3
thakis
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

tt created this revision.Jun 15 2022, 11:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 15 2022, 11:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
tt requested review of this revision.Jun 15 2022, 11:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 11:31 PM
int3 added a subscriber: int3.Jun 16 2022, 6:03 AM

Can you make the commit message more detailed? It's 1) nice not to make people go to GH issues to understand the changes 2) the description in the issue isn't super clear as-is (I understood the diff better after looking at the test case)

While refreshing my memory of how -force_load worked, I realized we lacked a test case covering what happens if a library is loaded as a regular (Default) load and then subsequently force-loaded. It looks like in that case ld64 doesn't force-load it (so our current implementation is behaving correctly in that regard). I'll put up a diff with those tests in a bit.

lld/MachO/Driver.cpp
250–251

how about having a single map of StringRef -> {ForceLoad, ArchiveFile *}, and then perform a load / update the map only if the existing entry has ForceLoad::No but the new load is ForceLoad::Yes or Default?

lld/test/MachO/lc-linker-option.ll
81

explicitly

95

explicitly

106

can we add a test case for the -force_load + LC_LINKER_OPTION combination?

thakis resigned from this revision.Jul 22 2022, 8:39 AM
thakis added a subscriber: thakis.

this can be closed, something else landed elsewhere

smeenai requested changes to this revision.Aug 27 2022, 12:28 AM
smeenai added a subscriber: smeenai.

Placing back in author's queue to abandon

This revision now requires changes to proceed.Aug 27 2022, 12:28 AM