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

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

On Wed, Sep 6, 2017 at 10:28 PM, Davide Italiano <davide at freebsd.org>
wrote:
> 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.
>
My first priority is fixing the old API so that we can release LDC with
ThinLTO working for the stdlib on current OSX setups. I'll probably stick
with adding the size to the identifier for simplicity (we have our own fork
of LLVM for releasing).
>From what I can tell from online sources, latest ld64 (274.2) is using the
old api.

> CC:ed some Mach-O people, they probably know more about this than I do.
>
Thanks.

- Johan
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170907/7d22e7dd/attachment.html>

On Wed, Sep 6, 2017 at 1:28 PM, Davide Italiano via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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).
>
AFAIK, no. Mehdi started to look at this awhile back but likely has not
been able to pick it back up.

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

Right, the linker can compose the new identifier. The proposed fix
in ThinLTOCodeGenerator.cpp could be a partial workaround until linkers do
this.

Agree that it should be documented, at the very least in the header file
interfaces to the new/old LTO APIs. Not sure there is a better place to
document?

Teresa



CC:ed some Mach-O people, they probably know more about this than I
do.
>
> Thanks,
>
> --
> Davide
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
<(408)%20460-2413>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170907/d7626ea9/attachment.html>

Hi Johan,

ld64 only calls functions from llvm/include/llvm-c/lto.h  (defined
in llvm/tools/lto/lto.cpp)

For instance ThinLTOCodeGenerator::addModule is called
through thinlto_codegen_add_module().

Apple hasn't released the code for ld64 in Xcode 9 yet, did you check if it
is fixed in Xcode 9?
(I think I remember fixing it in ld64 but I'm not totally sure...).
>From what I can see with Xcode 8.2, the linker just passes the file name
(prefixed with the archive name):
https://github.com/michaelweiser/ld64/blob/master/src/ld/parsers/lto_file.cpp#L546
(original here:
https://opensource.apple.com/source/ld64/ld64-274.2/src/ld/parsers/lto_file.cpp.auto.html
)

We could workaround this in ThinLTOCodeGenerator by adding a incremental
suffix to every new buffer. Something like this diff:

diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index ffd78dad9228..d6e5d4d0c213 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -535,7 +535,9 @@ static void initTMBuilder(TargetMachineBuilder
&TMBuilder,
 } // end anonymous namespace

 void ThinLTOCodeGenerator::addModule(StringRef Identifier, StringRef Data)
{
-  ThinLTOBuffer Buffer(Data, Identifier);
+  std::string Id +      (Twine(Identifier) + "_" +
std::to_string(Modules.size())).str();
+  ThinLTOBuffer Buffer(Data, std::move(Id));
   LLVMContext Context;
   StringRef TripleStr;
   ErrorOr<std::string> TripleOrErr = expectedToErrorOrAndEmitErrors(




-- 
Mehdi


2017-09-07 8:18 GMT-07:00 Teresa Johnson <tejohnson at google.com>:
>
>
> On Wed, Sep 6, 2017 at 1:28 PM, Davide Italiano via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> 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).
>>
>
> AFAIK, no. Mehdi started to look at this awhile back but likely has not
> been able to pick it back up.
>
>
>> 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.
>
>
> Right, the linker can compose the new identifier. The proposed fix
> in ThinLTOCodeGenerator.cpp could be a partial workaround until linkers do
> this.
>
> Agree that it should be documented, at the very least in the header file
> interfaces to the new/old LTO APIs. Not sure there is a better place to
> document?
>
> Teresa
>
>
>
> CC:ed some Mach-O people, they probably know more about this than I do.
>>
>> Thanks,
>>
>> --
>> Davide
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170907/f6d35f03/attachment.html>