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

[Rui Ueyama via llvm-commits]

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.

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