[PATCH] D130229: [ELF] Add --thinlto-index= and --remapping-file=

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D130229#3673158 <https://reviews.llvm.org/D130229#3673158>, @wenlei wrote:

> In D130229#3672451 <https://reviews.llvm.org/D130229#3672451>, @MaskRay wrote:
>
>> In D130229#3671604 <https://reviews.llvm.org/D130229#3671604>, @tejohnson wrote:
>>
>>> In D130229#3670852 <https://reviews.llvm.org/D130229#3670852>, @wenlei wrote:
>>>
>>>> In D130229#3670843 <https://reviews.llvm.org/D130229#3670843>, @MaskRay wrote:
>>>>
>>>>> In D130229#3670802 <https://reviews.llvm.org/D130229#3670802>, @wenlei wrote:
>>>>>
>>>>>> I'm not sure how this solution works because symbol resolution before and after importing aren't guaranteed to produce same result even if input order is identical.
>>>>>>
>>>>>> Say we have foo in a.o, which references bar in b.o (lazy object). Assuming a.o imports bar, before importing, b.o gets picked because of that foo->bar reference, but the reference will be gone after importing and we may not pick b.o during final linking. The ripple effect from dropping b.o can disturb other symbol's resolution, making final link and thin link diverge.
>>>>>
>>>>> Imported functions have available_externally linkage and are not considered definitions in the IR symbol table. There is no symbol resolution difference in your example.
>>>>
>>>> I actually meant importing + inlining. After importing and inlining foo->bar, the reference to bar will be gone in a.o output from thinlto codegen, so the input for final linking won't see the reference for bar, right? Does that not change final linking symbol resolution still?
>>>
>>> This is my concern too. See discussion on https://reviews.llvm.org/D22356 (there's more discussion on llvm-commits that didn't make it into phab somehow), and the test case included there. That approach was abandoned in favor of the params file based on the extensive discussion there.
>>
>> As mentioned I had this concern but I am not convinced that forcing a non-lazy state is a necessity. With more fiddling I figured that the simple approach as implemented in this patch likely suffices.
>
> Continuing on the example I mentioned earlier with importing + inlining, the most obvious issue is that dropping b.o on final link will also drop static initializers come with that object. And that will be an observable difference between local vs dist thinlto. This is a real case we ran into. The alternative solution to include selected native obj in thinlto-index-only output doesn't have this (class of) problem.

Can you craft a test case (ideally `RUN:` form) which manifests the issue?

>> The D22356 <https://reviews.llvm.org/D22356> and D22467 <https://reviews.llvm.org/D22467> examples work with --thinlto-index= without forcing the non-lazy state.
>>
>>   % fld.lld t.o --start-lib t2.o --end-lib --start-lib t3.o --end-lib --start-lib t4.o --end-lib -t -e main -mllvm -import-instr-limit=4 --thinlto-index=a.map
>>   % fld.lld t.o.5.precodegen.bc --start-lib t2.o.5.precodegen.bc --end-lib --start-lib t3.o.5.precodegen.bc --end-lib --start-lib t4.o --end-lib -t --remapping-file=a.map -e main
>>   t.o.5.precodegen.bc
>>   t3.o.5.precodegen.bc
>>   t2.o.5.precodegen.bc
>>
>> It presumably may not work with gold because GNU ld/gold has a different archive member extraction semantics (see https://lld.llvm.org/ELF/warn_backrefs.html).
>> lld adopts macOS ld64/link.exe's **"first lazy object wins" approach and in practice has more stable behaviors.**
>> I suspect lld just needs to drop --warn-backrefs for the final native link in certain cases, but the sacrifice is suitable to me.
>> (If we re-generate an order depending on `-t` output for the final native link, --warn-backrefs will not be useful anyway.)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130229