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

Thu May 4 16:12:02 PDT 2017

On Thu, May 4, 2017 at 3:58 PM, Mehdi AMINI <joker.eph at gmail.com> wrote:

>
>
> 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).
>

IIRC, I've tried to use those and they didn't work. My memory is vague, but
I think the issue was that CMAKE_AR and CMAKE_RANLIB were not propagated to
all configuration checks, so that some of them spuriously failed because
the CFLAGS had -flto but the CMAKE_AR/CMAKE_RANLIB was not being respected.

-- Sean Silva

>
>
>
>>
>> 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/02c49472/attachment.html>