[PATCH] D88944: [ELF] Skip repeated libraries.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D88944#2316057 <https://reviews.llvm.org/D88944#2316057>, @ikudrin wrote:

> In D88944#2316011 <https://reviews.llvm.org/D88944#2316011>, @MaskRay wrote:
>
>> I am skeptical about the change.
>>
>> - The duplicated libraries are usually indicator of user errors or build system brittleness.
>
> The provided optimization is aimed to slightly improve the dependent libraries' case, where lots (hundreds) of references is very common. In a regular link, its impact is not expected to be noticeable.
>
>> - The current way the state is introduced is incompatible with the direction on LLD-as-a-library (D88348 <https://reviews.llvm.org/D88348>) (This can be fixed).
>
> Do you mean that `addedLibraries` is not reset on reenter? If so, the same is true for other members of `LinkerDriver` as well, so the patch does not make the situation worse.
>
>> - The --verbose now diverges from GNU ld and gold. (This is usually a tie-breaker instead of a decisive reason.)
>
> Well, we should not imitate GNU linkers in every nuance, especially in the internals, aren't we?
>
>> - When duplicated libraries happen, the archive index can guarantee that the performance is not degraded.
>
> The more repeated libraries referenced, the more time and memory the optimization saves. Even reading and basically parsing the archive files takes some time, and if there are hundreds of them, the improvement becomes noticeable. Of course, saving is not huge, but why not have it if the implementation is cheap?

The implementation may look cheap but "the provided optimization is aimed to slightly improve the dependent libraries' case" The benefit may not be noticeable. So far I am on the fence. However, then there are a few downsides including the incompatibility with --warn-backrefs and --verbose output. (The `!config->formatBinary && !inWholeArchive` condition looks subtle and can be error-prone as well)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88944