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

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>

Hi Johan,

2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.engelen at gmail.com>:
> 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/sr
>>> c/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)
>
>
It is a matter of API contract, so I'm not sure I perceive a clear
right/wrong here. If we want to enforce an API contract on the caller's
responsibility to provide a unique ID, then the assert may be OK (however
we disable assertions in production and it means random crashes which isn't
super user-friendly).
On the other hand my patch here was trying to be resilient to API misuse
for "little cost".

Note that the plan was that this API (and the ThinLTOCodegenerator.cpp)
should just die, but unfortunately I haven't had time to work on the
replacement (a C binding for the new LTO API).

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170911/0e5296a6/attachment.html>

I've created a review for your patch Mehdi: https://reviews.llvm.org/D37961

First time using `arc`, so hope things went well.

- Johan


On Tue, Sep 12, 2017 at 5:25 AM, Mehdi AMINI <joker.eph at gmail.com>
wrote:
> Hi Johan,
>
> 2017-09-11 14:21 GMT-07:00 Johan Engelen <jbc.engelen at gmail.com>:
>
>> 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/sr
>>>> c/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)
>>
>>
> It is a matter of API contract, so I'm not sure I perceive a clear
> right/wrong here. If we want to enforce an API contract on the caller's
> responsibility to provide a unique ID, then the assert may be OK (however
> we disable assertions in production and it means random crashes which
isn't
> super user-friendly).
> On the other hand my patch here was trying to be resilient to API misuse
> for "little cost".
>
> Note that the plan was that this API (and the ThinLTOCodegenerator.cpp)
> should just die, but unfortunately I haven't had time to work on the
> replacement (a C binding for the new LTO API).
>
> --
> Mehdi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170917/64cfb9d6/attachment.html>