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

Mon Sep 11 14:21:55 PDT 2017

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>