[PATCH] D32721: Accept archive files with no symbol table instad of warning on them.

Wed May 3 18:12:07 PDT 2017

2017-05-03 18:07 GMT-07:00 Rui Ueyama <ruiu at google.com>:

> On Wed, May 3, 2017 at 5:58 PM, Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>>
>> On Wed, May 3, 2017 at 5:51 PM Rui Ueyama <ruiu at google.com> wrote:
>>
>>> On Wed, May 3, 2017 at 5:48 PM, Mehdi AMINI <joker.eph at gmail.com> wrote:
>>>
>>>>
>>>> On Wed, May 3, 2017 at 5:44 PM Rui Ueyama <ruiu at google.com> wrote:
>>>>
>>>>> On Wed, May 3, 2017 at 5:40 PM, Mehdi AMINI <joker.eph at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Wed, May 3, 2017 at 5:26 PM Rui Ueyama <ruiu at google.com> wrote:
>>>>>>
>>>>>>> On Wed, May 3, 2017 at 5:13 PM, Mehdi AMINI <joker.eph at gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Clang is incrementally linking in a matter of a few seconds, so
>>>>>>>> 0.5s to read the symbols is a double digit percentage of that.
>>>>>>>> And there are over 50 binaries in LLVM, not just one.
>>>>>>>>
>>>>>>>
>>>>>>> We do not support incremental linking,
>>>>>>>
>>>>>>
>>>>>> I'm talking about ThinLTO incremental linking, which we support.
>>>>>>
>>>>>
>>>>> How can that be faster than the regular build?
>>>>>
>>>>
>>>> Not sure what you mean: on my mac ld64 links clang in less than 2s.
>>>>
>>>
>>> But that is ld64. We are talking about LLD, no?
>>>
>>
>> I don't see where you're going: lld is supposed to be fast, isn't it? I
>> assume it has to be able to outspeed ld64.
>> So I'm giving you a reference of what is "a regular" build time and that
>> should explain why you .5s overhead is not trivial.
>>
>
> That is hypothetical. If LLD is already able to link Clang with ThinLTO in
> 2 seconds, it may make sense to warn on 0.5 second loss, but that's in
> reality not the case, so I'm not convinced that we should warn on it right
> now.
>

I disagree: we should define what is a *correct* build setting, and warn if
it is not honored.
Your changing this definition here, and I doubt it'll be easy to revert in
the future, which is why I don't like this.

>
>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>> but even if we support it, we don't need to read archives that
>>>>>>> haven't changed since the last build, so the overhead in that hypothetical
>>>>>>> case would be much smaller than 0.5s.
>>>>>>>
>>>>>>
>>>>>> So yes we need to read all the archives.
>>>>>>
>>>>>>
>>>>>>> And you still don't address the "principle of least surprise": the
>>>>>>>> configuration is *not* what is expected from the user.
>>>>>>>>
>>>>>>>
>>>>>>> As a naive user of LTO, I was surprised that LTO needs llvm-ar,
>>>>>>> which is certainly I didn't expect (due to lack of knowledge).
>>>>>>>
>>>>>>
>>>>>> That is why the warning is deserved.
>>>>>>
>>>>>
>>>>> But you no longer need it with this change.
>>>>>
>>>>> --
>>>>>> Mehdi
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> --
>>>>>>>> Mehdi
>>>>>>>>
>>>>>>>>
>>>>>>>> 2017-05-03 16:51 GMT-07:00 Rui Ueyama <ruiu at google.com>:
>>>>>>>>
>>>>>>>>> The cost of reading symbols from object files in archive files is
>>>>>>>>> probably much cheaper than you might be thinking. If I strip all symbols
>>>>>>>>> from archives from a clang debug build, LLD takes 8.16 seconds to link,
>>>>>>>>> while it can usually link it in 7.65 seconds. So the difference is only 0.5
>>>>>>>>> seconds, and clang is a fairly large program as a test. That test case uses
>>>>>>>>> ELF, but with Peter's patch I believe reading symbols from bitcode files is
>>>>>>>>> fast too.
>>>>>>>>>
>>>>>>>>> To me 0.5 seconds is too small that I want the tool to "just work"
>>>>>>>>> instead of annoy me every time I run make/ninja until I change the build
>>>>>>>>> configuration to shave off 0.5 seconds from a LTO build.
>>>>>>>>>
>>>>>>>>> On Wed, May 3, 2017 at 4:23 PM, Mehdi AMINI via Phabricator <
>>>>>>>>> reviews at reviews.llvm.org> wrote:
>>>>>>>>>
>>>>>>>>>> mehdi_amini added a comment.
>>>>>>>>>>
>>>>>>>>>> I personally  think *not* warn is a terrible thing to do when
>>>>>>>>>> there is a configuration issue. Erroring is annoying, but warning should be
>>>>>>>>>> intended in such cases!
>>>>>>>>>>
>>>>>>>>>> > True, but on the other hand, it's pretty much the exact same
>>>>>>>>>> work that the archiver would need to do,
>>>>>>>>>>
>>>>>>>>>> The archiver do it once for potentially a lot of linker
>>>>>>>>>> invocations.
>>>>>>>>>>
>>>>>>>>>> > and asking the user to change their archiver and rebuild would
>>>>>>>>>> probably consume even more time.
>>>>>>>>>>
>>>>>>>>>> This is a one time thing, and the user can live with the warning
>>>>>>>>>> (or pass a flag to disable the warning maybe) if they choose to.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Repository:
>>>>>>>>>>   rL LLVM
>>>>>>>>>>
>>>>>>>>>> https://reviews.llvm.org/D32721
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170503/2d1278bd/attachment.html>