[PATCH] D127941: [lld-macho] Fix an issue where Objective-C symbols where not force-loaded due to LC_LINKER_OPTION

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

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.



================
Comment at: lld/MachO/Driver.cpp:250-251
 
-static DenseMap<StringRef, ArchiveFile *> loadedArchives;
+static DenseMap<StringRef, ArchiveFile *> lazyLoadedArchives;
+static DenseMap<StringRef, ArchiveFile *> forceLoadedArchives;
 
----------------
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`?


================
Comment at: lld/test/MachO/lc-linker-option.ll:81
+;; Make sure -all_load has effect when libraries are loaded via LC_LINKER_OPTION flags
+;; and explicily passed as well
+; RUN: %lld -all_load %t/load-framework-foo.o %t/load-library-foo.o \
----------------
explicitly


================
Comment at: lld/test/MachO/lc-linker-option.ll:95
+;; Make sure -ObjC has effect when frameworks are loaded via LC_LINKER_OPTION flags
+;; and explicily passed as well
+; RUN: %lld -ObjC %t/load-framework-foo.o %t/load-library-foo.o \
----------------
explicitly


================
Comment at: lld/test/MachO/lc-linker-option.ll:106
+; SYMS_OBJC_LOAD-EMPTY:
+
 ;; Make sure that frameworks containing object files or bitcode instead of
----------------
can we add a test case for the `-force_load` + `LC_LINKER_OPTION` combination?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127941/new/

https://reviews.llvm.org/D127941