"H.J. Lu" <hjl.to...@gmail.com> writes: > On Tue, Nov 3, 2009 at 3:23 PM, Roland McGrath <rol...@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 @v...@. > 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