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

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>

On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> 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...).
>
I haven't tried with Xcode 9.

>
> 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:
>
I was assuming that we do want to assert/error on calling addModule with
the exact same module twice? Otherwise your diff is nice, thanks.

- Johan


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

On Fri, Sep 8, 2017 at 9:04 PM, Johan Engelen <jbc.engelen at gmail.com>
wrote:
>
> On Thu, Sep 7, 2017 at 5:44 PM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>> 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...).
>>
>
> I haven't tried with Xcode 9.
>
>
>>
>> From what I can see with Xcode 8.2, the linker just passes the file
name
>> (prefixed with the archive name): https://github.com/michaelweis
>> er/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:
>>
>
> I was assuming that we do want to assert/error on calling addModule with
> the exact same module twice? Otherwise your diff is nice, thanks.
>
>
Hi Mehdi,
  Can you advise me?
Is it OK to not error upon the exact same module being added twice? (and
thus your patch would be good)

Thanks,
  Johan



>
>
>> 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/20170911/18ababd2/attachment.html>