| Message ID | yddd3lzxlm6.fsf@manam.CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
On 03/10/2011 12:26 PM, Rainer Orth wrote: > + # GNU gold (GNU Binutils 2.21.51.20110225) 1.11 > + # > + # We extract the binutils version which is more familiar and specific > + # than the gold version. > + ld_vers=`echo $ld_ver | sed -n \ > + -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)) .*$,\1,p'` ^^ ^^ Perhaps changing these to [^)]* is better. Build parts are okay with that change. Patch
On Thu, 10 Mar 2011, Rainer Orth wrote: > After considerable discussion in this thread > > Unreviewed build, lto patch > http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00868.html > > Richard's recommendation was to only use the lto-plugin with known-good > linkers supporting -plugin, i.e. GNU ld or gold >= 2.21. > > I'm extracting the binutils version number from gold --version since > it's more familiar than the separate gold version number and provides a > higher resolution than the gold version number that only changes once > per binutils release. > > Here's the reworked patch that implements this suggestion. I couldn't > check with gold yet since it doesn't work on Solaris so far, but > i386-pc-solaris2.11 bootstraps with Sun ld and GNU ld 2.21 work as > expected, i.e. the failure described in PR lto/46944 is gone. > > Ok for mainline? If I read this patch correctly then 1) it doesn't change the condition under which lto-plugin/ is built (good) 2) it makes -fuse-linker-plugin the default for and only for known good linkers (GNU binutils >= 2.21) (good) 3) it makes it impossible to use -fuse-linker-plugin explicitly for other linkers or linkers that were not installed during configuring gcc (bad - esp. the latter) can you please try avoiding 3) at this stage? Or is the whole point of this patch 3) to be able to fix PR46944? Ideally we'd reject broken linkers at runtime, but that would require some major collect2 massaging (eventually falling back to collect2 or simply reporting an error). That said, I'm not 100% happy with 3) at this point (though 2) is very desirable). Can we to fix 46944 change the dejagnu require-linker-plugin to check if a linker plugin is used by default and not add -fuse-linker-plugin? Thanks, Richard. > Rainer > > > 2011-02-05 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > PR lto/46944 > * configure.ac (gcc_cv_gld_major_version, gcc_cv_gld_minor): > Handle in-tree gold. > (ld_vers): Extract binutils version for gold. > (gcc_cv_ld_hidden): Handle gold here. > (gcc_cv_lto_plugin): Require gld or gold 2.21. > * configure: Regenerate. > * gcc.c [HAVE_LTO_PLUGIN] (PLUGIN_COND, PLUGIN_COND_CLOSE): Remove. > (LINK_PLUGIN_SPEC): Define. > Extract from LINK_COMMAND_SPEC, integrate PLUGIN_COND, > PLUGIN_COND_CLOSE. > [!HAVE_LTO_PLUGIN] (LINK_PLUGIN_SPEC): Define, reject > -fuse-linker-plugin. > (LINK_COMMAND_SPEC): Use it. > > diff -r bbab4a602b6f gcc/configure.ac > --- a/gcc/configure.ac Sat Feb 26 09:27:25 2011 +0100 > +++ b/gcc/configure.ac Sat Mar 05 09:36:08 2011 +0100 > @@ -1967,7 +1967,8 @@ > esac > > AC_MSG_CHECKING(what linker to use) > -if test "$gcc_cv_ld" = ../ld/ld-new$build_exeext; then > +if test "$gcc_cv_ld" = ../ld/ld-new$build_exeext \ > + || test "$gcc_cv_ld" = ../gold/ld-new$build_exeext; then > # Single tree build which includes ld. We want to prefer it > # over whatever linker top-level may have detected, since > # we'll use what we're building after installation anyway. > @@ -1978,6 +1979,8 @@ > || grep 'EMUL = .*linux' ../ld/Makefile \ > || grep 'EMUL = .*lynx' ../ld/Makefile) > /dev/null; then > in_tree_ld_is_elf=yes > + elif test "$ld_is_gold" = yes; then > + in_tree_ld_is_elf=yes > fi > for f in $gcc_cv_ld_bfd_srcdir/configure $gcc_cv_ld_gld_srcdir/configure $gcc_cv_ld_gld_srcdir/configure.in $gcc_cv_ld_gld_srcdir/Makefile.in > do > @@ -2192,11 +2195,23 @@ > changequote(,)dnl > if test $in_tree_ld != yes ; then > ld_ver=`$gcc_cv_ld --version 2>/dev/null | sed 1q` > - if test x"$ld_is_gold" = xyes; then > - gcc_cv_ld_hidden=yes > - elif echo "$ld_ver" | grep GNU > /dev/null; then > - ld_vers=`echo $ld_ver | sed -n \ > - -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)$,\1,p'` > + if echo "$ld_ver" | grep GNU > /dev/null; then > + if test x"$ld_is_gold" = xyes; then > + # GNU gold --version looks like this: > + # > + # GNU gold (GNU Binutils 2.21.51.20110225) 1.11 > + # > + # We extract the binutils version which is more familiar and specific > + # than the gold version. > + ld_vers=`echo $ld_ver | sed -n \ > + -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)) .*$,\1,p'` > + else > + # GNU ld --version looks like this: > + # > + # GNU ld (GNU Binutils) 2.21.51.20110225 > + ld_vers=`echo $ld_ver | sed -n \ > + -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)$,\1,p'` > + fi > ld_date=`echo $ld_ver | sed -n 's,^.*\([2-9][0-9][0-9][0-9]\)[-]*\([01][0-9]\)[-]*\([0-3][0-9]\).*$,\1\2\3,p'` > ld_vers_major=`expr "$ld_vers" : '\([0-9]*\)'` > ld_vers_minor=`expr "$ld_vers" : '[0-9]*\.\([0-9]*\)'` > @@ -2235,7 +2250,9 @@ > fi > else > gcc_cv_ld_hidden=yes > - if echo "$ld_ver" | grep GNU > /dev/null; then > + if test x"$ld_is_gold" = xyes; then > + : > + elif echo "$ld_ver" | grep GNU > /dev/null; then > if test 0"$ld_date" -lt 20020404; then > if test -n "$ld_date"; then > # If there was date string, but was earlier than 2002-04-04, fail > @@ -3176,14 +3193,14 @@ > gcc_cv_lto_plugin=no > if test -f liblto_plugin.la; then > if test $in_tree_ld = yes -a x"$ORIGINAL_PLUGIN_LD_FOR_TARGET=" = x"$gcc_cv_ld"; then > - if test x"$ld_is_gold" = xyes; then > - gcc_cv_lto_plugin=yes > - elif test "$gcc_cv_gld_major_version" -eq 2 -a "$gcc_cv_gld_minor_version" -ge 21 -o "$gcc_cv_gld_major_version" -gt 2; then \ > + if test "$gcc_cv_gld_major_version" -eq 2 -a "$gcc_cv_gld_minor_version" -ge 21 -o "$gcc_cv_gld_major_version" -gt 2; then > gcc_cv_lto_plugin=yes > fi > - # Check if the linker supports --plugin-opt option > - elif $ORIGINAL_PLUGIN_LD_FOR_TARGET --help 2>/dev/null | grep plugin-opt > /dev/null; then > - gcc_cv_lto_plugin=yes > + elif echo "$ld_ver" | grep GNU > /dev/null; then > + # Require GNU ld or gold 2.21 for plugin support. > + if test "$ld_vers_major" -eq 2 -a "$ld_vers_minor" -ge 21; then > + gcc_cv_lto_plugin=yes > + fi > fi > fi > if test x"$gcc_cv_lto_plugin" = xyes; then > diff -r bbab4a602b6f gcc/gcc.c > --- a/gcc/gcc.c Sat Feb 26 09:27:25 2011 +0100 > +++ b/gcc/gcc.c Sat Mar 05 09:36:08 2011 +0100 > @@ -623,16 +623,20 @@ > > /* Conditional to test whether plugin is used or not. > FIXME: For slim LTO we will need to enable plugin unconditionally. This > - still cause problems with PLUGIN_LD != LD and when plugin is built but > + still causes problems with PLUGIN_LD != LD and when plugin is built but > not useable. For GCC 4.6 we don't support slim LTO and thus we can enable > - plugin only when LTO is enabled. We still honor explicit > - -fuse-linker-plugin. */ > + plugin only when LTO is enabled. */ > #ifdef HAVE_LTO_PLUGIN > -#define PLUGIN_COND "!fno-use-linker-plugin:%{flto|flto=*|fuse-linker-plugin" > -#define PLUGIN_COND_CLOSE "}" > +#define LINK_PLUGIN_SPEC \ > + "%{!fno-use-linker-plugin:%{flto|flto=*|fuse-linker-plugin: \ > + -plugin %(linker_plugin_file) \ > + -plugin-opt=%(lto_wrapper) \ > + -plugin-opt=-fresolution=%u.res \ > + %{!nostdlib:%{!nodefaultlibs:%:pass-through-libs(%(link_gcc_c_sequence))}} \ > + }}" > #else > -#define PLUGIN_COND "fuse-linker-plugin" > -#define PLUGIN_COND_CLOSE "" > +#define LINK_PLUGIN_SPEC "%{fuse-linker-plugin:\ > + %e-fuse-linker-plugin is not supported in this configuration}" > #endif > > > @@ -648,14 +652,9 @@ > #ifndef LINK_COMMAND_SPEC > #define LINK_COMMAND_SPEC "\ > %{!fsyntax-only:%{!c:%{!M:%{!MM:%{!E:%{!S:\ > - %(linker) \ > - %{"PLUGIN_COND": \ > - -plugin %(linker_plugin_file) \ > - -plugin-opt=%(lto_wrapper) \ > - -plugin-opt=-fresolution=%u.res \ > - %{!nostdlib:%{!nodefaultlibs:%:pass-through-libs(%(link_gcc_c_sequence))}} \ > - }"PLUGIN_COND_CLOSE" \ > - %{flto|flto=*:%<fcompare-debug*} \ > + %(linker) " \ > + LINK_PLUGIN_SPEC \ > + "%{flto|flto=*:%<fcompare-debug*} \ > %{flto} %{flto=*} %l " LINK_PIE_SPEC \ > "%X %{o*} %{e*} %{N} %{n} %{r}\ > %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}\ > > >
Richard Guenther <rguenther@suse.de> writes: > If I read this patch correctly then > > 1) it doesn't change the condition under which lto-plugin/ is built > (good) Right. > 2) it makes -fuse-linker-plugin the default for and only for > known good linkers (GNU binutils >= 2.21) (good) Indeed. > 3) it makes it impossible to use -fuse-linker-plugin explicitly > for other linkers or linkers that were not installed during > configuring gcc (bad - esp. the latter) > > can you please try avoiding 3) at this stage? Or is the whole > point of this patch 3) to be able to fix PR46944? That was my goal: the Solaris linker accepts -p <auditlib>, which causes confusion when it is called with -plugin. > Ideally we'd reject broken linkers at runtime, but that would > require some major collect2 massaging (eventually falling back > to collect2 or simply reporting an error). What about making LTO_PLUGIN 3-valued? 2 linker used has full -plugin support, i.e. gld/gold >= 2.21 1 linker has some -plugin support, i.e. gold >= 2.20 < 2.21 0 linker has no known plugin support, i.e. everything else, in particular vendor linkers We'd default to -fuse-linker-plugin for 2, accept it if given explicitly for 1, and reject it for 0. This would be similar to the first version of my patch, with the difference that we don't try to determine the level of -plugin support from trying to run the configured linker with -plugin and check if it works, but instead hardcode that knowledge. > That said, I'm not 100% happy with 3) at this point (though > 2) is very desirable). > > Can we to fix 46944 change the dejagnu require-linker-plugin > to check if a linker plugin is used by default and not add > -fuse-linker-plugin? That might be involved since it requires parsing gcc -Wl,-debug output. I suppose the solution outlined above is simpler and thus more robust. Rainer
On Thu, 10 Mar 2011, Rainer Orth wrote: > Richard Guenther <rguenther@suse.de> writes: > > > If I read this patch correctly then > > > > 1) it doesn't change the condition under which lto-plugin/ is built > > (good) > > Right. > > > 2) it makes -fuse-linker-plugin the default for and only for > > known good linkers (GNU binutils >= 2.21) (good) > > Indeed. > > > 3) it makes it impossible to use -fuse-linker-plugin explicitly > > for other linkers or linkers that were not installed during > > configuring gcc (bad - esp. the latter) > > > > can you please try avoiding 3) at this stage? Or is the whole > > point of this patch 3) to be able to fix PR46944? > > That was my goal: the Solaris linker accepts -p <auditlib>, which causes > confusion when it is called with -plugin. > > > Ideally we'd reject broken linkers at runtime, but that would > > require some major collect2 massaging (eventually falling back > > to collect2 or simply reporting an error). > > What about making LTO_PLUGIN 3-valued? > > 2 linker used has full -plugin support, i.e. gld/gold >= 2.21 > 1 linker has some -plugin support, i.e. gold >= 2.20 < 2.21 > 0 linker has no known plugin support, i.e. everything else, in > particular vendor linkers > > We'd default to -fuse-linker-plugin for 2, accept it if given explicitly > for 1, and reject it for 0. > > This would be similar to the first version of my patch, with the > difference that we don't try to determine the level of -plugin support > from trying to run the configured linker with -plugin and check if it > works, but instead hardcode that knowledge. > > > That said, I'm not 100% happy with 3) at this point (though > > 2) is very desirable). > > > > Can we to fix 46944 change the dejagnu require-linker-plugin > > to check if a linker plugin is used by default and not add > > -fuse-linker-plugin? > > That might be involved since it requires parsing gcc -Wl,-debug output. > I suppose the solution outlined above is simpler and thus more robust. It might be as simple as int res; int main() { int x; asm ("mov res, %0" : x(g)); return x; } which should fail to link with the plugin only (but yes, requies target dependent assembly ...). Or, use -save-temps and check for the existence of the resolution file for int main() {}. It should be named t.res for gcc -o t t.c -flto. Richard.
Richard Guenther <rguenther@suse.de> writes: >> > Can we to fix 46944 change the dejagnu require-linker-plugin >> > to check if a linker plugin is used by default and not add >> > -fuse-linker-plugin? >> >> That might be involved since it requires parsing gcc -Wl,-debug output. >> I suppose the solution outlined above is simpler and thus more robust. > > It might be as simple as > > int res; > int main() { int x; asm ("mov res, %0" : x(g)); return x; } > > which should fail to link with the plugin only (but yes, requies > target dependent assembly ...). ... which I'd consider far too complicated/hard to maintain to consider. > Or, use -save-temps and check for the existence of the resolution > file for int main() {}. It should be named t.res for gcc -o t t.c -flto. Only with -save-temps, otherwise it's some random file in /var/tmp. But even so the file is removed immediately. And even if we decide to fix PR lto/46944 like this, we're still left with the problem of users invoking gcc with -fuse-linker-plugin and getting either strange errors or no effect instead of a clear diagnostic. Rainer
diff -r bbab4a602b6f gcc/configure.ac --- a/gcc/configure.ac Sat Feb 26 09:27:25 2011 +0100 +++ b/gcc/configure.ac Sat Mar 05 09:36:08 2011 +0100 @@ -1967,7 +1967,8 @@ esac AC_MSG_CHECKING(what linker to use) -if test "$gcc_cv_ld" = ../ld/ld-new$build_exeext; then +if test "$gcc_cv_ld" = ../ld/ld-new$build_exeext \ + || test "$gcc_cv_ld" = ../gold/ld-new$build_exeext; then # Single tree build which includes ld. We want to prefer it # over whatever linker top-level may have detected, since # we'll use what we're building after installation anyway. @@ -1978,6 +1979,8 @@ || grep 'EMUL = .*linux' ../ld/Makefile \ || grep 'EMUL = .*lynx' ../ld/Makefile) > /dev/null; then in_tree_ld_is_elf=yes + elif test "$ld_is_gold" = yes; then + in_tree_ld_is_elf=yes fi for f in $gcc_cv_ld_bfd_srcdir/configure $gcc_cv_ld_gld_srcdir/configure $gcc_cv_ld_gld_srcdir/configure.in $gcc_cv_ld_gld_srcdir/Makefile.in do @@ -2192,11 +2195,23 @@ changequote(,)dnl if test $in_tree_ld != yes ; then ld_ver=`$gcc_cv_ld --version 2>/dev/null | sed 1q` - if test x"$ld_is_gold" = xyes; then - gcc_cv_ld_hidden=yes - elif echo "$ld_ver" | grep GNU > /dev/null; then - ld_vers=`echo $ld_ver | sed -n \ - -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)$,\1,p'` + if echo "$ld_ver" | grep GNU > /dev/null; then + if test x"$ld_is_gold" = xyes; then + # GNU gold --version looks like this: + # + # GNU gold (GNU Binutils 2.21.51.20110225) 1.11 + # + # We extract the binutils version which is more familiar and specific + # than the gold version. + ld_vers=`echo $ld_ver | sed -n \ + -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)) .*$,\1,p'` + else + # GNU ld --version looks like this: + # + # GNU ld (GNU Binutils) 2.21.51.20110225 + ld_vers=`echo $ld_ver | sed -n \ + -e 's,^.*[ ]\([0-9][0-9]*\.[0-9][0-9]*.*\)$,\1,p'` + fi ld_date=`echo $ld_ver | sed -n 's,^.*\([2-9][0-9][0-9][0-9]\)[-]*\([01][0-9]\)[-]*\([0-3][0-9]\).*$,\1\2\3,p'` ld_vers_major=`expr "$ld_vers" : '\([0-9]*\)'` ld_vers_minor=`expr "$ld_vers" : '[0-9]*\.\([0-9]*\)'` @@ -2235,7 +2250,9 @@ fi else gcc_cv_ld_hidden=yes - if echo "$ld_ver" | grep GNU > /dev/null; then + if test x"$ld_is_gold" = xyes; then + : + elif echo "$ld_ver" | grep GNU > /dev/null; then if test 0"$ld_date" -lt 20020404; then if test -n "$ld_date"; then # If there was date string, but was earlier than 2002-04-04, fail @@ -3176,14 +3193,14 @@ gcc_cv_lto_plugin=no if test -f liblto_plugin.la; then if test $in_tree_ld = yes -a x"$ORIGINAL_PLUGIN_LD_FOR_TARGET=" = x"$gcc_cv_ld"; then - if test x"$ld_is_gold" = xyes; then - gcc_cv_lto_plugin=yes - elif test "$gcc_cv_gld_major_version" -eq 2 -a "$gcc_cv_gld_minor_version" -ge 21 -o "$gcc_cv_gld_major_version" -gt 2; then \ + if test "$gcc_cv_gld_major_version" -eq 2 -a "$gcc_cv_gld_minor_version" -ge 21 -o "$gcc_cv_gld_major_version" -gt 2; then gcc_cv_lto_plugin=yes fi - # Check if the linker supports --plugin-opt option - elif $ORIGINAL_PLUGIN_LD_FOR_TARGET --help 2>/dev/null | grep plugin-opt > /dev/null; then - gcc_cv_lto_plugin=yes + elif echo "$ld_ver" | grep GNU > /dev/null; then + # Require GNU ld or gold 2.21 for plugin support. + if test "$ld_vers_major" -eq 2 -a "$ld_vers_minor" -ge 21; then + gcc_cv_lto_plugin=yes + fi fi fi if test x"$gcc_cv_lto_plugin" = xyes; then diff -r bbab4a602b6f gcc/gcc.c --- a/gcc/gcc.c Sat Feb 26 09:27:25 2011 +0100 +++ b/gcc/gcc.c Sat Mar 05 09:36:08 2011 +0100 @@ -623,16 +623,20 @@ /* Conditional to test whether plugin is used or not. FIXME: For slim LTO we will need to enable plugin unconditionally. This - still cause problems with PLUGIN_LD != LD and when plugin is built but + still causes problems with PLUGIN_LD != LD and when plugin is built but not useable. For GCC 4.6 we don't support slim LTO and thus we can enable - plugin only when LTO is enabled. We still honor explicit - -fuse-linker-plugin. */ + plugin only when LTO is enabled. */ #ifdef HAVE_LTO_PLUGIN -#define PLUGIN_COND "!fno-use-linker-plugin:%{flto|flto=*|fuse-linker-plugin" -#define PLUGIN_COND_CLOSE "}" +#define LINK_PLUGIN_SPEC \ + "%{!fno-use-linker-plugin:%{flto|flto=*|fuse-linker-plugin: \ + -plugin %(linker_plugin_file) \ + -plugin-opt=%(lto_wrapper) \ + -plugin-opt=-fresolution=%u.res \ + %{!nostdlib:%{!nodefaultlibs:%:pass-through-libs(%(link_gcc_c_sequence))}} \ + }}" #else -#define PLUGIN_COND "fuse-linker-plugin" -#define PLUGIN_COND_CLOSE "" +#define LINK_PLUGIN_SPEC "%{fuse-linker-plugin:\ + %e-fuse-linker-plugin is not supported in this configuration}" #endif @@ -648,14 +652,9 @@ #ifndef LINK_COMMAND_SPEC #define LINK_COMMAND_SPEC "\ %{!fsyntax-only:%{!c:%{!M:%{!MM:%{!E:%{!S:\ - %(linker) \ - %{"PLUGIN_COND": \ - -plugin %(linker_plugin_file) \ - -plugin-opt=%(lto_wrapper) \ - -plugin-opt=-fresolution=%u.res \ - %{!nostdlib:%{!nodefaultlibs:%:pass-through-libs(%(link_gcc_c_sequence))}} \ - }"PLUGIN_COND_CLOSE" \ - %{flto|flto=*:%<fcompare-debug*} \ + %(linker) " \ + LINK_PLUGIN_SPEC \ + "%{flto|flto=*:%<fcompare-debug*} \ %{flto} %{flto=*} %l " LINK_PIE_SPEC \ "%X %{o*} %{e*} %{N} %{n} %{r}\ %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}}\
After considerable discussion in this thread Unreviewed build, lto patch http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00868.html Richard's recommendation was to only use the lto-plugin with known-good linkers supporting -plugin, i.e. GNU ld or gold >= 2.21. I'm extracting the binutils version number from gold --version since it's more familiar than the separate gold version number and provides a higher resolution than the gold version number that only changes once per binutils release. Here's the reworked patch that implements this suggestion. I couldn't check with gold yet since it doesn't work on Solaris so far, but i386-pc-solaris2.11 bootstraps with Sun ld and GNU ld 2.21 work as expected, i.e. the failure described in PR lto/46944 is gone. Ok for mainline? Rainer 2011-02-05 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> PR lto/46944 * configure.ac (gcc_cv_gld_major_version, gcc_cv_gld_minor): Handle in-tree gold. (ld_vers): Extract binutils version for gold. (gcc_cv_ld_hidden): Handle gold here. (gcc_cv_lto_plugin): Require gld or gold 2.21. * configure: Regenerate. * gcc.c [HAVE_LTO_PLUGIN] (PLUGIN_COND, PLUGIN_COND_CLOSE): Remove. (LINK_PLUGIN_SPEC): Define. Extract from LINK_COMMAND_SPEC, integrate PLUGIN_COND, PLUGIN_COND_CLOSE. [!HAVE_LTO_PLUGIN] (LINK_PLUGIN_SPEC): Define, reject -fuse-linker-plugin. (LINK_COMMAND_SPEC): Use it.