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

[Ian Lance Taylor <iant at google dot com>]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> On Tue, Nov 3, 2009 at 3:23 PM, Roland McGrath <roland@redhat.com> wrote:
>> --with is wrong for this. ÂIt's not about the ambient system built against.
>> It's a feature selection for how you build binutils, which means --enable.
>>
>
> Here is the updated patch.

I'm still not entirely convinced that this is the way to go.  It seems
to me that ideally one wants to be able to select the linker at
runtime.  I don't see how this patch supports that.  What am I
missing?

This patch would be easier to review if you omitted the generated
files.

Do any of the binutils maintainers have any comments on the best
approach here?


> 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

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).



> 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?


> +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.


> --- 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.


> -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@.


> 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?

Ian