[PATCH] D129556: [lld-macho] Fix loading same libraries from both LC_LINKER_OPTION and command line

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Hm on seeing the latest iteration + thinking more about the problem, I'm wondering if we could simplify things a bit. This is my understanding of the requirements, correct me if I'm wrong:

1. `-ObjC` and `-force_load` only have an effect on command line arguments. LC_LINKER_OPTION libraries are only ever lazy-loaded
2. `-force_load` and `-all_load` take precedence over `-ObjC`, but only if the library hasn't already been loaded on the command line. Notably, if something has been loaded with `-ObjC`, we will never need to force-load it again later (so we should never need to compare `LoadScope::AllLoad` against `LoadScope::ObjCLoad`.)
3. Since lazy-loading an archive twice (or lazy-loading after an eager load) is redundant, we don't need to do anything if we see an LC_LINKER_OPTION of some libfoo.a if we have already loaded libfoo.a via a command-line argument
4. But if we have loaded libfoo.a via LC_LINKER_OPTION and then later see it on the command line, we need to load it if either `-ObjC` or force_load / all_load is used
5. If none of those flags are used, we could harmlessly lazy-load it a second time, but that would be unnecessary perf overhead

Prior to this fix, we only had the `ForceLoad` enum. `ForceLoad::Yes` and `ForceLoad::Default` only ever get passed to `addFile` if something is being loaded from the CLI. LC_LINKER_OPTION loads are always `ForceLoad::No`. So `addFile` already has enough information to determine whether something is a command-line load vs a LC load (although it's not super obvious from the name `ForceLoad::No` that this applies only to LC loads).

So here's my suggestion... we could rename `ForceLoad` to something more descriptive. `enum LoadType { CommandLineLoad, CommandLineForceLoad, LCLinkerLoad }` maybe. Also change `loadedArchives` to store a value of `struct { ArchiveFile *file, bool isCommandLineLoad }`. Now if we try to load an archive and find it already cached in `loadedArchives`, we only need to load it again if `isCommandLineLoad` is false and the current load is not another lazy load. In that case we can fall back to the existing logic; there isn't a need to calculate and store a `LoadScope` as `isCommandLineLoad` should be enough.

Pros of this design:

- No extra `commandLineInputs` cache
- Storing `bool isCommandLineLoad` rather than `LoadScope` makes it clear (from looking at the data structures alone) that our cache update policy depends only on whether something is being loaded from LC_LINKER_OPTION vs the CLI

Thoughts?



================
Comment at: lld/MachO/Driver.cpp:275
+        forceLoadArchive == ForceLoad::Yes) {
+      currentScope = ArchiveFile::LoadScope::AllLoad;
+    } else if (forceLoadArchive == ForceLoad::Default &&
----------------
nit: I don't know if nesting the enum members under two scopes is really necessary... why not leave it as a raw enum?


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

https://reviews.llvm.org/D129556