| Message ID | 20110404181738.52D661909EB@elbrus2.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
ppluzhnikov@google.com (Paul Pluzhnikov) writes: > Several Linux distributions (e.g. Fedora) carry local patches that turn > on --hash-style=gnu for all links. Shouldn't configure verify that the linker used actually understands that option? Rainer
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > ppluzhnikov@google.com (Paul Pluzhnikov) writes: > >> Several Linux distributions (e.g. Fedora) carry local patches that turn >> on --hash-style=gnu for all links. > > Shouldn't configure verify that the linker used actually understands > that option? No, I don't think so. This is an explicit configure option. People who use explicit configure options should get what they request, whether or not it actually works. Ian
On 04.04.2011 20:17, Paul Pluzhnikov wrote: > Greetings, > > Several Linux distributions (e.g. Fedora) carry local patches that turn > on --hash-style=gnu for all links. > > Attached is a proposed patch (originally by Satoru Takabayashi) that makes > default hash style a configure option. > > Tested by doing native bootstrap and verifying that no --hash-style is > passed to the linker, and also configuring with --with-linker-hash-style=gnu > and verifying that --hash-style=gnu is then passed to the linker. Linux distributions pass more than that by default to the linker, e.g. --as-needed and --no-copy-dt-needed-entries. Wouldn't it make more sense to add something like --with-linker-default-options=... ? Matthias
On Tue, 5 Apr 2011, Matthias Klose wrote: > On 04.04.2011 20:17, Paul Pluzhnikov wrote: > > Greetings, > > > > Several Linux distributions (e.g. Fedora) carry local patches that turn > > on --hash-style=gnu for all links. > > > > Attached is a proposed patch (originally by Satoru Takabayashi) that makes > > default hash style a configure option. > > > > Tested by doing native bootstrap and verifying that no --hash-style is > > passed to the linker, and also configuring with --with-linker-hash-style=gnu > > and verifying that --hash-style=gnu is then passed to the linker. > > Linux distributions pass more than that by default to the linker, e.g. > --as-needed and --no-copy-dt-needed-entries. Wouldn't it make more sense to add > something like --with-linker-default-options=... ? We have --with-specs that configures specs for the driver's own command line - maybe there's a use for something like that for other specs. But that's very much a last-resort option where there isn't a more structured way of configuring something; I'd prefer common use cases to be easier than that.
Ping? On Mon, Apr 4, 2011 at 5:01 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Tue, 5 Apr 2011, Matthias Klose wrote: ... >> Linux distributions pass more than that by default to the linker, e.g. >> --as-needed and --no-copy-dt-needed-entries. Wouldn't it make more sense to add >> something like --with-linker-default-options=... ? > > We have --with-specs that configures specs for the driver's own command > line - maybe there's a use for something like that for other specs. But > that's very much a last-resort option where there isn't a more structured > way of configuring something; I'd prefer common use cases to be easier > than that. The first version of this patch Satoru proposed was a general "do anything to specs" patch. Ian voted that down as being too generic and difficult to use correctly. Thanks,
Ping? Ping?
On Mon, Apr 11, 2011 at 11:00 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:
> Ping?
Ping? Ping? Ping? On Mon, Apr 18, 2011 at 9:45 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > Ping? Ping? > > On Mon, Apr 11, 2011 at 11:00 AM, Paul Pluzhnikov > <ppluzhnikov@google.com> wrote: >> Ping?
Ping? Ping? Ping? Ping? This is getting ridiculous. Would someone please accept the patch, tell me what to fix in it to make it acceptable, or explain why it is a bad idea? Thanks! On Mon, Apr 25, 2011 at 9:08 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > Ping? Ping? Ping? > > On Mon, Apr 18, 2011 at 9:45 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >> Ping? Ping? >> >> On Mon, Apr 11, 2011 at 11:00 AM, Paul Pluzhnikov >> <ppluzhnikov@google.com> wrote: >>> Ping? > > -- > Paul Pluzhnikov >
On Mon, 2 May 2011, Paul Pluzhnikov wrote: > Ping? Ping? Ping? Ping? > > This is getting ridiculous. Would someone please accept the patch, > tell me what to fix in it to make it acceptable, or explain why it is > a bad idea? When pinging, please include the URL of the patch being pinged and CC relevant maintainers (in this case, build system maintainers).
On Mon, May 2, 2011 at 7:59 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: Thanks for your comments. > When pinging, please include the URL of the patch being pinged and CC http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html > relevant maintainers (in this case, build system maintainers). All of them? [I've started with two ...] Thanks!
Ping? Ping? Ping? Ping? Ping? http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html CC'ing the rest of build system maintainers. On Mon, May 2, 2011 at 8:56 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Mon, May 2, 2011 at 7:59 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: > > Thanks for your comments. > >> When pinging, please include the URL of the patch being pinged and CC > > http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html > >> relevant maintainers (in this case, build system maintainers). > > All of them? [I've started with two ...] Thanks!
On 05/09/2011 05:59 PM, Paul Pluzhnikov wrote: > Ping? Ping? Ping? Ping? Ping? > > http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html > > CC'ing the rest of build system maintainers. None of the build system maintainers can approve gcc.c changes. And those can be approved only by either a global reviewer, or by Joseph. That's why I haven't replied anything up to now. Paolo
On Mon, 9 May 2011, Paolo Bonzini wrote: > On 05/09/2011 05:59 PM, Paul Pluzhnikov wrote: > > Ping? Ping? Ping? Ping? Ping? > > > > http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html > > > > CC'ing the rest of build system maintainers. > > None of the build system maintainers can approve gcc.c changes. And those can > be approved only by either a global reviewer, or by Joseph. That's why I > haven't replied anything up to now. I'm thinking of it as a build-system patch with a driver bit - where build system maintainers need to decide the general principle of the desirability of the feature and what all of the implementation outside gcc.c should look like, before it makes sense to review the details of the gcc.c bit.
On Mon, May 9, 2011 at 18:45, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Mon, 9 May 2011, Paolo Bonzini wrote: > >> On 05/09/2011 05:59 PM, Paul Pluzhnikov wrote: >> > Ping? Ping? Ping? Ping? Ping? >> > >> > http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html >> > >> > CC'ing the rest of build system maintainers. >> >> None of the build system maintainers can approve gcc.c changes. And those can >> be approved only by either a global reviewer, or by Joseph. That's why I >> haven't replied anything up to now. > > I'm thinking of it as a build-system patch with a driver bit - where build > system maintainers need to decide the general principle of the > desirability of the feature and what all of the implementation outside > gcc.c should look like, before it makes sense to review the details of the > gcc.c bit. Uhm, so we deadlocked, I thought the other way. I cannot really express any opinion about the desirability of the feature, but the configure syntax is certainly okay with me, and I gather from the thread that you are fine with that as well. Paolo
On Mon, 9 May 2011, Paolo Bonzini wrote: > On Mon, May 9, 2011 at 18:45, Joseph S. Myers <joseph@codesourcery.com> wrote: > > On Mon, 9 May 2011, Paolo Bonzini wrote: > > > >> On 05/09/2011 05:59 PM, Paul Pluzhnikov wrote: > >> > Ping? Ping? Ping? Ping? Ping? > >> > > >> > http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00246.html > >> > > >> > CC'ing the rest of build system maintainers. > >> > >> None of the build system maintainers can approve gcc.c changes. And those can > >> be approved only by either a global reviewer, or by Joseph. That's why I > >> haven't replied anything up to now. > > > > I'm thinking of it as a build-system patch with a driver bit - where build > > system maintainers need to decide the general principle of the > > desirability of the feature and what all of the implementation outside > > gcc.c should look like, before it makes sense to review the details of the > > gcc.c bit. > > Uhm, so we deadlocked, I thought the other way. I cannot really > express any opinion about the desirability of the feature, but the > configure syntax is certainly okay with me, and I gather from the > thread that you are fine with that as well. Given the build system changes, the gcc.c changes are OK.
On Mon, May 9, 2011 at 9:51 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Mon, 9 May 2011, Paolo Bonzini wrote: > >> Uhm, so we deadlocked, I thought the other way. I cannot really >> express any opinion about the desirability of the feature, but the >> configure syntax is certainly okay with me, and I gather from the >> thread that you are fine with that as well. > > Given the build system changes, the gcc.c changes are OK. Ok for trunk then? I'll wait till tomorrow in case someone has additional comments on the desirability part. Thanks!
On Mon, May 9, 2011 at 7:11 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Mon, May 9, 2011 at 9:51 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: >> On Mon, 9 May 2011, Paolo Bonzini wrote: >> >>> Uhm, so we deadlocked, I thought the other way. I cannot really >>> express any opinion about the desirability of the feature, but the >>> configure syntax is certainly okay with me, and I gather from the >>> thread that you are fine with that as well. >> >> Given the build system changes, the gcc.c changes are OK. > > Ok for trunk then? > > I'll wait till tomorrow in case someone has additional comments on the > desirability part. I wonder why this is a GCC specific patch and not a linker patch. Why not change the linker(s) to accept such configure option that changes its default behavior? Otherwise if people link with ld they suddenly get different hash-style. That looks wrong to me. Richard. > Thanks! > -- > Paul Pluzhnikov >
Richard Guenther <richard.guenther@gmail.com> writes: > On Mon, May 9, 2011 at 7:11 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >> On Mon, May 9, 2011 at 9:51 AM, Joseph S. Myers <joseph@codesourcery.com> wrote: >>> On Mon, 9 May 2011, Paolo Bonzini wrote: >>> >>>> Uhm, so we deadlocked, I thought the other way. I cannot really >>>> express any opinion about the desirability of the feature, but the >>>> configure syntax is certainly okay with me, and I gather from the >>>> thread that you are fine with that as well. >>> >>> Given the build system changes, the gcc.c changes are OK. >> >> Ok for trunk then? >> >> I'll wait till tomorrow in case someone has additional comments on the >> desirability part. > > I wonder why this is a GCC specific patch and not a linker patch. Why > not change the linker(s) to accept such configure option that changes its > default behavior? It is traditionally gcc which tells the linker what to do. E.g., Fedora has patched gcc to pass --hash-style=gnu to the linker. > Otherwise if people link with ld they suddenly get different hash-style. > That looks wrong to me. That turns out not to be the case. Both gold and GNU ld accept the same set of --hash-style options. Ian
On Tue, May 10, 2011 at 07:02:44AM -0700, Ian Lance Taylor wrote: > > Otherwise if people link with ld they suddenly get different hash-style. > > That looks wrong to me. > > That turns out not to be the case. Both gold and GNU ld accept the same > set of --hash-style options. And we keep telling people they shouldn't be invoking ld directly, they usually don't pass correct arguments. Jakub
On Tue, May 10, 2011 at 7:02 AM, Ian Lance Taylor <iant@google.com> wrote: > Richard Guenther <richard.guenther@gmail.com> writes: >> I wonder why this is a GCC specific patch and not a linker patch. Why >> not change the linker(s) to accept such configure option that changes its >> default behavior? > > It is traditionally gcc which tells the linker what to do. E.g., Fedora > has patched gcc to pass --hash-style=gnu to the linker. I think 'traditionally' is the key here. The same argument applies to e.g. --build-id. We already have GCC supply that, but we could have changed the default in binutils/{ld,gold}. Also, the argument is discoverable (by observing 'gcc -v' output). Changing the default in {ld,gold} makes it hard(er) to understand why e.g. binaries linked from the same object files using two different versions of GCC are different. >> Otherwise if people link with ld they suddenly get different hash-style. >> That looks wrong to me. > > That turns out not to be the case. I think Richard meant "if people link directly with either ld or gold (i.e. bypassing 'gcc' driver) get different hash-style". That's kind of expected though -- if you want to get the same output you get from 'gcc', you need to examine 'gcc -v' very carefully. And, as Jakub noted, linking directly with 'ld' is discouraged. Thanks,
On Tue, May 10, 2011 at 4:24 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > On Tue, May 10, 2011 at 7:02 AM, Ian Lance Taylor <iant@google.com> wrote: >> Richard Guenther <richard.guenther@gmail.com> writes: > >>> I wonder why this is a GCC specific patch and not a linker patch. Why >>> not change the linker(s) to accept such configure option that changes its >>> default behavior? >> >> It is traditionally gcc which tells the linker what to do. E.g., Fedora >> has patched gcc to pass --hash-style=gnu to the linker. > > I think 'traditionally' is the key here. > > The same argument applies to e.g. --build-id. We already have GCC > supply that, but we could have changed the default in > binutils/{ld,gold}. I think we patched binutils for hash-style (we use =both) and gcc for build-id. Richard. > Also, the argument is discoverable (by observing 'gcc -v' output). > Changing the default in {ld,gold} makes it hard(er) to understand why > e.g. binaries linked from the same object files using two different > versions of GCC are different. > >>> Otherwise if people link with ld they suddenly get different hash-style. >>> That looks wrong to me. >> >> That turns out not to be the case. > > I think Richard meant "if people link directly with either ld or gold > (i.e. bypassing 'gcc' driver) get different hash-style". That's kind > of expected though -- if you want to get the same output you get from > 'gcc', you need to examine 'gcc -v' very carefully. And, as Jakub > noted, linking directly with 'ld' is discouraged. > > Thanks, > -- > Paul Pluzhnikov >
> 2011-04-04 Satoru Takabayashi <satorux@google.com> > Paul Pluzhnikov <ppluzhnikov@google.com> > > * gcc/doc/install.texi (Configuration): Document > --with-linker-hash-style. > * gcc/gcc.c (init_spec): Handle LINKER_HASH_STYLE. > * gcc/config.in: Add LINKER_HASH_STYLE. > * gcc/configure.ac: Add --with-linker-hash-style. > * gcc/configure: Regenerate. No gcc/ prefix in the ChangeLog file of the gcc/ directory. See other entries.
On Wed, May 11, 2011 at 4:01 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> No gcc/ prefix in the ChangeLog file of the gcc/ directory. See other entries.
Fixed. Sorry about that.
Index: gcc/doc/install.texi =================================================================== --- gcc/doc/install.texi (revision 171942) +++ gcc/doc/install.texi (working copy) @@ -1657,6 +1657,11 @@ support @option{--build-id} option, a warning is issued and the @option{--enable-linker-build-id} option is ignored. The default is off. +@item --with-linker-hash-style=@var{choice} +Tells GCC to pass @option{--hash-style=@var{choice}} option to the +linker for all final links. @var{choice} can be one of +@samp{sysv}, @samp{gnu}, and @samp{both} where @samp{sysv} is the default. + @item --enable-gnu-unique-object @itemx --disable-gnu-unique-object Tells GCC to use the gnu_unique_object relocation for C++ template Index: gcc/gcc.c =================================================================== --- gcc/gcc.c (revision 171942) +++ gcc/gcc.c (working copy) @@ -1438,7 +1438,8 @@ } #endif -#if defined LINK_EH_SPEC || defined LINK_BUILDID_SPEC +#if defined LINK_EH_SPEC || defined LINK_BUILDID_SPEC || \ + defined LINKER_HASH_STYLE # ifdef LINK_BUILDID_SPEC /* Prepend LINK_BUILDID_SPEC to whatever link_spec we had before. */ obstack_grow (&obstack, LINK_BUILDID_SPEC, sizeof(LINK_BUILDID_SPEC) - 1); @@ -1447,6 +1448,16 @@ /* Prepend LINK_EH_SPEC to whatever link_spec we had before. */ obstack_grow (&obstack, LINK_EH_SPEC, sizeof(LINK_EH_SPEC) - 1); # endif +# ifdef LINKER_HASH_STYLE + /* Prepend --hash-style=LINKER_HASH_STYLE to whatever link_spec we had + before. */ + { + static const char hash_style[] = "--hash-style="; + obstack_grow (&obstack, hash_style, sizeof(hash_style) - 1); + obstack_grow (&obstack, LINKER_HASH_STYLE, sizeof(LINKER_HASH_STYLE) - 1); + obstack_1grow (&obstack, ' '); + } +# endif obstack_grow0 (&obstack, link_spec, strlen (link_spec)); link_spec = XOBFINISH (&obstack, const char *); #endif Index: gcc/config.in =================================================================== --- gcc/config.in (revision 171942) +++ gcc/config.in (working copy) @@ -1580,6 +1580,12 @@ #endif +/* The linker hash style */ +#ifndef USED_FOR_TARGET +#undef LINKER_HASH_STYLE +#endif + + /* Define to the name of the LTO plugin DSO that must be passed to the linker's -plugin=LIB option. */ #ifndef USED_FOR_TARGET Index: gcc/configure.ac =================================================================== --- gcc/configure.ac (revision 171942) +++ gcc/configure.ac (working copy) @@ -4936,6 +4936,30 @@ fi +# Specify what hash style to use by default. +AC_ARG_WITH([linker-hash-style], +[AC_HELP_STRING([--with-linker-hash-style={sysv,gnu,both}], + [specify the linker hash style])], +[case x"$withval" in + xsysv) + LINKER_HASH_STYLE=sysv + ;; + xgnu) + LINKER_HASH_STYLE=gnu + ;; + xboth) + LINKER_HASH_STYLE=both + ;; + *) + AC_MSG_ERROR([$withval is an invalid option to --with-linker-hash-style]) + ;; + esac], +[LINKER_HASH_STYLE='']) +if test x"${LINKER_HASH_STYLE}" != x; then + AC_DEFINE_UNQUOTED(LINKER_HASH_STYLE, "$LINKER_HASH_STYLE", + [The linker hash style]) +fi + # Configure the subdirectories # AC_CONFIG_SUBDIRS($subdirs) Index: gcc/configure =================================================================== --- gcc/configure (revision 171942) +++ gcc/configure (working copy) @@ -913,6 +913,7 @@ with_slibdir enable_plugin enable_libquadmath_support +with_linker_hash_style ' ac_precious_vars='build_alias host_alias @@ -1663,6 +1664,8 @@ with the compiler --with-system-zlib use installed libz --with-slibdir=DIR shared libraries in DIR [LIBDIR] + --with-linker-hash-style={sysv,gnu,both} + specify the linker hash style Some influential environment variables: CC C compiler command @@ -17505,7 +17508,7 @@ lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 17508 "configure" +#line 17511 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -17611,7 +17614,7 @@ lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 17614 "configure" +#line 17617 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -26461,6 +26464,36 @@ fi +# Specify what hash style to use by default. + +# Check whether --with-linker-hash-style was given. +if test "${with_linker_hash_style+set}" = set; then : + withval=$with_linker_hash_style; case x"$withval" in + xsysv) + LINKER_HASH_STYLE=sysv + ;; + xgnu) + LINKER_HASH_STYLE=gnu + ;; + xboth) + LINKER_HASH_STYLE=both + ;; + *) + as_fn_error "$withval is an invalid option to --with-linker-hash-style" "$LINENO" 5 + ;; + esac +else + LINKER_HASH_STYLE='' +fi + +if test x"${LINKER_HASH_STYLE}" != x; then + +cat >>confdefs.h <<_ACEOF +#define LINKER_HASH_STYLE "$LINKER_HASH_STYLE" +_ACEOF + +fi + # Configure the subdirectories # AC_CONFIG_SUBDIRS($subdirs)