diff mbox

Review of --enable-gold=both patch

Message ID AANLkTim8CO2L_FnxucveYBT+mGj=MMTcDVW1b_YTEr2s@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Nov. 23, 2010, 6:02 p.m. UTC 
On Tue, Nov 23, 2010 at 9:09 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Nov 23, 2010 at 5:26 AM, Matthias Klose <doko@ubuntu.com> wrote:
>> On 23.11.2010 13:24, Nick Clifton wrote:
>>>
>>> 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.
>>
>> checked in in gcc as r167076.  Nick, Tristan, could you check it into
>> binutils trunk and branch?
>>
>
> Binutils is broken now:
>
> http://www.sourceware.org/bugzilla/show_bug.cgi?id=12258
>

This change:

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

is wrong.  At this time, default_ld is set to gold only by --enable-gold=default
What we should test is

if test x${default_ld} != x; then

which checks that default_ld isn't set.  Also comments are wrong:

# 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 only enables gold, not make it default unless
ld is disabled.

# --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 "gold") and ld (installed as "ld").

gold is installed as ld.gold.  ld is also installed ld.bfd.

#     In other words, ld is default
# --enable-gold=default --enable-ld=default
#     Error.

I am checking in this patch as an obvious fix.

Comments

Tristan Gingold Nov. 24, 2010, 10 a.m. UTC | #1 
On Nov 23, 2010, at 7:02 PM, H.J. Lu wrote:

> 
> This change:
> 
> 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
>    ;;
> 
> is wrong.  At this time, default_ld is set to gold only by --enable-gold=default
> What we should test is
> 
> if test x${default_ld} != x; then
> 
> which checks that default_ld isn't set.  Also comments are wrong:
> 
> # 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 only enables gold, not make it default unless
> ld is disabled.
> 
> # --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 "gold") and ld (installed as "ld").
> 
> gold is installed as ld.gold.  ld is also installed ld.bfd.
> 
> #     In other words, ld is default
> # --enable-gold=default --enable-ld=default
> #     Error.
> 
> I am checking in this patch as an obvious fix.

Looks like this hasn't yet been committed to binutils.

Tristan.
H.J. Lu Nov. 24, 2010, 1:11 p.m. UTC | #2 
On Wed, Nov 24, 2010 at 2:00 AM, Tristan Gingold <gingold@adacore.com> wrote:
>
> On Nov 23, 2010, at 7:02 PM, H.J. Lu wrote:
>
>>
>> This change:
>>
>> 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
>>    ;;
>>
>> is wrong.  At this time, default_ld is set to gold only by --enable-gold=default
>> What we should test is
>>
>> if test x${default_ld} != x; then
>>
>> which checks that default_ld isn't set.  Also comments are wrong:
>>
>> # 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 only enables gold, not make it default unless
>> ld is disabled.
>>
>> # --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 "gold") and ld (installed as "ld").
>>
>> gold is installed as ld.gold.  ld is also installed ld.bfd.
>>
>> #     In other words, ld is default
>> # --enable-gold=default --enable-ld=default
>> #     Error.
>>
>> I am checking in this patch as an obvious fix.
>
> Looks like this hasn't yet been committed to binutils.
>

I don't know what happened to cvs archive. But it is in:

http://sourceware.org/git/?p=binutils.git;a=commit;h=e1cffffd802eb3d50c4f3e0718d1f194c58d8dc0
Tristan Gingold Nov. 24, 2010, 1:25 p.m. UTC | #3 
On Nov 24, 2010, at 2:11 PM, H.J. Lu wrote:

> On Wed, Nov 24, 2010 at 2:00 AM, Tristan Gingold <gingold@adacore.com> wrote:
>> 
>> On Nov 23, 2010, at 7:02 PM, H.J. Lu wrote:
>> 
>>> 
>>> This change:
>>> 
>>> 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
>>>    ;;
>>> 
>>> is wrong.  At this time, default_ld is set to gold only by --enable-gold=default
>>> What we should test is
>>> 
>>> if test x${default_ld} != x; then
>>> 
>>> which checks that default_ld isn't set.  Also comments are wrong:
>>> 
>>> # 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 only enables gold, not make it default unless
>>> ld is disabled.
>>> 
>>> # --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 "gold") and ld (installed as "ld").
>>> 
>>> gold is installed as ld.gold.  ld is also installed ld.bfd.
>>> 
>>> #     In other words, ld is default
>>> # --enable-gold=default --enable-ld=default
>>> #     Error.
>>> 
>>> I am checking in this patch as an obvious fix.
>> 
>> Looks like this hasn't yet been committed to binutils.
>> 
> 
> I don't know what happened to cvs archive. But it is in:
> 
> http://sourceware.org/git/?p=binutils.git;a=commit;h=e1cffffd802eb3d50c4f3e0718d1f194c58d8dc0

Indeed.

Tristan.
diff mbox

Patch

diff --git a/configure b/configure
index 2758ab0..8b67ba3 100755
--- a/configure
+++ b/configure
@@ -2854,13 +2854,17 @@  esac
 # Handle --enable-gold, --enable-ld.
 # --disable-gold [--enable-ld]
 #     Build only ld.  Default option.
-# --enable-gold[=default] [--enable-ld]
+# --enable-gold [--enable-ld]
+#     Build both gold and ld.  Install gold as "ld.gold", install ld
+#     as "ld.bfd" and "ld".
+# --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 "gold") and ld (installed as "ld").
+#     Build both gold (installed as "ld.gold") and ld (installed as "ld"
+#     and ld.bfd).
 #     In other words, ld is default
 # --enable-gold=default --enable-ld=default
 #     Error.
@@ -2920,7 +2924,7 @@  fi
 
 case "${ENABLE_LD}" in
   default)
-    if test x${default_ld} != xgold; then
+    if test x${default_ld} != x; then
       as_fn_error "either gold or ld can be the default ld" "$LINENO" 5
     fi
     ;;
diff --git a/configure.ac b/configure.ac
index 626bb4e..294f241 100644
--- a/configure.ac
+++ b/configure.ac
@@ -327,13 +327,17 @@  esac
 # Handle --enable-gold, --enable-ld.
 # --disable-gold [--enable-ld]
 #     Build only ld.  Default option.
-# --enable-gold[=default] [--enable-ld]
+# --enable-gold [--enable-ld]
+#     Build both gold and ld.  Install gold as "ld.gold", install ld
+#     as "ld.bfd" and "ld".
+# --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 "gold") and ld (installed as "ld").
+#     Build both gold (installed as "ld.gold") and ld (installed as "ld"
+#     and ld.bfd).
 #     In other words, ld is default
 # --enable-gold=default --enable-ld=default
 #     Error.
@@ -387,7 +391,7 @@  ENABLE_LD=yes)
 
 case "${ENABLE_LD}" in
   default)
-    if test x${default_ld} != xgold; then
+    if test x${default_ld} != x; then
       AC_MSG_ERROR([either gold or ld can be the default ld])
     fi
     ;;