[PATCH] D130975: --thinlto-full-index to index both native and bitcode objects

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D130975#3694853 <https://reviews.llvm.org/D130975#3694853>, @weiwang wrote:

>> And we lose opportunities to fix some problems for free:
>>
>> - If we are to support bitcode files in archives, we will need more options.
>> - This still misses an opportunity to properly fix "libcall symbols defined in bitcode member of an archive".
>>
>> I wish that you can give a fair consideration of D130229 <https://reviews.llvm.org/D130229> and not simply dismiss it with "it differs from in-process ThinLTO, it's not right."
>
> The concern we have is not about which linking order is right or wrong, it's rather making it consistent with its local in-process thinlto counterpart. We are kind of late in the game of distributed thinlto and we have large scale of services (20K inputs for linking) running with local thinlto for years. We can't guarantee that changing the resolution order of inputs from local to distributed thinlto won't make a difference in binary runtime behavior, and it would be a big headache to debug after everything goes production. So our current approach is to
>
> 1. Make sure that distributed thinlto thinlink uses the same inputs and order as local thinlto. This is more of the build system level change.
> 2. Make sure that distributed thinlto final link uses the same resolution order as thinlink with the help of this new option.
> 3. Verify that binaries generated from both local and distributed thinlink matches at per symbol level.

Thanks for being open. However, this is something I'd push back a bit.

See the last paragraph of https://reviews.llvm.org/D130229#3674024 : I think lld users need to ensure their build targets and the dependencies are in a decent state.
In the past, I have fixed a lot of internal builds to cooperate with the executable default `--no-allow-shlib-undefined` and later `--warn-backrefs`.
I did not add options to lld just so that my company could quickly adapt the dependency restriction features.

In addition, I suspect our current archive member extraction logic isn't ideal and we may need to revamp it. See mold which has implemented great parallel input file parsing.
Applications tied to every implementation detail of the archive member extraction is brittle and should be avoided.
Improving lld performance isn't my work priority, so I basically do it in my spare time in a slow but hopefully steady pace.
See https://discourse.llvm.org/t/parallel-input-file-parsing/60164 and a few patches from me mentioning "parallel". There have been some changes like removing `ArchiveFile`.
I think we will do similar things if parallel non-local symbol resolution can become more feasible, as long as the symbol resolution logic still makes sense.

As mentioned, the in-process ThinLTO symbol resolution probably isn't that ideal and we need to fix that, too, though probably with a low priority.
For this patch, I want to give a clear NAK unless you can come up with a reasonable example that `--thinlto-index=` + `--remapping-file=` don't work well (not a brittle archive shadow example).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130975