Re: [PATCH] ld: Override default linker emulation at configure time

[Cupertino Miranda <Cupertino dot Miranda at synopsys dot com>]

Hi Andrew,

Fair, over-engineering was an over-statement. ;-)

I was simply, advocating for the triplet use in this case, however, I do
agree with your reasoning.
> My two mains dislikes for switching on the vendor string are, first,
> it ties us to the vendor string, what if we don't want to use the
> vendor name in the target for whatever reason, or what if the vendor
> name changes in the future?  Second, having worked so hard to make the
> configuration NOT depend on the vendor string in all other parts of
> the toolchain, having the linker be the odd-one out feels like a
> likely source of confusion.  Some end user builds the toolchain with
> --with-cpu=nps400, but miss-spells, or drops the vendor string.
> Everything else has configured/built correctly, except for the default
> emulation.
>
I was also with the impression that:
 - there was some other patch to expose the "with_cpu" variable and
 - that no other target checked more than the triplet (I now see that
powerpc*-*-elf* already does something).

Just a suggestion, they do it with a "case" construct instead of the "if
then else" ... seems cleaner. :-)

Best regards,
Cupertino

On 01/26/2017 03:55 PM, Andrew Burgess wrote:
> Cupertino,
>
> Thanks for taking the time to look through this patch.  I always
> appreciate your feedback.
>
> * Cupertino Miranda <Cupertino.Miranda@synopsys.com> [2017-01-26 13:15:19 +0000]:
>
>> I apologize for just coming with such a question now. I don't really
>> know how I missed this thread.
>> But why not to specify the triplet with melanox to use the particular
>> emulation?
>>
>> IMHO, although it is discouraged, as a general rule, to use the vendor
>> name to classify a target, in this particular case it makes total sense
>> to me.
>> It is clear that no one else apart from melanox will ever be using this
>> particular linker script.
>>
>> Moreover, the required over enginering for the default ARC configure
>> process looks to me like a very good reason for to break that rule.
> It's not clear which part seems over-engineered, could you explain in
> a little more detail what your concerns are please?
>
> When I think of over-engineering I think of introducing overly complex
> solutions in order to solve simple problems.  Maybe inventing a new
> solution when a solution already exists and can be reused.
>
> As an example, adding a whole new configuration switch like
> '--default-linker-emulation=NAME' in order to solve this problem might
> be seen as overly complex (if someone was ever silly enough to suggest
> such a solution) while reusing an existing solution (say
> --with-cpu=NAME) would be much simpler.
>
>> The second thing is that, there is already other target / vendor in
>> configure.tgt that does this for the same reason:
>>     vax-dec-ultrix* | vax-dec-bsd*) targ_emul=vax ;;
>>
>> If we follow the path to check the "with_cpu" flag on the configure.tgt,
>> we will also be doing something that certainly should be avoided and
>> perhaps lead to future problems.
> Can you expand on this too please.  Why should this be avoided?
>
> I and Claudiu have both invested effort in making sure that
> --with-cpu=NAME works for the ARC toolchain in both GCC and gas
> (though to be clear Claudiu has not done any --with-cpu= work for ld)
> however the --with-cpu switch is a well established cpu switch within
> binutils and GCC, and given that both Synopsys and others have a
> proven interest in this feature working (for ARC) it seems unlikely
> that it's going to go away anytime soon.
>
> My two mains dislikes for switching on the vendor string are, first,
> it ties us to the vendor string, what if we don't want to use the
> vendor name in the target for whatever reason, or what if the vendor
> name changes in the future?  Second, having worked so hard to make the
> configuration NOT depend on the vendor string in all other parts of
> the toolchain, having the linker be the odd-one out feels like a
> likely source of confusion.  Some end user builds the toolchain with
> --with-cpu=nps400, but miss-spells, or drops the vendor string.
> Everything else has configured/built correctly, except for the default
> emulation.
>
>> Moreover, ARC will be the only target to do this kind of thing.
> There are lots of things that ARC is doing that others are not, like
> the table based features that you (and Claudiu) are/have introduced.
>
> Lets think of us as leading the way, hopefully everyone else will
> follow our lead!
>
> Thanks,
> Andrew
>
>
>> Best regards,
>> Cupertino
>>
>> On 01/26/2017 11:41 AM, Andrew Burgess wrote:
>>> * Alan Modra <amodra@gmail.com> [2017-01-25 13:22:48 +1030]:
>>>
>>>> On Tue, Jan 24, 2017 at 10:55:25AM +0000, Andrew Burgess wrote:
>>>>> I might end up having to add some code to ld/configure.ac to make
>>>>> --with-cpu=NAME visible in the ld configure script.... but how would
>>>>> this look to you?
>>>> Works for me.
>>> Thanks for all your time reviewing this patch.
>>>
>>> Here's a revised version in line with the previous discussion.
>>>
>>> Thanks,
>>> Andrew
>>>
>>> ---
>>>
>>> If we are configuring for an arc/linux target, and --with-cpu=nps400 is
>>> used at configure time then change the default linker emulation to the
>>> nps specific version.  All of the alternative linker emulations are
>>> still available using the -mNAME option for ld.
>>>
>>> ld/ChangeLog:
>>>
>>> 	* configure.tgt (arc*-*-linux*): Change the default linker
>>> 	emulation based on --with-cpu selection.
>>> 	* NEWS: Mention new configuration option.
>>> ---
>>>  ld/ChangeLog     |  6 ++++++
>>>  ld/NEWS          |  3 +++
>>>  ld/configure.tgt | 10 ++++++++--
>>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ld/NEWS b/ld/NEWS
>>> index d43f846..23ca25a 100644
>>> --- a/ld/NEWS
>>> +++ b/ld/NEWS
>>> @@ -2,6 +2,9 @@
>>>  
>>>  * Add support for the Texas Instruments PRU processor.
>>>  
>>> +* When configuring for arc*-*-linux* targets the default linker emulation will
>>> +  change if --with-cpu=nps400 is used at configure time.
>>> +
>>>  Changes in 2.28:
>>>  
>>>  * The EXCLUDE_FILE linker script construct can now be applied outside of the
>>> diff --git a/ld/configure.tgt b/ld/configure.tgt
>>> index 5a68083..cfb2689 100644
>>> --- a/ld/configure.tgt
>>> +++ b/ld/configure.tgt
>>> @@ -80,8 +80,14 @@ alpha*-*-*vms*)		targ_emul=alphavms
>>>  arc*-*-elf*)		targ_emul=arcelf
>>>  			targ_extra_emuls="arcelf_prof arclinux arclinux_nps arclinux_prof arcv2elf arcv2elfx"
>>>              		;;
>>> -arc*-*-linux*)		targ_emul=arclinux
>>> -			targ_extra_emuls="arclinux_nps arclinux_prof arcelf arcelf_prof arcv2elf arcv2elfx"
>>> +arc*-*-linux*)		if test "${with_cpu}" = "nps400"; then
>>> +			   targ_emul=arclinux_nps
>>> +			   targ_extra_emuls=arclinux
>>> +			else
>>> +			   targ_emul=arclinux
>>> +			   targ_extra_emuls=arclinux_nps
>>> +			fi
>>> +			targ_extra_emuls="${targ_extra_emuls} arclinux_prof arcelf arcelf_prof arcv2elf arcv2elfx"
>>>  			;;
>>>  arm-epoc-pe)		targ_emul=arm_epoc_pe ;	targ_extra_ofiles="deffilep.o pe-dll.o" ;;
>>>  arm*-*-cegcc*)		targ_emul=arm_wince_pe ; targ_extra_ofiles="deffilep.o pe-dll.o"
>>