llvm dev - Sep 2017 - [ThinLTO] static library failure with object files with the same name

On Tue, Sep 5, 2017 at 2:09 PM, Teresa Johnson <tejohnson at google.com>
wrote:
>
> Hi Johan,
>
> Right, per the bug this is fixed in lld (and was already handled in
gold-plugin), but I guess not in ld64. Note that lld and gold-plugin use the new
LTO API, while ld64 (and probably other linkers) are still using the legacy
libLTO (which is what ThinLTOCodeGenerator.cpp is part of). Fixing it in the
location you propose could work for all legacy libLTO users. But I don't
think that adding just the size will (always) be enough to disambiguate
(couldn't the 2 same named members have the same size?) - although lld is
doing the same thing so this may be as good as what is done there. For
gold-plugin we add the byte offset into the archive where the member starts,
which will be unique.
> +davide for thoughts since he fixed it on the lld side.
>
Yes, Teresa is right, this is the correct fix.
I'm not sure what subset of GNU archives Mach-O supports, but the only
way of being safe is using offset in the archive + archive name.
Fun fact, you apparently can have a single GNU archive with two
different members with the same name. Using the offset is the only way
 to disambiguate.
I think we should really consider documenting this assumption
somewhere as many people have been bitten in the past and tracking
down is not trivial as it shows up as assertion failures in the mover
or duplicate/undefined symbols reported as linker errors, as the
ThinLTO logic will pick the an archive randomly.

On Tue, Sep 5, 2017 at 11:34 PM, Davide Italiano <dccitaliano at
gmail.com>
wrote:
> On Tue, Sep 5, 2017 at 2:09 PM, Teresa Johnson <tejohnson at
google.com>
> wrote:
> >
> > Hi Johan,
> >
> > Right, per the bug this is fixed in lld (and was already handled in
> gold-plugin), but I guess not in ld64. Note that lld and gold-plugin use
> the new LTO API, while ld64 (and probably other linkers) are still using
> the legacy libLTO (which is what ThinLTOCodeGenerator.cpp is part of).
> Fixing it in the location you propose could work for all legacy libLTO
> users. But I don't think that adding just the size will (always) be
enough
> to disambiguate (couldn't the 2 same named members have the same size?)
-
> although lld is doing the same thing so this may be as good as what is done
> there. For gold-plugin we add the byte offset into the archive where the
> member starts, which will be unique.
> > +davide for thoughts since he fixed it on the lld side.
> >
>
> Yes, Teresa is right, this is the correct fix.
> I'm not sure what subset of GNU archives Mach-O supports, but the only
> way of being safe is using offset in the archive + archive name.
>
ThinLTOCodeGenerator::addModule is called by the linker, right? (I can't
find any callers)
When it is called, we cannot retrieve the offset of the module inside the
archive, because the linker didn't tell us about it.
What we could do is retrieve the module hash from the buffer (although
perhaps it is not always there with ThinLTO?), and use that for
disambiguation?

I am assuming that we do want to assert/error on calling addModule with the
exact same module twice? Otherwise, doing a search for the identifier in
Modules vector first and disambiguate if found would work (multithreading
would need to be taken into account too I think?).

- Johan
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170906/9de731fa/attachment.html>

On Wed, Sep 6, 2017 at 1:10 PM, Johan Engelen via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> On Tue, Sep 5, 2017 at 11:34 PM, Davide Italiano <dccitaliano at
gmail.com>
> wrote:
>>
>> On Tue, Sep 5, 2017 at 2:09 PM, Teresa Johnson <tejohnson at
google.com>
>> wrote:
>> >
>> > Hi Johan,
>> >
>> > Right, per the bug this is fixed in lld (and was already handled
in
>> > gold-plugin), but I guess not in ld64. Note that lld and
gold-plugin use the
>> > new LTO API, while ld64 (and probably other linkers) are still
using the
>> > legacy libLTO (which is what ThinLTOCodeGenerator.cpp is part of).
Fixing it
>> > in the location you propose could work for all legacy libLTO
users. But I
>> > don't think that adding just the size will (always) be enough
to
>> > disambiguate (couldn't the 2 same named members have the same
size?) -
>> > although lld is doing the same thing so this may be as good as
what is done
>> > there. For gold-plugin we add the byte offset into the archive
where the
>> > member starts, which will be unique.
>> > +davide for thoughts since he fixed it on the lld side.
>> >
>>
>> Yes, Teresa is right, this is the correct fix.
>> I'm not sure what subset of GNU archives Mach-O supports, but the
only
>> way of being safe is using offset in the archive + archive name.
>
>
> ThinLTOCodeGenerator::addModule is called by the linker, right? (I
can't
> find any callers)
> When it is called, we cannot retrieve the offset of the module inside the
> archive, because the linker didn't tell us about it.
See here for the fix.
https://reviews.llvm.org/D25495
You pass the the archive name + offset to `lto::InputFile::create`,
assuming your linker uses the new LTO API (and I'm not sure whether
ld64 already switched).
The linker knows the archive name from which it's fetching the member,
as well as its offset (it asks libArchive about it, for lld).
I'm not sure how it works ld64, but of course, to get this mechanism
working, you need some linker modifications.
CC:ed some Mach-O people, they probably know more about this than I do.

Thanks,

--
Davide