Re: PATCH: Support --enable-gold=both --with-linker=[bfd|gold]

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Jan 5, 2010 at 8:23 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jan 5, 2010 at 11:08 AM, Ian Lance Taylor <iant@google.com> wrote:
>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 407ab59..b349633 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -311,10 +311,11 @@ esac
>>> ?# Handle --enable-gold.
>>>
>>> ?AC_ARG_ENABLE(gold,
>>> -[ ?--enable-gold ? ? ? ? ? use gold instead of ld],
>>> +[ ?--enable-gold[[=ARG]] ? ? build gold [[ARG={yes,both}]]],
>>> ?ENABLE_GOLD=$enableval,
>>> ?ENABLE_GOLD=no)
>>> -if test "${ENABLE_GOLD}" = "yes"; then
>>> +case "${ENABLE_GOLD}" in
>>> +yes|both)
>>> ? ?# Check for ELF target.
>>> ? ?is_elf=no
>>> ? ?case "${target}" in
>>> @@ -334,11 +335,17 @@ if test "${ENABLE_GOLD}" = "yes"; then
>>> ? ? ?# Check for target supported by gold.
>>> ? ? ?case "${target}" in
>>> ? ? ? ?i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-*)
>>> - ? ? ? ?configdirs="`echo " ${configdirs} " | sed -e 's/ ld / gold /'`"
>>> + ? ? ? ?if test "${ENABLE_GOLD}" = both; then
>>> + ? ? ? ? ?configdirs="$configdirs gold"
>>> + ? ? else
>>> + ? ? ? ? ?configdirs="`echo " ${configdirs} " | sed -e 's/ ld / gold /'`"
>>> + ? ? fi
>>> ? ? ? ? ?;;
>>> ? ? ?esac
>>> ? ?fi
>>> -fi
>>> + ?ENABLE_GOLD=yes
>>> + ?;;
>>> +esac
>>
>> You should have an error case here to ensure that --enable-gold only
>> has the values "yes", "both", or "no". ?--enable-gold=foo should give
>> an error.
>>
>> This patch uses two configure options: one to build both gold and ld,
>> and one to select which linker is the default. ?Why not use just one
>> configure option? ?Perhaps something like:
>> ? ?--enable-gold ? ? ? ? ? ? ?Build only gold, gold is default
>> ? ?--disable-gold [default] ? Build only GNU ld, GNU ld is default
>> ? ?--enable-gold=no ? ? ? ? ? Same
>> ? ?--enable-gold=both ? ? ? ? Build both gold and GNU ld, gold is default
>> ? ?--enable-gold=both/gold ? ?Same
>> ? ?--enable-gold=both/bfd ? ? Build both gold and GNU ld, GNU ld is default
>
> Done.
>
>> But of course this approach is conditional on whether there should be
>> some way to select the linker to use at runtime.
>>
>>
>>
>>> diff --git a/gold/Makefile.am b/gold/Makefile.am
>>> index 8d8b617..85b103b 100644
>>> --- a/gold/Makefile.am
>>> +++ b/gold/Makefile.am
>>> @@ -163,12 +163,20 @@ check: libgold.a
>>>
>>> ?install-exec-local: ld-new$(EXEEXT)
>>> ? ? ? $(mkinstalldirs) $(DESTDIR)$(bindir) $(DESTDIR)$(tooldir)/bin
>>> - ? ? n=`echo ld | sed '$(transform)'`; \
>>> + ? ? n=`echo @installed_linker@ | sed '$(transform)'`; \
>>> ? ? ? $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(bindir)/$${n}$(EXEEXT); \
>>> - ? ? if test "$(bindir)" != "$(tooldir)/bin"; then \
>>> - ? ? ? rm -f $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> - ? ? ? ln $(DESTDIR)$(bindir)/$${n}$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT) >/dev/null 2>/dev/null \
>>> + ? ? if test "@linker@" = "ld.gold"; then \
>>> + ? ? ? if test "@installed_linker@" != "ld"; then \
>>> + ? ? ? ? ld=`echo ld | sed '$(transform)'`; \
>>> + ? ? ? ? rm -f $(DESTDIR)$(bindir)/$${ld}$(EXEEXT); \
>>> + ? ? ? ? ln $(DESTDIR)$(bindir)/$${n}$(EXEEXT) $(DESTDIR)$(bindir)/$${ld}$(EXEEXT) >/dev/null 2>/dev/null \
>>> + ? ? ? ? || $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(bindir)/$${ld}$(EXEEXT); \
>>> + ? ? ? fi; \
>>> + ? ? ? if test "$(bindir)" != "$(tooldir)/bin"; then \
>>> + ? ? ? ? rm -f $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> + ? ? ? ? ln $(DESTDIR)$(bindir)/$${n}$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT) >/dev/null 2>/dev/null \
>>> ? ? ? ? ? || $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> + ? ? ? fi; \
>>> ? ? ? fi
>>
>> In Makefile.am you don't need to use @@ quoting in rules. ?You can
>> just use $(installed_linker) and $(linker).
>>
>
> Done.
>
>>
>>> diff --git a/gold/configure.ac b/gold/configure.ac
>>> index 85e23f9..10389a9 100644
>>> --- a/gold/configure.ac
>>> +++ b/gold/configure.ac
>>> @@ -38,6 +38,29 @@ AC_DEFINE_UNQUOTED(TARGET_SYSTEM_ROOT, "$sysroot",
>>> ?AC_DEFINE_UNQUOTED(TARGET_SYSTEM_ROOT_RELOCATABLE, $sysroot_relocatable,
>>> ? ?[Whether the system root can be relocated])
>>>
>>> +AC_ARG_ENABLE(gold,
>>> +[ ?--enable-gold[[=ARG]] ? ? build gold [[ARG={yes,both}]]],
>>> +[if test "${enableval}" = "both"; then
>>> + ? bfd_linker=ld.bfd
>>> + ? installed_linker=ld.gold
>>> + else
>>> + ? bfd_linker=ld.gold
>>> + ? installed_linker=ld
>>> + fi],
>>> +[bfd_linker=ld.bfd
>>> + installed_linker=ld])
>>> +AC_SUBST(installed_linker)
>>
>> It seems rather weird to set a variable named bfd_linker to be
>> ld.gold. ?What does that mean?
>
> Rewritten.
>
>>
>>> +AC_ARG_ENABLE(linker,
>>> +[ ?--enable-linker=[[ARG]] ? specify the default linker [[ARG={bfd,gold}]]],
>>> +[if test "${enableval}" = "gold"; then
>>> + ? linker=ld.gold
>>> + else
>>> + ? linker=$bfd_linker
>>> + fi],
>>> +[linker=$bfd_linker])
>>> +AC_SUBST(linker)
>>
>> You should give an error if --enable-linker is used with an
>> unrecognized value.
>
> Done.
>
>>
>>> --- a/ld/Makefile.am
>>> +++ b/ld/Makefile.am
>>> @@ -95,7 +95,7 @@ CXX_FOR_TARGET = ` \
>>> ? ? ?fi; \
>>> ? ?fi`
>>>
>>> -transform = s/^ld-new$$/ld/;@program_transform_name@
>>> +transform = s/^ld-new$$/@installed_linker@/;$(program_transform_name)
>>> ?bin_PROGRAMS = ld-new
>>> ?info_TEXINFOS = ld.texinfo
>>> ?ld_TEXINFOS = configdoc.texi
>>
>> $(installed_linker). ?Although actually as far as I can tell this line
>> is completely unnecessary. ?Only constant strings are passed to
>> $(transform), and those constant strings never include ld-new.
>>
>
> I'd like to keep it in this patch. ?We can have a separate patch to remove it.
>
>>> -install-exec-local: ld-new$(EXEEXT)
>>> +install-exec-local: ld-new$(EXEEXT) install-binPROGRAMS
>>> ? ? ? $(mkinstalldirs) $(DESTDIR)$(tooldir)/bin
>>> - ? ? n=`echo ld | sed '$(transform)'`; \
>>> - ? ? if [ "$(bindir)/$$n$(EXEEXT)" != "$(tooldir)/bin/ld$(EXEEXT)" ]; then \
>>> - ? ? ? rm -f $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> - ? ? ? ln $(DESTDIR)$(bindir)/$$n$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT) >/dev/null 2>/dev/null \
>>> - ? ? ? || $(LIBTOOL) --mode=install $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> + ? ? n=`echo @installed_linker@ | sed '$(transform)'`; \
>>> + ? ? if test "@linker@" = "ld.bfd"; then \
>>> + ? ? ? if test "@installed_linker@" != "ld"; then \
>>> + ? ? ? ? ld=`echo ld | sed '$(transform)'`; \
>>> + ? ? ? ? rm -f $(DESTDIR)$(bindir)/$$ld$(EXEEXT); \
>>> + ? ? ? ? ln $(DESTDIR)$(bindir)/$$n$(EXEEXT) $(DESTDIR)$(bindir)/$$ld$(EXEEXT) >/dev/null 2>/dev/null \
>>> + ? ? ? ? || $(LIBTOOL) --mode=install $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(bindir)/$$ld$(EXEEXT); \
>>> + ? ? ? fi; \
>>> + ? ? ? if test "$(bindir)/$$n$(EXEEXT)" != "$(tooldir)/bin/ld$(EXEEXT)"; then \
>>> + ? ? ? ? rm -f $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> + ? ? ? ? ln $(DESTDIR)$(bindir)/$$n$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT) >/dev/null 2>/dev/null \
>>> + ? ? ? ? || $(LIBTOOL) --mode=install $(INSTALL_PROGRAM) ld-new$(EXEEXT) $(DESTDIR)$(tooldir)/bin/ld$(EXEEXT); \
>>> + ? ? ? fi; \
>>> ? ? ? fi
>>
>> Here also use $(VAR) rather than @VAR@.
>
> Done.
>
>>
>>> diff --git a/ld/configure.in b/ld/configure.in
>>> index c4655f5..9786953 100644
>>> --- a/ld/configure.in
>>> +++ b/ld/configure.in
>>> @@ -69,6 +69,29 @@ AC_SUBST(use_sysroot)
>>> ?AC_SUBST(TARGET_SYSTEM_ROOT)
>>> ?AC_SUBST(TARGET_SYSTEM_ROOT_DEFINE)
>>>
>>> +AC_ARG_ENABLE(gold,
>>> +[ ?--enable-gold[[=ARG]] ? ? build gold [[ARG={yes,both}]]],
>>> +[if test "${enableval}" = "both"; then
>>> + ?gold_linker=ld.gold
>>> + ?installed_linker=ld.bfd
>>> +else
>>> + ?gold_linker=ld.bfd
>>> + ?installed_linker=ld
>>> +fi],
>>> +[gold_linker=ld.bfd
>>> + installed_linker=ld])
>>> +AC_SUBST(installed_linker)
>>
>> How can gold_linker be set to ld.bfd?
>>
>
> Rewritten.
>
> Thanks.
>
>
> --
> H.J.
> ---
> 2010-01-05 ?Roland McGrath ?<roland@redhat.com>
> ? ? ? ? ? ?H.J. Lu ?<hongjiu.lu@intel.com>
>
> ? ? ? ?* configure.ac (--enable-gold): Support both, both/gold and
> ? ? ? ?both/bfd to add gold to configdirs without removing ld.
> ? ? ? ?* configure: Regenerated.
>
> gold/
>
> 2010-01-05 ?H.J. Lu ?<hongjiu.lu@intel.com>
>
> ? ? ? ?* Makefile.am (install-exec-local): Install as $(installed_linker).
> ? ? ? ?Install as ld if $(linker) == "ld.gold" and $(installed_linker)
> ? ? ? ?!= "ld".
> ? ? ? ?* Makefile.in: Regenerated.
>
> ? ? ? ?* configure.ac (--enable-gold): Support both, both/gold and
> ? ? ? ?both/bfd.
> ? ? ? ?* configure: Regenerated.
>
> ld/
>
> 2010-01-05 ?H.J. Lu ?<hongjiu.lu@intel.com>
>
> ? ? ? ?* Makefile.am (transform): Install as $(installed_linker).
> ? ? ? ?(install-exec-local): Depend on install-binPROGRAMS. ?Install
> ? ? ? ?as $(installed_linker). ?Install as ld if $(linker) == "ld.bfd"
> ? ? ? ?and $(installed_linker) != "ld".
> ? ? ? ?* Makefile.in: Regenerated.
>
> ? ? ? ?* configure.in (--enable-gold): Support both, both/gold and
> ? ? ? ?both/bfd.
> ? ? ? ?* configure: Regenerated.
>

Here is the updated patch to set ENABLE_GOLD to yes only if
target is supported.

Any comments?

Thanks.


-- 
H.J.

Attachment: binutils-gold-7.patch
Description: Text document