Mehdi AMINI via llvm-dev
2017-Sep-07 15:44 UTC
[llvm-dev] [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>
Johan Engelen via llvm-dev
2017-Sep-08 19:04 UTC
[llvm-dev] [ThinLTO] static library failure with object files with the same name
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>
Johan Engelen via llvm-dev
2017-Sep-11 21:21 UTC
[llvm-dev] [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>