Peter Collingbourne via llvm-dev
2017-May-21 22:23 UTC
[llvm-dev] RFC: A new llvm-dlltool driver and llvm-lib driver improvements
On Sun, May 21, 2017 at 3:11 PM, Martell Malone <martellmalone at gmail.com> wrote:> Hey Peter, > > How is crt0_c.c being added to the link? If crt0_c.c is supplying a >> definition of the main function I would expect it to be in an archive which >> would appear after the user supplied inputs on the command line. Archives >> and objects appearing earlier in the link have priority over archives >> appearing later, so the user's definition of main should then have priority >> over the CRT's definition. However, objects added to the link have equal >> priority, so if crt0_c.c is being added directly as an object file I would >> expect it to cause a duplicate symbol error, as its definition of main >> would conflict with the user's definition. > > > crt0_c.c is not being added directly to the linker, this is when linking a > simple hello world program with libmingwex.a and the other MS libraries. >A few months ago I was able to alter mingw-w64 to be able to link with msvc> link.exe, I will try pull out those patches again to produce a link repo. > It should be much clearer when MSVC links it and lld doesn't but I do feel > as though this is definitely a regression. >So the crt0_c.c object file is a member of libmingwex.a? If so, this does sound like a regression to me. Please do send the repro once you have something that links with the MSVC linker and fails with lld. Peter> > > We should probably implement /FORCE:MULTIPLE for MSVC compatibility, but I >> don't think you should be passing it from your driver, as that would just >> be working around the bug that was presumably introduced by r289280. If you >> just want to get past this issue though, implementing it would probably >> just involve downgrading the fatal error in SymbolTable::reportDuplicate to >> a warning if /FORCE:MULTIPLE is passed. > > I worked around it earlier by commenting out the path that result in an > error on duplicates earlier. > This isn't ideal of course as you mentioned. > > Best, > Martell > > > On Sun, May 21, 2017 at 9:51 PM, Peter Collingbourne <peter at pcc.me.uk> > wrote: > >> Hi Martell, >> >> r289280 was not intended to be a significant functional change in the >> sense that it would cause programs to fail to link, so this may be a bug I >> introduced in r289280 (or one of the followup patches, which also changed >> link order). >> >> How is crt0_c.c being added to the link? If crt0_c.c is supplying a >> definition of the main function I would expect it to be in an archive which >> would appear after the user supplied inputs on the command line. Archives >> and objects appearing earlier in the link have priority over archives >> appearing later, so the user's definition of main should then have priority >> over the CRT's definition. However, objects added to the link have equal >> priority, so if crt0_c.c is being added directly as an object file I would >> expect it to cause a duplicate symbol error, as its definition of main >> would conflict with the user's definition. >> >> Note that this is all how I would expect things to work both before and >> after my change. The priority that existed before was just an >> implementation detail; priority is now implicit in the link order (i.e. the >> order in which files appear in the linker's command line, roughly speaking). >> >> We should probably implement /FORCE:MULTIPLE for MSVC compatibility, but >> I don't think you should be passing it from your driver, as that would just >> be working around the bug that was presumably introduced by r289280. If you >> just want to get past this issue though, implementing it would probably >> just involve downgrading the fatal error in SymbolTable::reportDuplicate to >> a warning if /FORCE:MULTIPLE is passed. >> >> Peter >> >> On Sun, May 21, 2017 at 7:05 AM, Martell Malone <martellmalone at gmail.com> >> wrote: >> >>> Hey Peter, >>> >>> Currently I still use the custom frontend I have for using lld so it >>> wouldn't be much use to do that. >>> I'm looking to get all the other components merged before I submit an in >>> process shim like Rui suggested. >>> >>> I think the best way to approach this would be from MSVC terms. >>> >>> Before rL289280 we had some sort of priority for deciding on which >>> symbol to link. >>> >>> MSVC Link.exe defines a flag called /FORCE >>> https://msdn.microsoft.com/en-us/library/70abkas3.aspx >>> We currently implement /FORCE:UNRESOLVED from what I see in the driver. >>> I would need /FORCE:MULTIPLE to have this functionality back. >>> >>> I'm not sure how we would go about implementing that since that commit >>> brings us further away from that. >>> Feedback on what we should do for /FORCE:MULTIPLE would be a good place >>> to start. >>> >>> Best, >>> Martell >>> >>> >>> On Thu, May 11, 2017 at 6:41 PM, Peter Collingbourne <peter at pcc.me.uk> >>> wrote: >>> >>>> Would it be possible to share a repro.tar file created with /linkrepro >>>> so that we can reproduce the problem? >>>> >>>> Peter >>>> >>>> On Thu, May 11, 2017 at 3:00 AM, Martell Malone < >>>> martellmalone at gmail.com> wrote: >>>> >>>>> There are a few things running in parallel and thanks for taking the >>>>> time to review and help me get this in tree. >>>>> I wanted to follow up with a question on the linker side of things. >>>>> >>>>> Since rL289280 I can no longer link programs with the mingw-crt >>>>> because of dup function errors of the main function. >>>>> >>>>> In mingw-w64-crt/crt/crtexe.c a main function is called >>>>> from __tmainCRTStartup >>>>> mingw-w64-crt/crt/crt0_c.c defines this main and calls WinMain in the >>>>> case where main does not exist. >>>>> What used to happen before was the crt0_c.c main would be ignored at >>>>> link time because of priority on the users main function but now I just get >>>>> a dup symbol error as there is no checking on priority anymore. >>>>> >>>>> >>>>> On Mon, Mar 27, 2017 at 7:00 PM, Martell Malone < >>>>> martellmalone at gmail.com> wrote: >>>>> >>>>>> Just to follow up here I found some time to progress. >>>>>> Anyone who wants to follow along can see here. >>>>>> https://reviews.llvm.org/D29892 >>>>>> >>>>>> Kind Regards >>>>>> Martell >>>>>> >>>>>> On Tue, Feb 14, 2017 at 2:21 AM, Rui Ueyama <ruiu at google.com> wrote: >>>>>> >>>>>>> The code in LLD is somewhat tightly coupled with the rest of the >>>>>>> linker. Probably it is better to create a new library rather than trying to >>>>>>> split LLD into two. I believe this would result in a better API. Of course >>>>>>> you can reuse code, but what we need is a clean interface. >>>>>>> >>>>>>> The library shouldn't be complicated. In fact, it would export only >>>>>>> one function. >>>>>>> >>>>>>> std::vector<uint8_t> createImportLibrary(std::vecto >>>>>>> r<ExportSymbol>) >>>>>>> >>>>>>> I.e. it takes a list of DLLExported symbols and returns a .lib file. >>>>>>> ExportSymbol type contains information about DLLExported symbols. >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> -- >>>> Peter >>>> >>> >>> >> >> >> -- >> -- >> Peter >> > >-- -- Peter -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170521/d0594fdd/attachment.html>
Martell Malone via llvm-dev
2017-Jul-24 18:05 UTC
[llvm-dev] RFC: A new llvm-dlltool driver and llvm-lib driver improvements
Hey Peter, Wanted to follow up here after getting this test case setup on MSVC link.exe over the weekend. When I updated LLD to the latest trunk the problem seems to have resolved itself. I haven't gotten a decent chance to check through each iteration of the history to find the commit yet but I will move back on to getting a mingw driver into LLD (hopefully this coming weekend) to make sure we have not only a driver but also test cases to ensure this stays working. For anyone interested, llvm-dlltool landed in rL308379 which was before the 5.0 branch thanks to Rui for his patience on getting this merged. Best, Martell On Sun, May 21, 2017 at 11:23 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:> On Sun, May 21, 2017 at 3:11 PM, Martell Malone <martellmalone at gmail.com> > wrote: > >> Hey Peter, >> >> How is crt0_c.c being added to the link? If crt0_c.c is supplying a >>> definition of the main function I would expect it to be in an archive which >>> would appear after the user supplied inputs on the command line. Archives >>> and objects appearing earlier in the link have priority over archives >>> appearing later, so the user's definition of main should then have priority >>> over the CRT's definition. However, objects added to the link have equal >>> priority, so if crt0_c.c is being added directly as an object file I would >>> expect it to cause a duplicate symbol error, as its definition of main >>> would conflict with the user's definition. >> >> >> crt0_c.c is not being added directly to the linker, this is when linking >> a simple hello world program with libmingwex.a and the other MS libraries. >> > A few months ago I was able to alter mingw-w64 to be able to link with >> msvc link.exe, I will try pull out those patches again to produce a link >> repo. >> It should be much clearer when MSVC links it and lld doesn't but I do >> feel as though this is definitely a regression. >> > > So the crt0_c.c object file is a member of libmingwex.a? If so, this does > sound like a regression to me. Please do send the repro once you have > something that links with the MSVC linker and fails with lld. > > Peter > >> >> >> We should probably implement /FORCE:MULTIPLE for MSVC compatibility, but >>> I don't think you should be passing it from your driver, as that would just >>> be working around the bug that was presumably introduced by r289280. If you >>> just want to get past this issue though, implementing it would probably >>> just involve downgrading the fatal error in SymbolTable::reportDuplicate to >>> a warning if /FORCE:MULTIPLE is passed. >> >> I worked around it earlier by commenting out the path that result in an >> error on duplicates earlier. >> This isn't ideal of course as you mentioned. >> >> Best, >> Martell >> >> >> On Sun, May 21, 2017 at 9:51 PM, Peter Collingbourne <peter at pcc.me.uk> >> wrote: >> >>> Hi Martell, >>> >>> r289280 was not intended to be a significant functional change in the >>> sense that it would cause programs to fail to link, so this may be a bug I >>> introduced in r289280 (or one of the followup patches, which also changed >>> link order). >>> >>> How is crt0_c.c being added to the link? If crt0_c.c is supplying a >>> definition of the main function I would expect it to be in an archive which >>> would appear after the user supplied inputs on the command line. Archives >>> and objects appearing earlier in the link have priority over archives >>> appearing later, so the user's definition of main should then have priority >>> over the CRT's definition. However, objects added to the link have equal >>> priority, so if crt0_c.c is being added directly as an object file I would >>> expect it to cause a duplicate symbol error, as its definition of main >>> would conflict with the user's definition. >>> >>> Note that this is all how I would expect things to work both before and >>> after my change. The priority that existed before was just an >>> implementation detail; priority is now implicit in the link order (i.e. the >>> order in which files appear in the linker's command line, roughly speaking). >>> >>> We should probably implement /FORCE:MULTIPLE for MSVC compatibility, but >>> I don't think you should be passing it from your driver, as that would just >>> be working around the bug that was presumably introduced by r289280. If you >>> just want to get past this issue though, implementing it would probably >>> just involve downgrading the fatal error in SymbolTable::reportDuplicate to >>> a warning if /FORCE:MULTIPLE is passed. >>> >>> Peter >>> >>> On Sun, May 21, 2017 at 7:05 AM, Martell Malone <martellmalone at gmail.com >>> > wrote: >>> >>>> Hey Peter, >>>> >>>> Currently I still use the custom frontend I have for using lld so it >>>> wouldn't be much use to do that. >>>> I'm looking to get all the other components merged before I submit an >>>> in process shim like Rui suggested. >>>> >>>> I think the best way to approach this would be from MSVC terms. >>>> >>>> Before rL289280 we had some sort of priority for deciding on which >>>> symbol to link. >>>> >>>> MSVC Link.exe defines a flag called /FORCE >>>> https://msdn.microsoft.com/en-us/library/70abkas3.aspx >>>> We currently implement /FORCE:UNRESOLVED from what I see in the driver. >>>> I would need /FORCE:MULTIPLE to have this functionality back. >>>> >>>> I'm not sure how we would go about implementing that since that commit >>>> brings us further away from that. >>>> Feedback on what we should do for /FORCE:MULTIPLE would be a good place >>>> to start. >>>> >>>> Best, >>>> Martell >>>> >>>> >>>> On Thu, May 11, 2017 at 6:41 PM, Peter Collingbourne <peter at pcc.me.uk> >>>> wrote: >>>> >>>>> Would it be possible to share a repro.tar file created with /linkrepro >>>>> so that we can reproduce the problem? >>>>> >>>>> Peter >>>>> >>>>> On Thu, May 11, 2017 at 3:00 AM, Martell Malone < >>>>> martellmalone at gmail.com> wrote: >>>>> >>>>>> There are a few things running in parallel and thanks for taking the >>>>>> time to review and help me get this in tree. >>>>>> I wanted to follow up with a question on the linker side of things. >>>>>> >>>>>> Since rL289280 I can no longer link programs with the mingw-crt >>>>>> because of dup function errors of the main function. >>>>>> >>>>>> In mingw-w64-crt/crt/crtexe.c a main function is called >>>>>> from __tmainCRTStartup >>>>>> mingw-w64-crt/crt/crt0_c.c defines this main and calls WinMain in the >>>>>> case where main does not exist. >>>>>> What used to happen before was the crt0_c.c main would be ignored at >>>>>> link time because of priority on the users main function but now I just get >>>>>> a dup symbol error as there is no checking on priority anymore. >>>>>> >>>>>> >>>>>> On Mon, Mar 27, 2017 at 7:00 PM, Martell Malone < >>>>>> martellmalone at gmail.com> wrote: >>>>>> >>>>>>> Just to follow up here I found some time to progress. >>>>>>> Anyone who wants to follow along can see here. >>>>>>> https://reviews.llvm.org/D29892 >>>>>>> >>>>>>> Kind Regards >>>>>>> Martell >>>>>>> >>>>>>> On Tue, Feb 14, 2017 at 2:21 AM, Rui Ueyama <ruiu at google.com> wrote: >>>>>>> >>>>>>>> The code in LLD is somewhat tightly coupled with the rest of the >>>>>>>> linker. Probably it is better to create a new library rather than trying to >>>>>>>> split LLD into two. I believe this would result in a better API. Of course >>>>>>>> you can reuse code, but what we need is a clean interface. >>>>>>>> >>>>>>>> The library shouldn't be complicated. In fact, it would export only >>>>>>>> one function. >>>>>>>> >>>>>>>> std::vector<uint8_t> createImportLibrary(std::vecto >>>>>>>> r<ExportSymbol>) >>>>>>>> >>>>>>>> I.e. it takes a list of DLLExported symbols and returns a .lib >>>>>>>> file. ExportSymbol type contains information about DLLExported symbols. >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> -- >>>>> Peter >>>>> >>>> >>>> >>> >>> >>> -- >>> -- >>> Peter >>> >> >> > > > -- > -- > Peter >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170724/3e9a6cbe/attachment.html>