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

Thu May 4 15:58:38 PDT 2017

2017-05-04 15:50 GMT-07:00 Sean Silva <chisophugis at gmail.com>:

>
>
> On Wed, May 3, 2017 at 6:28 PM, Rui Ueyama via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Wed, May 3, 2017 at 6:12 PM, Mehdi AMINI <joker.eph at gmail.com> wrote:
>>
>>>
>>>
>>> 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.
>>>
>>
>> That's a valid concern, and this change certainly relaxes the definition
>> what is correct (or at least what is not incorrect). But I still think that
>> that's beneficial for users overall. You are extremely familiar with LLVM
>> LTO, but ordinary developers don't know much about LLVM or build systems
>> compared to you. Attempting to use llvm-ar took a fair amount of time even
>> to me (which I hope better than the average Unix user). If we print out a
>> warning on every linker invocation, we'd probably be teaching users to
>> ignore warnings rather than let them change the build configuration, as it
>> just works with a marginal performance difference.
>>
>
>
> In my experience (and mentioned in this thread too), changing the archiver
> is very difficult. One necessary requirement for emitting a warning is that
> it has to be actionable.
>
> For example, in CMake, I only know how to do it after looking at Rafael's
> https://github.com/espindola/llvm-scripts (and I have no idea how Rafael
> figured it out).
>

As a side note: this is suboptimal I believe, cmake has -DCMAKE_AR /
-DCMAKE_RANLIB options (just like -DCMAKE_C_COMPILER).


>
> And I can easily imagine (and have been in situations) where it's just not
> feasible to hook up an LTO-friendly archiver, which would cause users to
> just give up on LTO. So we should cater to the use case of users being in a
> permanent situation of not having an LTO-friendly archiver.
>
> So my vote is that:
>
> 1. We have an opt-in flag for linking without archive symbol tables (or
> some other suitable behavior that provides for this use case). If this
> comes at a nontrivial performance cost, the name should communicate that
> this forces the linker to do extra work so that users don't get confused
> about any slowdown.
> 2. We emit an error mentioning the opt-in flag
>
> Alternatively, if we can handle the no-symbol-table case just as
> efficiently (or nearly as much), we should just do it transparently IMO,
> but that might not be achievable.
>
>
>
>> In most LTO use cases, I believe users enable LTO only to build binaries
>> for shipping. They don't do as many LTO builds as you do. Making it "just
>> work" seems to be worth doing.
>>
>
> IIRC, one of the original goals of ThinLTO is to make it feasible for
> everyday developer Release builds to default to ThinLTO. I.e. it is not
> just "golden"/"final" builds using LTO.
>
> -- Sean Silva
>
>
>
>>
>>
>>>
>>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170504/65ef4119/attachment.html>