llvm dev - May 2017 - RFC: A new llvm-dlltool driver and llvm-lib driver improvements

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::vector<ExportSymbol>)
>>>>>
>>>>> I.e. it takes a list of DLLExported symbols and returns a
.lib file.
>>>>> ExportSymbol type contains information about DLLExported
symbols.
>>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> --
>> Peter
>>
>
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170521/45915e86/attachment.html>

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.

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::vector<ExportSymbol>)
>>>>>>
>>>>>> I.e. it takes a list of DLLExported symbols and returns
a .lib file.
>>>>>> ExportSymbol type contains information about
DLLExported symbols.
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>> --
>>> --
>>> Peter
>>>
>>
>>
>
>
> --
> --
> Peter
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170521/6c179cfc/attachment.html>

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>