From: Paolo Bonzini <bonzini@gnu.org>
To: "H.J. Lu" <hjl.tools@gmail.com>
Cc: Matthias Klose <doko@ubuntu.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Diego Novillo <dnovillo@google.com>
Subject: Re: PATCH: Enable both ld and gold in GCC 4.8
Date: Wed, 28 Nov 2012 09:10:00 -0000 [thread overview]
Message-ID: <50B5D4D9.6000400@gnu.org> (raw)
In-Reply-To: <CAMe9rOrWL8bK8=c0nOb6JUMxR+LjK6weZPBfk-QjWu0jVSMP5A@mail.gmail.com>
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 <dougkwan@google.com>
>
> Google ref 41164-p2
> Backport upstream patch under review.
>
> 2011-01-19 Nick Clifton <nickc@redhat.com>
> Matthias Klose <doko@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.
> - 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.
> - 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.
>
> /* 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
> @@ -990,15 +1004,21 @@ main (int argc, char **argv)
> else if (! strcmp (argv[i], "-flto-partition=none"))
> no_partition = true;
> else if ((! strncmp (argv[i], "-flto=", 6)
> - || ! strcmp (argv[i], "-flto")) && ! use_plugin)
> + || ! strcmp (argv[i], "-flto"))
> + && selected_linker != PLUGIN_LINKER)
> lto_mode = LTO_MODE_WHOPR;
> else if (!strncmp (argv[i], "-fno-lto", 8))
> lto_mode = LTO_MODE_NONE;
> else if (! strcmp (argv[i], "-plugin"))
> {
> - use_plugin = true;
> + selected_linker = PLUGIN_LINKER;
> lto_mode = LTO_MODE_NONE;
> }
> + else if (! strcmp (argv[i], "-use-gold"))
> + selected_linker = GOLD_LINKER;
> + else if (! strcmp (argv[i], "-use-ld"))
> + selected_linker = BFD_LINKER;
> +
> #ifdef COLLECT_EXPORT_LIST
> /* since -brtl, -bexport, -b64 are not position dependent
> also check for them here */
> @@ -1081,35 +1101,108 @@ main (int argc, char **argv)
> /* Try to discover a valid linker/nm/strip to use. */
>
> /* Maybe we know the right file to use (if not cross). */
> - ld_file_name = 0;
> + ld_file_name = NULL;
> #ifdef DEFAULT_LINKER
> if (access (DEFAULT_LINKER, X_OK) == 0)
> ld_file_name = DEFAULT_LINKER;
> - if (ld_file_name == 0)
> + if (ld_file_name == NULL)
> #endif
> #ifdef REAL_LD_FILE_NAME
> ld_file_name = find_a_file (&path, REAL_LD_FILE_NAME);
> - if (ld_file_name == 0)
> + if (ld_file_name == NULL)
> #endif
> /* Search the (target-specific) compiler dirs for ld'. */
> ld_file_name = find_a_file (&cpath, real_ld_suffix);
> /* Likewise for `collect-ld'. */
> - if (ld_file_name == 0)
> + if (ld_file_name == NULL)
> ld_file_name = find_a_file (&cpath, collect_ld_suffix);
> /* 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);
> + if (ld_file_name == NULL)
> + switch (selected_linker)
> + {
> + default:
> + case DFLT_LINKER:
> + ld_file_name = find_a_file (&cpath, ld_suffix);
> + break;
> + case PLUGIN_LINKER:
> + ld_file_name = find_a_file (&cpath, plugin_ld_suffix);
> + break;
> + case GOLD_LINKER:
> + ld_file_name = find_a_file (&cpath, gold_suffix);
> + break;
> + case BFD_LINKER:
> + ld_file_name = find_a_file (&cpath, bfd_ld_suffix);
> + break;
> + }
If you use an array, no switch is needed here...
> /* 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);
> + if (ld_file_name == NULL)
> + switch (selected_linker)
> + {
> + default:
> + case DFLT_LINKER:
> + ld_file_name = find_a_file (&path, full_ld_suffix);
> + break;
> + case PLUGIN_LINKER:
> + ld_file_name = find_a_file (&path, full_plugin_ld_suffix);
> + break;
> + case GOLD_LINKER:
> + ld_file_name = find_a_file (&path, full_gold_suffix);
> + break;
> + case BFD_LINKER:
> + ld_file_name = find_a_file (&path, full_bfd_ld_suffix);
> + break;
> + }
... here...
> + /* If we failed to find a plugin-capable linker, try the ordinary one. */
> + if (ld_file_name == NULL && selected_linker == PLUGIN_LINKER)
> + ld_file_name = find_a_file (&cpath, ld_suffix);
> +
> + if ((vflag || debug) && ld_file_name == NULL)
> + {
> + struct prefix_list * p;
> + const char * s;
> +
> + notice ("collect2: warning: unable to find linker.\n");
> +
> +#ifdef DEFAULT_LINKER
> + notice (" Searched for this absolute executable:\n");
> + notice (" %s\n", DEFAULT_LINKER);
> +#endif
> +
> + notice (" Searched in these paths:\n");
> + for (p = cpath.plist; p != NULL; p = p->next)
> + notice (" %s\n", p->prefix);
> + notice (" For these executables:\n");
> + notice (" %s\n", real_ld_suffix);
> + notice (" %s\n", collect_ld_suffix);
> + switch (selected_linker)
> + {
> + default:
> + case DFLT_LINKER: s = ld_suffix; break;
> + case PLUGIN_LINKER: s = plugin_ld_suffix; break;
> + case GOLD_LINKER: s = gold_suffix; break;
> + case BFD_LINKER: s = bfd_ld_suffix; break;
> + }
... here...
> + notice (" %s\n", s);
> +
> + notice (" And searched in these paths:\n");
> + for (p = path.plist; p != NULL; p = p->next)
> + notice (" %s\n", p->prefix);
> + notice (" For these executables:\n");
> +#ifdef REAL_LD_FILE_NAME
> + notice (" %s\n", REAL_LD_FILE_NAME);
> +#endif
> + switch (selected_linker)
> + {
> + default:
> + case DFLT_LINKER: s = full_ld_suffix; break;
> + case PLUGIN_LINKER: s = full_plugin_ld_suffix; break;
> + case GOLD_LINKER: s = full_gold_suffix; break;
> + case BFD_LINKER: s = full_bfd_ld_suffix; break;
> + }
... and here.
> + notice (" %s\n", s);
> + }
>
> #ifdef REAL_NM_FILE_NAME
> nm_file_name = find_a_file (&path, REAL_NM_FILE_NAME);
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 4c8bd11..ca67e89 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2171,6 +2171,9 @@ funwind-tables
> Common Report Var(flag_unwind_tables) Optimization
> Just generate unwind tables for exception handling
>
> +fuse-ld=
> +Common Joined Undocumented
> +
> fuse-linker-plugin
> Common Undocumented
>
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index c6f57bd..305a1cd 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -2027,6 +2027,17 @@ else
> AC_PATH_PROG(gcc_cv_ld, $LD_FOR_TARGET)
> fi])
>
> +gcc_cv_ld_gold_srcdir=`echo $srcdir | sed -e 's,/gcc$,,'`/gold
Please do not use gcc_cv_ld_gold_srcdir, it's not a cache variable. In
fact, you can just use $srcdir/../gold/configure.ac in the "test -f"
command below.
> +AS_VAR_SET_IF(gcc_cv_gold,, [
> +if test -f $gcc_cv_ld_gold_srcdir/configure.ac \
> + && test -f ../gold/Makefile \
> + && test x$build = x$host; then
> + gcc_cv_gold=../gold/ld-new$build_exeext
> +else
> + gcc_cv_gold=''
See below, I'm not sure this default is correct.
> +fi])
> +
> ORIGINAL_PLUGIN_LD_FOR_TARGET=$gcc_cv_ld
> PLUGIN_LD_SUFFIX=`basename $gcc_cv_ld | sed -e "s,$target_alias-,,"`
> # if the PLUGIN_LD is set ld-new, just have it as ld
> @@ -2062,6 +2073,9 @@ case "$ORIGINAL_LD_FOR_TARGET" in
> *) AC_CONFIG_FILES(collect-ld:exec-tool.in, [chmod +x collect-ld]) ;;
> esac
>
> +ORIGINAL_GOLD_FOR_TARGET=$gcc_cv_gold
> +AC_SUBST(ORIGINAL_GOLD_FOR_TARGET)
> +
Please move these three lines above, in the previous hunk.
> 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.
> @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.
>
> invoked=`basename "$0"`
> id=$invoked
> @@ -36,15 +38,44 @@ 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
> prog=ld-new$exeext
> - dir=ld
> + # Look for the a command line option
> + # specifying the linker to be used.
> + case " $* " in
> + *\ -use-gold\ *)
> + original=$ORIGINAL_GOLD_FOR_TARGET
> + dir=gold
> + ;;
> + *\ -use-ld\ * | *\ -use-ld.bfd\ *)
> + original=$ORIGINAL_LD_FOR_TARGET
> + dir=ld
> + ;;
> + *\ -plugin\ *)
> + original=$ORIGINAL_PLUGIN_LD_FOR_TARGET
> + dir=ld
> + ;;
> + *)
> + original=$ORIGINAL_LD_FOR_TARGET
> + dir=ld
> + ;;
> + esac
> +
> + # If the selected linker has not been configured then
> + # try using the others, in the order PLUGIN-LD, LD, GOLD.
> + if test x"$original" = x; then
> + if test x"$ORIGINAL_PLUGIN_LD_FOR_TARGET" != x; then
> + original=$ORIGINAL_PLUGIN_LD_FOR_TARGET
> + dir=ld
> + elif test x"$ORIGINAL_LD_FOR_TARGET" != x; then
> + original=$ORIGINAL_LD_FOR_TARGET
> + dir=ld
> + elif test x"$ORIGINAL_GOLD_FOR_TARGET" != x; then
> + original=$ORIGINAL_GOLD_FOR_TARGET
> + dir=gold
> + # Otherwise do nothing - the case statement below
> + # will issue an error message for us.
> + fi
> + fi
> id=ld
> ;;
> nm)
> @@ -61,29 +92,58 @@ case "$original" in
> scriptdir=`cd "$tdir" && pwd`
>
> if test -x $scriptdir/../$dir/$prog; then
> - test "$fast_install" = yes || exec $scriptdir/../$dir/$prog ${1+"$@"}
> -
> - # if libtool did everything it needs to do, there's a fast path
> - lt_prog=$scriptdir/../$dir/$objdir/lt-$prog
> - test -x $lt_prog && exec $lt_prog ${1+"$@"}
> -
> - # libtool has not relinked ld-new yet, but we cannot just use the
> - # previous stage (because then the relinking would just never happen!).
> - # So we take extra care to use prev-ld/ld-new *on recursive calls*.
> - eval LT_RCU="\${LT_RCU_$id}"
> - test x"$LT_RCU" = x"1" && exec $scriptdir/../prev-$dir/$prog ${1+"$@"}
> -
> - eval LT_RCU_$id=1
> - export LT_RCU_$id
> - $scriptdir/../$dir/$prog ${1+"$@"}
> - result=$?
> - exit $result
> + if test "$fast_install" = yes; then
> + # If libtool did everything it needs to do, there's a fast path.
> + lt_prog=$scriptdir/../$dir/$objdir/lt-$prog
Just like the -v behavior, reversing the ifs should be a separate patch.
It is quite confusing to review this patch as you posted it.
> + if test -x $lt_prog; then
> + original=$lt_prog
> + else
> + # Libtool has not relinked ld-new yet, but we cannot just use the
> + # previous stage (because then the relinking would just never happen!).
> + # So we take extra care to use prev-ld/ld-new *on recursive calls*.
> + eval LT_RCU="\${LT_RCU_$id}"
> + if test x"$LT_RCU" = x"1"; then
> + original=$scriptdir/../prev-$dir/$prog
> + else
> + eval LT_RCU_$id=1
> + export LT_RCU_$id
> + case " $* " in
> + *\ -v\ *)
> + echo "$invoked $version"
> + echo $scriptdir/../$dir/$prog $*
> + ;;
> + esac
> + $scriptdir/../$dir/$prog ${1+"$@"}
> + result=$?
> + exit $result
> + fi
> + fi
> + else
> + original=$scriptdir/../$dir/$prog
> + fi
> else
> - exec $scriptdir/../prev-$dir/$prog ${1+"$@"}
> + original=$scriptdir/../prev-$dir/$prog
> fi
> ;;
> - *)
> - exec $original ${1+"$@"}
> + "")
> + echo "$invoked: executable not configured"
> + exit 1
> ;;
> esac
> +
> +# If -v has been used then display our version number
> +# and then echo the command we are about to invoke.
> +case " $* " in
> + *\ -v\ *)
> + echo "$invoked $version"
> + echo $original $*
> + ;;
> +esac
> +
> +if test -x $original; then
> + exec "$original" ${1+"$@"}
> +else
> + echo "$invoked: unable to locate executable: $original"
> + exit 1
> +fi
> 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?
Paolo
> "%X %{o*} %{e*} %{N} %{n} %{r}\
> %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}\
> %{static:} %{L*} %(mfwrap) %(link_libgcc) %o\
> diff --git a/gcc/opts.c b/gcc/opts.c
> index b3a9afe..ae4b61a 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1753,8 +1753,9 @@ common_handle_option (struct gcc_options *opts,
> dc->max_errors = value;
> break;
>
> + case OPT_fuse_ld_:
> case OPT_fuse_linker_plugin:
> - /* No-op. Used by the driver and passed to us because it starts with f.*/
> + /* No-op. Used by the driver and passed to us because it starts with f. */
> break;
>
> default:
> -- 1.7.11.7
>
next prev parent reply other threads:[~2012-11-28 9:10 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-27 18:36 H.J. Lu
2012-11-28 9:10 ` Paolo Bonzini [this message]
2012-11-28 19:11 ` H.J. Lu
2012-11-28 21:13 ` Paolo Bonzini
2012-11-28 21:28 ` H.J. Lu
2012-11-28 22:12 ` Joseph S. Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50B5D4D9.6000400@gnu.org \
--to=bonzini@gnu.org \
--cc=dnovillo@google.com \
--cc=doko@ubuntu.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).