On Wed, Nov 28, 2012 at 1:09 AM, Paolo Bonzini <bonz...@gnu.org> wrote:
> I cannot approve the collect2.c, gcc.c, opts.c, doc/invoke.texi.  I'll
> include some comments anyway, since the patch needs some more work.
>
>> 2011-06-27   Doug Kwan  <dougk...@google.com>
>>
>>       Google ref 41164-p2
>>       Backport upstream patch under review.
>>
>>       2011-01-19   Nick Clifton  <ni...@redhat.com>
>>                            Matthias Klose <d...@debian.org>
>>
>>       * configure.ac (gcc_cv_gold_srcdir): New cached variable -
>>       contains the location of the gold sources.
>
> This is gcc_cv_ld_gold_srcdir in configure.ac.  I prefer the name in the
> changelog.

I removed configure.ac change.  We can create a separate patch
to select gold as the default linker.  But this patch will only support
-fuse-ld=bfd and -fuse-ld=gold.

>> -  static const char *const ld_suffix = "ld";
>> -  static const char *const plugin_ld_suffix = PLUGIN_LD_SUFFIX;
>> -  static const char *const real_ld_suffix = "real-ld";
>> +  static const char *const ld_suffix      = "ld";
>> +  static const char *const gold_suffix       = "ld.gold";
>> +  static const char *const bfd_ld_suffix     = "ld.bfd";
>> +  static const char *const plugin_ld_suffix  = PLUGIN_LD_SUFFIX;
>> +  static const char *const real_ld_suffix    = "real-ld";
>
> Please use an array for ld_suffix, gold_suffix, bfd_ld_suffix,
> plugin_ld_suffix.  Similar for full_ld_suffix and friends.

Done.

>> -  bool use_plugin = false;
>> +  enum linker_select
>> +  {
>> +    DFLT_LINKER,
>> +    PLUGIN_LINKER,
>> +    GOLD_LINKER,
>> +    BFD_LINKER
>> +  } selected_linker = DFLT_LINKER;
>
> Please change the names to avoid the "DFLT" abbreviation.  For example
> USE_DEFAULT_LD, USE_PLUGIN_LD, USE_GOLD, USE_LD.

Done.

>
>>  AC_MSG_CHECKING(what linker to use)
>>  if test "$gcc_cv_ld" = ../ld/ld-new$build_exeext \
>>     || test "$gcc_cv_ld" = ../gold/ld-new$build_exeext; then
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index 51b6e85..b78f698 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -425,7 +425,7 @@ Objective-C and Objective-C++ Dialects}.
>>  -funit-at-a-time -funroll-all-loops -funroll-loops @gol
>>  -funsafe-loop-optimizations -funsafe-math-optimizations -funswitch-loops 
>> @gol
>>  -fvariable-expansion-in-unroller -fvect-cost-model -fvpt -fweb @gol
>> --fwhole-program -fwpa -fuse-linker-plugin @gol
>> +-fwhole-program -fwpa -fuse-ld -fuse-linker-plugin @gol
>>  --param @var{name}=@var{value}
>>  -O  -O0  -O1  -O2  -O3  -Os -Ofast -Og}
>>
>> @@ -8419,6 +8419,16 @@ the comparison operation before register allocation 
>> is complete.
>>
>>  Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
>>
>> +@item -fuse-ld=gold
>> +Use the @command{gold} linker instead of the default linker.
>> +This option is only necessary if GCC has been configured with
>> +@option{--enable-gold} and @option{--enable-ld=default}.
>
> This is not really clear.  You could have built binutils separately,
> would -fuse-ld=gold work in that case?  If not, I think that's a bug.
>
>> +@item -fuse-ld=bfd
>> +Use the @command{ld.bfd} linker instead of the default linker.
>> +This option is only necessary if GCC has been configured with
>> +@option{--enable-gold} and @option{--enable-ld}.
>
> Same concerns here.

Fixed.

>>  @item -fcprop-registers
>>  @opindex fcprop-registers
>>  After register allocation and post-register allocation instruction 
>> splitting,
>> diff --git a/gcc/exec-tool.in b/gcc/exec-tool.in
>> index 8a10775..46aaedc 100644
>> --- a/gcc/exec-tool.in
>> +++ b/gcc/exec-tool.in
>> @@ -1,6 +1,6 @@
>>  #! /bin/sh
>>
>> -# Copyright (C) 2007, 2008, 2010 Free Software Foundation, Inc.
>> +# Copyright (C) 2007, 2008, 2010, 2011 Free Software Foundation, Inc.
>>  # This file is part of GCC.
>>
>>  # GCC is free software; you can redistribute it and/or modify
>> @@ -21,11 +21,13 @@
>>
>>  ORIGINAL_AS_FOR_TARGET="@ORIGINAL_AS_FOR_TARGET@"
>>  ORIGINAL_LD_FOR_TARGET="@ORIGINAL_LD_FOR_TARGET@"
>> +ORIGINAL_GOLD_FOR_TARGET="@ORIGINAL_GOLD_FOR_TARGET@"
>>  ORIGINAL_PLUGIN_LD_FOR_TARGET="@ORIGINAL_PLUGIN_LD_FOR_TARGET@"
>>  ORIGINAL_NM_FOR_TARGET="@ORIGINAL_NM_FOR_TARGET@"
>>  exeext=@host_exeext@
>>  fast_install=@enable_fast_install@
>>  objdir=@objdir@
>> +version="1.1"
>
> Why is the -v behavior now required?  Please remove it from this patch.
>

I rewrote exec-tool.in change.  Now you get

# ./xgcc -B./  /tmp/x.c -fuse-ld=gold -v
...
./collect-ld --eh-frame-hdr -m elf_x86_64 -dynamic-linker
/lib64/ld-linux-x86-64.so.2 -fuse-ld=gold /lib/../lib64/crt1.o
/lib/../lib64/crti.o ./crtbegin.o -L. -L/lib/../lib64
-L/usr/lib/../lib64 /tmp/cclVWcGz.o -v -lgcc --as-needed -lgcc_s
--no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed ./crtend.o
/lib/../lib64/crtn.o
GNU gold (Linux/GNU Binutils 2.23.51.0.7.20121127) 1.11
/usr/local/bin/ld.gold: fatal error: -f/--auxiliary may not be used
without -shared
collect2: error: ld returned 1 exit status

This is because we pass everything to ld and ld.bfd/ld.gold doesn't
understand -fuse-ld=bfd/-fuse-ld=gold.  It isn't a problem for collect2
since it will filter-out -fuse-ld=bfd/-fuse-ld=gold.  I will submit a binutils
patch to ignore -fuse-ld=bfd/-fuse-ld=gold, similar to -flto options.

>> diff --git a/gcc/gcc.c b/gcc/gcc.c
>> index 13e93e5..1702d2c 100644
>> --- a/gcc/gcc.c
>> +++ b/gcc/gcc.c
>> @@ -705,6 +705,9 @@ proper position among the other output files.  */
>>      LINK_PLUGIN_SPEC \
>>      "%{flto|flto=*:%<fcompare-debug*} \
>>      %{flto} %{flto=*} %l " LINK_PIE_SPEC \
>> +   "%{fuse-ld=gold:%{fuse-ld=bfd:%e-fuse-ld=gold and -fuse-ld=bfd may not 
>> be used together}} \
>> +    %{fuse-ld=gold:-use-gold} \
>> +    %{fuse-ld=bfd:-use-ld}" \
>
> Why do we need to use separate spellings of the options in gcc and collect2?
>

This is a bug in the original patch. If we pass  -fuse-ld=bfd/-fuse-ld=gold to
linker, linker will complain.  -use-gold/-use-ld are valid linker options:

  -u SYMBOL, --undefined SYMBOL

It is wrong to abuse  -use-gold/-use-ld.

Here is the updated patch.  OK for trunk?

Thanks.

-- 
H.J.
----
2012-11-28   Nick Clifton  <ni...@redhat.com>
             Matthias Klose <d...@debian.org>
             Doug Kwan  <dougk...@google.com>
             H.J. Lu  <hongjiu...@intel.com>

        * collect2.c (main): Support -fuse-ld=bfd and -fuse-ld=gold.
        * exec-tool.in: Likewise.

        * common.opt: Add fuse-ld=bfd and fuse-ld=gold.

        * gcc.c (LINK_COMMAND_SPEC): Pass -fuse-ld=* to collect2.

        * opts.c (comman_handle_option): Ignore -fuse-ld=bfd and
        -fuse-ld=gold.

        * doc/invoke.texi: Document Da-fuse-ld=bfd and -fuse-ld=gold.

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 49c4030..19bdc29 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -842,8 +842,21 @@ maybe_run_lto_and_relink (char **lto_ld_argv,
char **object_lst,
 int
 main (int argc, char **argv)
 {
-  static const char *const ld_suffix   = "ld";
-  static const char *const plugin_ld_suffix = PLUGIN_LD_SUFFIX;
+  enum linker_select
+    {
+      USE_DEFAULT_LD,
+      USE_PLUGIN_LD,
+      USE_GOLD_LD,
+      USE_BFD_LD,
+      USE_LD_MAX
+    } selected_linker = USE_DEFAULT_LD;
+  static const char *const ld_suffixes[USE_LD_MAX] =
+    {
+      "ld",
+      PLUGIN_LD_SUFFIX,
+      "ld.gold",
+      "ld.bfd"
+    };
   static const char *const real_ld_suffix = "real-ld";
   static const char *const collect_ld_suffix = "collect-ld";
   static const char *const nm_suffix   = "nm";
@@ -854,16 +867,13 @@ main (int argc, char **argv)
   static const char *const strip_suffix = "strip";
   static const char *const gstrip_suffix = "gstrip";

+  const char *full_ld_suffixes[USE_LD_MAX];
 #ifdef CROSS_DIRECTORY_STRUCTURE
   /* If we look for a program in the compiler directories, we just use
      the short name, since these directories are already system-specific.
      But it we look for a program in the system directories, we need to
      qualify the program name with the target machine.  */

-  const char *const full_ld_suffix =
-    concat(target_machine, "-", ld_suffix, NULL);
-  const char *const full_plugin_ld_suffix =
-    concat(target_machine, "-", plugin_ld_suffix, NULL);
   const char *const full_nm_suffix =
     concat (target_machine, "-", nm_suffix, NULL);
   const char *const full_gnm_suffix =
@@ -877,13 +887,11 @@ main (int argc, char **argv)
   const char *const full_gstrip_suffix =
     concat (target_machine, "-", gstrip_suffix, NULL);
 #else
-  const char *const full_ld_suffix     = ld_suffix;
-  const char *const full_plugin_ld_suffix = plugin_ld_suffix;
-  const char *const full_nm_suffix     = nm_suffix;
-  const char *const full_gnm_suffix    = gnm_suffix;
 #ifdef LDD_SUFFIX
   const char *const full_ldd_suffix    = ldd_suffix;
 #endif
+  const char *const full_nm_suffix     = nm_suffix;
+  const char *const full_gnm_suffix    = gnm_suffix;
   const char *const full_strip_suffix  = strip_suffix;
   const char *const full_gstrip_suffix = gstrip_suffix;
 #endif /* CROSS_DIRECTORY_STRUCTURE */
@@ -900,6 +908,7 @@ main (int argc, char **argv)
   char **ld1_argv;
   const char **ld1;
   bool use_plugin = false;
+  bool use_collect_ld = false;

   /* The kinds of symbols we will have to consider when scanning the
      outcome of a first pass link.  This is ALL to start with, then might
@@ -919,6 +928,15 @@ main (int argc, char **argv)
   int first_file;
   int num_c_args;
   char **old_argv;
+  int i;
+
+  for (i = 0; i < USE_LD_MAX; i++)
+    full_ld_suffixes[i]
+#ifdef CROSS_DIRECTORY_STRUCTURE
+      = concat(target_machine, "-", ld_suffixes[i], NULL);
+#else
+      = ld_suffixes[i];
+#endif

   p = argv[0] + strlen (argv[0]);
   while (p != argv[0] && !IS_DIR_SEPARATOR (p[-1]))
@@ -980,7 +998,6 @@ main (int argc, char **argv)
      are called.  We also look for the -flto or -flto-partition=none
flag to know
      what LTO mode we are in.  */
   {
-    int i;
     bool no_partition = false;

     for (i = 1; argv[i] != NULL; i ++)
@@ -998,7 +1015,14 @@ main (int argc, char **argv)
          {
            use_plugin = true;
            lto_mode = LTO_MODE_NONE;
+           if (selected_linker == USE_DEFAULT_LD)
+             selected_linker = USE_PLUGIN_LD;
          }
+       else if (strcmp (argv[i], "-fuse-ld=bfd") == 0)
+         selected_linker = USE_BFD_LD;
+       else if (strcmp (argv[i], "-fuse-ld=gold") == 0)
+         selected_linker = USE_GOLD_LD;
+
 #ifdef COLLECT_EXPORT_LIST
        /* since -brtl, -bexport, -b64 are not position dependent
           also check for them here */
@@ -1095,21 +1119,18 @@ main (int argc, char **argv)
   ld_file_name = find_a_file (&cpath, real_ld_suffix);
   /* Likewise for `collect-ld'.  */
   if (ld_file_name == 0)
-    ld_file_name = find_a_file (&cpath, collect_ld_suffix);
+    {
+      ld_file_name = find_a_file (&cpath, collect_ld_suffix);
+      use_collect_ld = ld_file_name != 0;
+    }
   /* Search the compiler directories for `ld'.  We have protection against
      recursive calls in find_a_file.  */
   if (ld_file_name == 0)
-    ld_file_name = find_a_file (&cpath,
-                               use_plugin
-                               ? plugin_ld_suffix
-                               : ld_suffix);
+    ld_file_name = find_a_file (&cpath, ld_suffixes[selected_linker]);
   /* Search the ordinary system bin directories
      for `ld' (if native linking) or `TARGET-ld' (if cross).  */
   if (ld_file_name == 0)
-    ld_file_name = find_a_file (&path,
-                               use_plugin
-                               ? full_plugin_ld_suffix
-                               : full_ld_suffix);
+    ld_file_name = find_a_file (&path, full_ld_suffixes[selected_linker]);

 #ifdef REAL_NM_FILE_NAME
   nm_file_name = find_a_file (&path, REAL_NM_FILE_NAME);
@@ -1266,6 +1287,13 @@ main (int argc, char **argv)
                         "configuration");
 #endif
                }
+             else if (!use_collect_ld
+                      && strncmp (arg, "-fuse-ld=", 9) == 0)
+               {
+                 /* Do not pass -fuse-ld={bfd|gold} to the linker. */
+                 ld1--;
+                 ld2--;
+               }
 #ifdef TARGET_AIX_VERSION
              else
                {
diff --git a/gcc/common.opt b/gcc/common.opt
index 4c8bd11..7b36ba3 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2171,6 +2171,12 @@ funwind-tables
 Common Report Var(flag_unwind_tables) Optimization
 Just generate unwind tables for exception handling

+fuse-ld=bfd
+Common Var(flag_use_ld_bfd) Negative(fuse-ld=gold) Undocumented
+
+fuse-ld=gold
+Common Var(flag_use_ld_gold) Negative(fuse-ld=bfd) Undocumented
+
 fuse-linker-plugin
 Common Undocumented

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 51b6e85..3558f43 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -425,7 +425,7 @@ Objective-C and Objective-C++ Dialects}.
 -funit-at-a-time -funroll-all-loops -funroll-loops @gol
 -funsafe-loop-optimizations -funsafe-math-optimizations -funswitch-loops @gol
 -fvariable-expansion-in-unroller -fvect-cost-model -fvpt -fweb @gol
--fwhole-program -fwpa -fuse-linker-plugin @gol
+-fwhole-program -fwpa -fuse-ld=@var{linker} -fuse-linker-plugin @gol
 --param @var{name}=@var{value}
 -O  -O0  -O1  -O2  -O3  -Os -Ofast -Og}

@@ -8419,6 +8419,12 @@ the comparison operation before register
allocation is complete.

 Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.

+@item -fuse-ld=gold
+Use the @command{gold} linker instead of the default linker.
+
+@item -fuse-ld=bfd
+Use the @command{ld.bfd} linker instead of the default linker.
+
 @item -fcprop-registers
 @opindex fcprop-registers
 After register allocation and post-register allocation instruction splitting,
diff --git a/gcc/exec-tool.in b/gcc/exec-tool.in
index 8a10775..8f9ae27 100644
--- a/gcc/exec-tool.in
+++ b/gcc/exec-tool.in
@@ -36,13 +36,28 @@ case "$invoked" in
     dir=gas
     ;;
   collect-ld)
-    # when using a linker plugin, gcc will always pass '-plugin' as the
-    # first or second option to the linker.
-    if test x"$1" = "x-plugin" || test x"$2" = "x-plugin"; then
-      original=$ORIGINAL_PLUGIN_LD_FOR_TARGET
-    else
-      original=$ORIGINAL_LD_FOR_TARGET
-    fi
+    # Check -fuse-ld=bfd and -fuse-ld=gold
+    case " $* " in
+      *\ -fuse-ld=bfd\ *)
+       if test -x ${ORIGINAL_LD_FOR_TARGET}.bfd; then
+         original=${ORIGINAL_LD_FOR_TARGET}.bfd;
+       fi
+       ;;
+      *\ -fuse-ld=gold\ *)
+       if test -x ${ORIGINAL_LD_FOR_TARGET}.gold; then
+         original=${ORIGINAL_LD_FOR_TARGET}.gold;
+       fi
+       ;;
+      *)
+       # when using a linker plugin, gcc will always pass '-plugin' as the
+       # first or second option to the linker.
+       if test x"$1" = "x-plugin" || test x"$2" = "x-plugin"; then
+         original=$ORIGINAL_PLUGIN_LD_FOR_TARGET
+       else
+         original=$ORIGINAL_LD_FOR_TARGET
+       fi
+       ;;
+    esac
     prog=ld-new$exeext
     dir=ld
     id=ld
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 13e93e5..e0fee40 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -705,7 +705,8 @@ proper position among the other output files.  */
     LINK_PLUGIN_SPEC \
     "%{flto|flto=*:%<fcompare-debug*} \
     %{flto} %{flto=*} %l " LINK_PIE_SPEC \
-   "%X %{o*} %{e*} %{N} %{n} %{r}\
+   "%{fuse-ld=*:-fuse-ld=%*}\
+    %X %{o*} %{e*} %{N} %{n} %{r}\
     %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}\
     %{static:} %{L*} %(mfwrap) %(link_libgcc) %o\
     %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)}\
diff --git a/gcc/opts.c b/gcc/opts.c
index b3a9afe..ff1b51e 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1753,6 +1753,8 @@ common_handle_option (struct gcc_options *opts,
       dc->max_errors = value;
       break;

+    case OPT_fuse_ld_bfd:
+    case OPT_fuse_ld_gold:
     case OPT_fuse_linker_plugin:
       /* No-op. Used by the driver and passed to us because it starts with f.*/
       break;

Reply via email to