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

Thu May 4 16:23:30 PDT 2017

On Thu, May 4, 2017 at 4:21 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Thu, May 4, 2017 at 4:18 PM, Rui Ueyama <ruiu at google.com> wrote:
>
>> The other way of making it user-friendly is to make a change to Clang so
>> that it uses ELF (with symbol table but with no other data) as a container
>> for bitcode files, so that default ar commands can construct symbol tables
>> for them. But as it is an obvious solution, I believe you guys considered
>> it before and turned it down.
>>
>> As to the performance of symbol table-less archives, https://reviews.llvm
>> .org/D32881 makes them as fast as (or even faster than for some reason)
>> archives with symbol table.
>>
>
> Just to be clear, symbol table-less archives are faster in the
> single-threaded case too?
>

I didn't experimented that test case with this patch, but that should be
the same as it without this patch, so, no.

-- Sean Silva
>
>
>>
>> My memory is vague too, but I tried -DCMAKE_AR and found that it didn't
>> work for me.
>>
>> On Thu, May 4, 2017 at 4:12 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>>
>>>
>>> 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/1fc373b3/attachment.html>