diff mbox

Review of --enable-gold=both patch

Message ID 4CDB3413.1090900@ubuntu.com
State New
Headers show

Commit Message

Matthias Klose Nov. 11, 2010, 12:08 a.m. UTC 
On 29.10.2010 17:12, Paolo Bonzini wrote:
> On 10/29/2010 04:54 PM, Matthias Klose wrote:
>> On 29.10.2010 11:20, Nick Clifton wrote:
>>> Hi Matthias,
>>>
>>>>> OK. I suppose we could also have --with-default-linker=gold -- or
>>>>> --enable :-) -- but I'm not worried about that.
>>>>>
>>>>> Thanks again for your help with this.
>>>>
>>>> the --enable-gold=both option is already in current binutils trunk,
>>>> which will soon be released as binutils-2.21. Should this be changed
>>>> before the binutils release, or should the binutils patch be kept as it
>>>> is? Any recommendations how to update the patch and/or current binutils?
>>>
>>> We should really make the 2.21 release support --with-default-linker=
>>> Would you care to submit a patch for this ?
>>
>> to summarize:
>>
>> - in binutils, --with-default-linker= should trigger which linker is
>> built, and should determine which linker the ld symlink points to.
>>
>> - if both linkers are built, the non-default linker build has to be
>> enabled with either --enable-ld or --enable-gold.
>>
>> - values (other than yes or no) passed to --enable-ld and --enable-gold
>> are ignored.
>
> Uh, actually no. Rereading the thread, the only point on which we had consensus
> :) was to remove the current wart of --enable-gold disabling --enable-ld; you're
> proposing to double it instead.
>
> This was my proposal:
>
> --disable-gold [--enable-ld]
> Build only ld. Default option.
>
> --enable-gold[=default] [--enable-ld]
> Build both gold and ld. Install gold as "gold" and "ld", install ld
> as "ld.bfd". This is different from the current behavior, in that ld
> is currently not installed at all.
>
> --enable-gold[=default] --disable-ld
> Build only gold, which is then installed as both "gold" and "ld".
>
> --enable-gold --enable-ld=default
> Build both gold (installed as "gold") and ld (installed as "ld").
> In other words, ld is default
>
> --enable-gold=default --enable-ld=default
> Error.

The attached patch implements this proposal, tested the combinations above, and 
disabling ld without gold (leading to an error).

I did choose to always install the bfd linker as ld.bfd and the gold linker as 
ld.gold, and pointing the link to one of the above.

Nick, is this ok with you?

   Matthias

Comments

Paolo Bonzini Nov. 11, 2010, 8:37 a.m. UTC | #1 
On 11/11/2010 01:08 AM, Matthias Klose wrote:
> +    if test x${default_ld} = xgold; then
> +      AC_MSG_ERROR([either gold or ld can be the default ld])
> +    fi

If you test for x${ENABLE_GOLD} = xdefault, the default_ld variable can 
go away.

> +  no)
> +    if test x${ENABLE_GOLD} != xyes; then
> +      AC_MSG_ERROR([gold must be enabled to disable ld])
> +    fi

I think this is not necessary, you can disable both packages.  This 
would have the same effect as --without-gnu-ld.  Please conditionalize 
the message on $use_gnu_ld != no and make it a warning rather than an error.

Ok with this fixed.

Paolo
Matthias Klose Nov. 12, 2010, 6:44 p.m. UTC | #2 
On 11.11.2010 09:37, Paolo Bonzini wrote:
> On 11/11/2010 01:08 AM, Matthias Klose wrote:
>> + no)
>> + if test x${ENABLE_GOLD} != xyes; then
>> + AC_MSG_ERROR([gold must be enabled to disable ld])
>> + fi
>
> I think this is not necessary, you can disable both packages. This would have
> the same effect as --without-gnu-ld. Please conditionalize the message on
> $use_gnu_ld != no and make it a warning rather than an error.

in the context of binutils, at least one linker should be built?  why not detect 
this early?

   Matthias
Paolo Bonzini Nov. 12, 2010, 7:35 p.m. UTC | #3 
On 11/12/2010 07:44 PM, Matthias Klose wrote:
> On 11.11.2010 09:37, Paolo Bonzini wrote:
>> On 11/11/2010 01:08 AM, Matthias Klose wrote:
>>> + no)
>>> + if test x${ENABLE_GOLD} != xyes; then
>>> + AC_MSG_ERROR([gold must be enabled to disable ld])
>>> + fi
>>
>> I think this is not necessary, you can disable both packages. This
>> would have
>> the same effect as --without-gnu-ld. Please conditionalize the message on
>> $use_gnu_ld != no and make it a warning rather than an error.
>
> in the context of binutils, at least one linker should be built?

Not necessarily, if the user wants the rope.  Have you tried configuring 
binutils --without-gnu-ld?

Paolo
Andreas Schwab Nov. 12, 2010, 8:36 p.m. UTC | #4 
Matthias Klose <doko@ubuntu.com> writes:

> in the context of binutils, at least one linker should be built?  why not
> detect this early?

There are still targets without linker support.

Andreas.
Nick Clifton Nov. 23, 2010, 12:24 p.m. UTC | #5 
Hi Matthias,

   [Sorry about the delay in replying - I am in a bit of a muddle right 
now].

> The attached patch implements this proposal, tested the combinations
> above, and disabling ld without gold (leading to an error).
>
> I did choose to always install the bfd linker as ld.bfd and the gold
> linker as ld.gold, and pointing the link to one of the above.
>
> Nick, is this ok with you?

Yes, it is fine.  Thanks for doing this.

Cheers
   Nick
diff mbox

Patch

--- binutils-2.20.90.20101105.orig/configure.ac
+++ binutils-2.20.90.20101105/configure.ac
@@ -321,19 +321,27 @@ 
   yes) skipdirs=`echo " ${skipdirs} " | sed -e 's/ target-newlib / /'` ;;
 esac
 
-# Handle --enable-gold.
-#   --enable-gold		Build only gold
-#   --disable-gold [default]	Build only ld
-#   --enable-gold=both		Build both gold and ld, ld is default
-#   --enable-gold=both/ld	Same
-#   --enable-gold=both/gold	Build both gold and ld, gold is default, ld is renamed ld.bfd
+# Handle --enable-gold, --enable-ld.
+# --disable-gold [--enable-ld]
+#     Build only ld.  Default option.
+# --enable-gold[=default] [--enable-ld]
+#     Build both gold and ld.  Install gold as "ld.gold" and "ld",
+#     install ld as "ld.bfd".
+# --enable-gold[=default] --disable-ld
+#     Build only gold, which is then installed as both "ld.gold" and "ld".
+# --enable-gold --enable-ld=default
+#     Build both gold (installed as "ld.gold") and ld (installed as "ld").
+#     In other words, ld is default
+# --enable-gold=default --enable-ld=default
+#     Error.
 
+default_ld=
 AC_ARG_ENABLE(gold,
-[[  --enable-gold[=ARG]     build gold [ARG={both}[/{gold,ld}]]]],
+[[  --enable-gold[=ARG]     build gold [ARG={default,yes,no}]]],
 ENABLE_GOLD=$enableval,
 ENABLE_GOLD=no)
-  case "${ENABLE_GOLD}" in
-  yes|both|both/gold|both/ld)
+case "${ENABLE_GOLD}" in
+  yes|default)
     # Check for ELF target.
     is_elf=no
     case "${target}" in
@@ -353,14 +361,10 @@ 
       # Check for target supported by gold.
       case "${target}" in
         i?86-*-* | x86_64-*-* | sparc*-*-* | powerpc*-*-* | arm*-*-*)
-	  case "${ENABLE_GOLD}" in 
-	  both*)
-            configdirs="$configdirs gold"
-	    ;;
-	  *)
-            configdirs=`echo " ${configdirs} " | sed -e 's/ ld / gold /'`
-	    ;;
-	  esac
+	  configdirs="$configdirs gold"
+	  if test x${ENABLE_GOLD} = xdefault; then
+	    default_ld=gold
+	  fi
 	  ENABLE_GOLD=yes
           ;;
       esac
@@ -371,7 +375,31 @@ 
   *)
     AC_MSG_ERROR([invalid --enable-gold argument])
     ;;
-  esac
+esac
+
+AC_ARG_ENABLE(ld,
+[[  --enable-ld[=ARG]     build ld [ARG={default,yes,no}]]],
+ENABLE_LD=$enableval,
+ENABLE_LD=yes)
+
+case "${ENABLE_LD}" in
+  default)
+    if test x${default_ld} = xgold; then
+      AC_MSG_ERROR([either gold or ld can be the default ld])
+    fi
+    ;;
+  yes)
+    ;;
+  no)
+    if test x${ENABLE_GOLD} != xyes; then
+      AC_MSG_ERROR([gold must be enabled to disable ld])
+    fi
+    configdirs=`echo " ${configdirs} " | sed -e 's/ ld / /'`
+    ;;
+  *)
+    AC_MSG_ERROR([invalid --enable-ld argument])
+    ;;
+esac
 
 # Configure extra directories which are host specific
 
--- binutils-2.20.90.20101105.orig/ld/configure.in
+++ binutils-2.20.90.20101105/ld/configure.in
@@ -73,13 +73,13 @@ 
 dnl "install_as_default" is set to false if gold is the default linker.
 dnl "installed_linker" is the installed BFD linker name.
 AC_ARG_ENABLE(gold,
-[[  --enable-gold[=ARG]     build gold [ARG={both}[/{gold,ld}]]]],
+[[  --enable-gold[=ARG]     build gold [ARG={default,yes,no}]]],
 [case "${enableval}" in 
- yes|both/gold)
+ default)
    install_as_default=no
    installed_linker=ld.bfd
    ;;
- both|both/ld)
+ yes|no)
    install_as_default=yes
    installed_linker=ld.bfd
    ;;
@@ -87,8 +87,8 @@ 
    AC_MSG_ERROR([invalid --enable-gold argument])
    ;;
  esac],
-[install_as_default=ld
- installed_linker=ld])
+[install_as_default=yes
+ installed_linker=ld.bfd])
 AC_SUBST(install_as_default)
 AC_SUBST(installed_linker)
 
--- binutils-2.20.90.20101105.orig/gold/configure.ac
+++ binutils-2.20.90.20101105/gold/configure.ac
@@ -41,27 +41,33 @@ 
 dnl "install_as_default" is true if the linker to be installed as the
 dnl default linker, ld.
 dnl "installed_linker" is the installed gold linker name.
-AC_ARG_ENABLE(gold,
-[[  --enable-gold[=ARG]     build gold [ARG={both}[/{gold,ld}]]]],
+
+default_ld=
+AC_ARG_ENABLE(ld,
+[[  --enable-ld[=ARG]     build ld [ARG={default,yes,no}]]],
 [case "${enableval}" in 
- yes)
-   install_as_default=gold
-   installed_linker=ld
-   ;;
- both/gold)
-   install_as_default=yes
+  default)
+    default_ld=ld.bfd
+    ;;
+esac])
+
+AC_ARG_ENABLE(gold,
+[[  --enable-gold[=ARG]     build gold [ARG={default,yes,no}]]],
+[case "${enableval}" in
+ yes|default)
+   if test x${default_ld} = x; then
+     install_as_default=yes
+   fi
    installed_linker=ld.gold
    ;;
- both|both/ld)
-   install_as_default=no
-   installed_linker=ld.gold
+ no)
    ;;
  *)
    AC_MSG_ERROR([invalid --enable-gold argument])
    ;;
  esac],
-[install_as_default=gold
- installed_linker=ld])
+[install_as_default=no
+ installed_linker=ld.gold])
 AC_SUBST(install_as_default)
 AC_SUBST(installed_linker)