diff mbox

PR66870 PowerPC64 Enable gold linker with split stack

Message ID 55DF7A72.2040208@linux.vnet.ibm.com
State New
Headers show

Commit Message

Lynn A. Boger Aug. 27, 2015, 9 p.m. UTC 
Here is an updated patch, with a summary of the differences from my previous
patch:

- In my previous patch gcc configure was verifying the gold linker even 
if it was the
default linker, but that is not necessary since in that case 
-fuse-ld=gold does not
need to be set.  Gold version checking is now only done if the alternate 
linker is gold
and the default linker is not.
- The STACK_SPLIT_STACK spec define found in gcc/gcc.c now adds 
-fuse-ld=gold
if the gcc configure determines the alternate gold linker has split 
stack support.
- A case statement is now used in gcc configure to verify the gold 
version, to make it
easier for other platforms to add their checks if necessary.  I don't 
know if other
platforms require this checking; Matthias' patch did not check the 
version.  For powerpc64
big and little endian we have to check the gold linker version because 
the split
stack support was added recently and older gold linkers won't work.
- The version checking of the gold linker was removed from the libgo 
configure
  since gcc configure has already decided if it is correct.
- TARGET_CAN_SPLIT_STACK_64BIT is now defined in sysv4.h if the glibc 
version
is correct for split stack for powerpc64 big and little endian. This 
define is used in
go/gospec.c in the same way that TARGET_CAN_SPLIT_STACK is used now but,
  additionally verifies that it is a 64 bit compile.  This is necessary 
because split
stack support is not available for ppc 32 bit big endian in gcc or the 
gold linker.

Bootstrapped and tested on x86_64, ppc64le, ppc64 (ran m32, m64 tests)


2015-08-27    Lynn Boger <laboger@linux.vnet.ibm.com>

gcc/
          PR target/66870
          config/rs6000/sysv4.h:  Define TARGET_CAN_SPLIT_STACK_64BIT
          based on LIBC version.
          config.in:  Set up HAVE_GOLD_ALTERNATE.
          configure.ac:  Define HAVE_GOLD_ALTERNATE if the version of 
the gold
          linker supports split stack.
          configure: Regenerate.
          gcc.c:  Add -fuse-ld=gold to STACK_SPLIT_SPEC if 
HAVE_GOLD_ALTERNATE
          is defined.
          go/gospec.c:  (lang_specific_driver):  Use 
TARGET_CAN_SPLIT_STACK_64BIT
          to control setting of fsplit-stack and u,pthread_create 
options for 64 bit
          compiles.


On 08/19/2015 06:05 PM, Lynn A. Boger wrote:
> Also, I don't think it is sufficient to add the option to enable the
> gold linker in gospec.c.  That will only affect links when using gccgo.
> You also want to use the gold linker with gcc if -fsplit-stack is used.
> That is why I had to add it to a spec in linux64.h, so that -fuse-ld=gold
>  is added if fsplit-stack is set for all compilers, not just gccgo.
>
> On 08/19/2015 02:33 PM, Matthias Klose wrote:
>> On 08/18/2015 10:36 PM, Lynn A. Boger wrote:
>>> As discussed in PR 66870, for ppc64le and ppc64le it is preferred to
>>>   use the gold linker with gccgo or gcc if the split stack option is 
>>> enabled.
>>> Use of the gold linker with the split stack option results in less 
>>> storage
>>> allocated for goroutine stacks; if split stack is used without the gold
>>> linker then some testcase failures can occur.
>>>
>>>    Since the use of the gold linker has not been well tested
>>> with all gcc compilers on Power, it is only used as the linker if the
>>> split stack option is used.
>>>
>>> This adds the capability to the configure for gcc and libgo to 
>>> determine
>>> if the gold linker is available at build time, either in the path or 
>>> explicitly
>>>   configured, and its version supports split stack.  If that is the 
>>> case then
>>> defines are set that cause the gold linker to be used by the 
>>> compiler when
>>> -fsplit-stack is used.  This applies to ppc64 and ppc64le. Other 
>>> platforms
>>> with split stack work as before.
>>>
>>> 2015-08-18    Lynn Boger <laboger@linux.vnet.ibm.com>
>>>
>>> gcc/
>>>          PR target/66870
>>>          config/rs6000/linux64.h: When 
>>> HAVE_LD_GOLD_SUPPORTS_SPLIT_STACK
>>>          is defined add -fuse-ld=gold if fsplit-stack and not m32
>>>          config/rs6000/sysv4.h:  Define TARGET_CAN_SPLIT_STACK based on
>>>          LIBC version.
>>>          config.in:  Set up HAVE_LD_GOLD_SUPPORTS_SPLIT_STACK.
>>>          configure.ac:  When gold linker is available and its version
>>>          supports split stack on ppc64, ppc64le, define
>>>          HAVE_LD_GOLD_SUPPORTS_SPLIT_STACK.
>>>          configure:  Regenerate.
>>>
>>> libgo/
>>>          PR target/66870
>>>          configure.ac:  When gccgo for building libgo uses the gold 
>>> version
>>>          containing split stack support on ppc64, ppc64le, define
>>>          LINKER_SUPPORTS_SPLIT_STACK.
>>>          configure:  Regenerate.
>>>
>>> For platforms other than ppc64 and ppc64le, the configure for gcc
>>> and libgo behave as before.
>> why keep the old behaviour for other archs that have split stack 
>> support? Is it
>> really necessary to make this dependent on the target? I'm still 
>> using an
>> unreviewed/unpinged patch to enable gold for gccgo (attached).
>>
>> Matthias
>>
>>
>
>
>

Comments

Ian Lance Taylor Sept. 15, 2015, 4:32 p.m. UTC | #1 
On Thu, Aug 27, 2015 at 2:00 PM, Lynn A. Boger
<laboger@linux.vnet.ibm.com> wrote:
> Here is an updated patch, with a summary of the differences from my previous
> patch:
>
> - In my previous patch gcc configure was verifying the gold linker even if
> it was the
> default linker, but that is not necessary since in that case -fuse-ld=gold
> does not
> need to be set.  Gold version checking is now only done if the alternate
> linker is gold
> and the default linker is not.
> - The STACK_SPLIT_STACK spec define found in gcc/gcc.c now adds
> -fuse-ld=gold
> if the gcc configure determines the alternate gold linker has split stack
> support.
> - A case statement is now used in gcc configure to verify the gold version,
> to make it
> easier for other platforms to add their checks if necessary.  I don't know
> if other
> platforms require this checking; Matthias' patch did not check the version.
> For powerpc64
> big and little endian we have to check the gold linker version because the
> split
> stack support was added recently and older gold linkers won't work.
> - The version checking of the gold linker was removed from the libgo
> configure
>  since gcc configure has already decided if it is correct.
> - TARGET_CAN_SPLIT_STACK_64BIT is now defined in sysv4.h if the glibc
> version
> is correct for split stack for powerpc64 big and little endian. This define
> is used in
> go/gospec.c in the same way that TARGET_CAN_SPLIT_STACK is used now but,
>  additionally verifies that it is a 64 bit compile.  This is necessary
> because split
> stack support is not available for ppc 32 bit big endian in gcc or the gold
> linker.
>
> Bootstrapped and tested on x86_64, ppc64le, ppc64 (ran m32, m64 tests)
>
>
> 2015-08-27    Lynn Boger <laboger@linux.vnet.ibm.com>
>
> gcc/
>          PR target/66870
>          config/rs6000/sysv4.h:  Define TARGET_CAN_SPLIT_STACK_64BIT
>          based on LIBC version.
>          config.in:  Set up HAVE_GOLD_ALTERNATE.
>          configure.ac:  Define HAVE_GOLD_ALTERNATE if the version of the
> gold
>          linker supports split stack.
>          configure: Regenerate.
>          gcc.c:  Add -fuse-ld=gold to STACK_SPLIT_SPEC if
> HAVE_GOLD_ALTERNATE
>          is defined.
>          go/gospec.c:  (lang_specific_driver):  Use
> TARGET_CAN_SPLIT_STACK_64BIT
>          to control setting of fsplit-stack and u,pthread_create options for
> 64 bit
>          compiles.


I'm not sure who is going to approve this patch overall, but I'd
rather the gospec.c changes weren't so #ifdef heavy.  I would set m32
unconditionally.  Then write something like

   supports_split_stack = 0;
#ifdef TARGET_CAN_SPLIT_STACK
  supports_split_stack = 1;
#endif
#ifdef TARGET_CAN_SPLIT_STACK64
  if (m32)
    supports_split_stack = 0;
#endif

Then change the #ifdef TARGET_CAN_SPLIT_STACK code to test supports_split_stack.

Thanks.

Ian
David Edelsohn Sept. 15, 2015, 6:18 p.m. UTC | #2 
On Thu, Aug 27, 2015 at 5:00 PM, Lynn A. Boger
<laboger@linux.vnet.ibm.com> wrote:
> Here is an updated patch, with a summary of the differences from my previous
> patch:
>
> - In my previous patch gcc configure was verifying the gold linker even if
> it was the
> default linker, but that is not necessary since in that case -fuse-ld=gold
> does not
> need to be set.  Gold version checking is now only done if the alternate
> linker is gold
> and the default linker is not.
> - The STACK_SPLIT_STACK spec define found in gcc/gcc.c now adds
> -fuse-ld=gold
> if the gcc configure determines the alternate gold linker has split stack
> support.
> - A case statement is now used in gcc configure to verify the gold version,
> to make it
> easier for other platforms to add their checks if necessary.  I don't know
> if other
> platforms require this checking; Matthias' patch did not check the version.
> For powerpc64
> big and little endian we have to check the gold linker version because the
> split
> stack support was added recently and older gold linkers won't work.
> - The version checking of the gold linker was removed from the libgo
> configure
>  since gcc configure has already decided if it is correct.
> - TARGET_CAN_SPLIT_STACK_64BIT is now defined in sysv4.h if the glibc
> version
> is correct for split stack for powerpc64 big and little endian. This define
> is used in
> go/gospec.c in the same way that TARGET_CAN_SPLIT_STACK is used now but,
>  additionally verifies that it is a 64 bit compile.  This is necessary
> because split
> stack support is not available for ppc 32 bit big endian in gcc or the gold
> linker.
>
> Bootstrapped and tested on x86_64, ppc64le, ppc64 (ran m32, m64 tests)
>
>
> 2015-08-27    Lynn Boger <laboger@linux.vnet.ibm.com>
>
> gcc/
>          PR target/66870
>          config/rs6000/sysv4.h:  Define TARGET_CAN_SPLIT_STACK_64BIT
>          based on LIBC version.
>          config.in:  Set up HAVE_GOLD_ALTERNATE.
>          configure.ac:  Define HAVE_GOLD_ALTERNATE if the version of the
> gold
>          linker supports split stack.
>          configure: Regenerate.
>          gcc.c:  Add -fuse-ld=gold to STACK_SPLIT_SPEC if
> HAVE_GOLD_ALTERNATE
>          is defined.
>          go/gospec.c:  (lang_specific_driver):  Use
> TARGET_CAN_SPLIT_STACK_64BIT
>          to control setting of fsplit-stack and u,pthread_create options for
> 64 bit
>          compiles.

I don't have authority to approve for most of the patch.

I noticed that the test for Gold version number is different than a
similar test in another part of configure.  And the sed pattern seems
to be too restrictive -- it fails to extract a version number for my
tests with Gold in a few different Linux distros.  This should be
addressed instead of silently misidentifying split stack support in
some distros.

- David
Lynn A. Boger Sept. 15, 2015, 6:24 p.m. UTC | #3 
I will make the changes Ian suggested to gospec.c and was
planning to fix the sed string in gcc/configure.ac as David suggested.

I need some feedback on whether to enable the gold linker at
all for split stack on platforms other than Power in gcc/configure.ac.
I don't know if there are gold linker versions that should be verified for
non-Power platforms.  My first patch only enabled it on Power and that
  is what I think should be done.  Those who would like to use gold
with split stack for other platforms can enable it under the appropriate
conditions.

On 09/15/2015 01:18 PM, David Edelsohn wrote:
> On Thu, Aug 27, 2015 at 5:00 PM, Lynn A. Boger
> <laboger@linux.vnet.ibm.com> wrote:
>> Here is an updated patch, with a summary of the differences from my previous
>> patch:
>>
>> - In my previous patch gcc configure was verifying the gold linker even if
>> it was the
>> default linker, but that is not necessary since in that case -fuse-ld=gold
>> does not
>> need to be set.  Gold version checking is now only done if the alternate
>> linker is gold
>> and the default linker is not.
>> - The STACK_SPLIT_STACK spec define found in gcc/gcc.c now adds
>> -fuse-ld=gold
>> if the gcc configure determines the alternate gold linker has split stack
>> support.
>> - A case statement is now used in gcc configure to verify the gold version,
>> to make it
>> easier for other platforms to add their checks if necessary.  I don't know
>> if other
>> platforms require this checking; Matthias' patch did not check the version.
>> For powerpc64
>> big and little endian we have to check the gold linker version because the
>> split
>> stack support was added recently and older gold linkers won't work.
>> - The version checking of the gold linker was removed from the libgo
>> configure
>>   since gcc configure has already decided if it is correct.
>> - TARGET_CAN_SPLIT_STACK_64BIT is now defined in sysv4.h if the glibc
>> version
>> is correct for split stack for powerpc64 big and little endian. This define
>> is used in
>> go/gospec.c in the same way that TARGET_CAN_SPLIT_STACK is used now but,
>>   additionally verifies that it is a 64 bit compile.  This is necessary
>> because split
>> stack support is not available for ppc 32 bit big endian in gcc or the gold
>> linker.
>>
>> Bootstrapped and tested on x86_64, ppc64le, ppc64 (ran m32, m64 tests)
>>
>>
>> 2015-08-27    Lynn Boger <laboger@linux.vnet.ibm.com>
>>
>> gcc/
>>           PR target/66870
>>           config/rs6000/sysv4.h:  Define TARGET_CAN_SPLIT_STACK_64BIT
>>           based on LIBC version.
>>           config.in:  Set up HAVE_GOLD_ALTERNATE.
>>           configure.ac:  Define HAVE_GOLD_ALTERNATE if the version of the
>> gold
>>           linker supports split stack.
>>           configure: Regenerate.
>>           gcc.c:  Add -fuse-ld=gold to STACK_SPLIT_SPEC if
>> HAVE_GOLD_ALTERNATE
>>           is defined.
>>           go/gospec.c:  (lang_specific_driver):  Use
>> TARGET_CAN_SPLIT_STACK_64BIT
>>           to control setting of fsplit-stack and u,pthread_create options for
>> 64 bit
>>           compiles.
> I don't have authority to approve for most of the patch.
>
> I noticed that the test for Gold version number is different than a
> similar test in another part of configure.  And the sed pattern seems
> to be too restrictive -- it fails to extract a version number for my
> tests with Gold in a few different Linux distros.  This should be
> addressed instead of silently misidentifying split stack support in
> some distros.
>
> - David
>
>
Ian Lance Taylor Sept. 15, 2015, 7:58 p.m. UTC | #4 
On Tue, Sep 15, 2015 at 11:24 AM, Lynn A. Boger
<laboger@linux.vnet.ibm.com> wrote:
>
> I need some feedback on whether to enable the gold linker at
> all for split stack on platforms other than Power in gcc/configure.ac.
> I don't know if there are gold linker versions that should be verified for
> non-Power platforms.  My first patch only enabled it on Power and that
>  is what I think should be done.  Those who would like to use gold
> with split stack for other platforms can enable it under the appropriate
> conditions.

I think that is fine for now.  If you want to enable gold for
i386/x86_64 with -fsplit-stack, that is also fine.

Ian
diff mbox

Patch

Index: gcc/config/rs6000/sysv4.h
===================================================================
--- gcc/config/rs6000/sysv4.h	(revision 227266)
+++ gcc/config/rs6000/sysv4.h	(working copy)
@@ -946,6 +946,14 @@  ncrtn.o%s"
 #undef TARGET_ASAN_SHADOW_OFFSET
 #define TARGET_ASAN_SHADOW_OFFSET rs6000_asan_shadow_offset
 
+/* On ppc64 and ppc64le, split stack is only support for
+   64 bit. */
+#undef TARGET_CAN_SPLIT_STACK_64BIT
+#if TARGET_GLIBC_MAJOR > 2 \
+  || (TARGET_GLIBC_MAJOR == 2 && TARGET_GLIBC_MINOR >= 18)
+#define TARGET_CAN_SPLIT_STACK_64BIT
+#endif
+
 /* This target uses the sysv4.opt file.  */
 #define TARGET_USES_SYSV4_OPT 1
 
Index: gcc/config.in
===================================================================
--- gcc/config.in	(revision 227266)
+++ gcc/config.in	(working copy)
@@ -1296,6 +1296,12 @@ 
 #endif
 
 
+/* Define if the gold linker is available as a non-default */
+#ifndef USED_FOR_TARGET
+#undef HAVE_GOLD_NON_DEFAULT
+#endif
+
+
 /* Define if you have the iconv() function. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_ICONV
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 227266)
+++ gcc/configure.ac	(working copy)
@@ -2247,6 +2247,37 @@  if test x$gcc_cv_ld != x; then
 fi
 AC_MSG_RESULT($ld_is_gold)
 
+AC_MSG_CHECKING(gold linker available as non default)
+# Check to see if default ld is not gold, but gold is
+# available.  If gcc was configured with gold then 
+# nothing more is needed.
+# 
+if test x$ld_is_gold = xno && which ${gcc_cv_ld}.gold >/dev/null 2>&1; then
+  case $target in
+# check that the gold version contains the split stack support
+# on powerpc64 big and little endian
+    powerpc64*-*-*)
+      ld_gold=`which ${gcc_cv_ld}.gold`
+      gold_vers=`$ld_gold --version | sed 1q | sed -n -e 's/.*Binutils.* \([[0-9]][[0-9]]*\.[[^)]]*\)).*$/\1/p'`
+      case "$gold_vers" in
+        2.25.[[1-9]]*|2.2[[6-9]][[.0-9]]*|2.[[3-9]][[.0-9]]*|[[3-9]].[[.0-9]]*) gold_non_default=yes
+        ;;
+        *) gold_non_default=no
+        ;;
+      esac
+      ;;
+# Version check needed on other platforms that care about gold with split stack?
+    *)
+      gold_non_default=yes
+      ;;
+  esac
+  if test $gold_non_default = yes; then
+    AC_DEFINE(HAVE_GOLD_NON_DEFAULT, 1,
+    	    [Define if the gold linker is available as a non-default])
+  fi
+fi
+AC_MSG_RESULT($gold_non_default)
+
 ORIGINAL_LD_FOR_TARGET=$gcc_cv_ld
 AC_SUBST(ORIGINAL_LD_FOR_TARGET)
 case "$ORIGINAL_LD_FOR_TARGET" in
Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 227266)
+++ gcc/gcc.c	(working copy)
@@ -666,7 +666,11 @@  proper position among the other output files.  */
    libgcc.  This is not yet a real spec, though it could become one;
    it is currently just stuffed into LINK_SPEC.  FIXME: This wrapping
    only works with GNU ld and gold.  */
+#ifdef HAVE_GOLD_NON_DEFAULT
+#define STACK_SPLIT_SPEC " %{fsplit-stack: -fuse-ld=gold --wrap=pthread_create}"
+#else
 #define STACK_SPLIT_SPEC " %{fsplit-stack: --wrap=pthread_create}"
+#endif
 
 #ifndef LIBASAN_SPEC
 #define STATIC_LIBASAN_LIBS \
Index: gcc/go/gospec.c
===================================================================
--- gcc/go/gospec.c	(revision 227266)
+++ gcc/go/gospec.c	(working copy)
@@ -117,6 +117,11 @@  lang_specific_driver (struct cl_decoded_option **i
   /* Whether the -S option was used.  */
   bool saw_opt_S = false;
 
+#ifdef TARGET_CAN_SPLIT_STACK_64BIT
+  /* Whether the -m32 option was used. */
+  bool saw_opt_m32 = false;
+#endif
+
   /* The first input file with an extension of .go.  */
   const char *first_go_file = NULL;  
 
@@ -152,6 +157,12 @@  lang_specific_driver (struct cl_decoded_option **i
 	    library = (library == 0) ? 1 : library;
 	  break;
 
+#ifdef TARGET_CAN_SPLIT_STACK_64BIT
+	case OPT_m32:
+	  saw_opt_m32 = true;
+	  break;
+#endif
+
 	case OPT_pg:
 	case OPT_p:
 	  saw_profile_flag = true;
@@ -237,8 +248,12 @@  lang_specific_driver (struct cl_decoded_option **i
   new_decoded_options[j++] = decoded_options[i++];
 
   /* If we are linking, pass -fsplit-stack if it is supported.  */
-#ifdef TARGET_CAN_SPLIT_STACK
+#if defined(TARGET_CAN_SPLIT_STACK) || defined(TARGET_CAN_SPLIT_STACK_64BIT)
+#ifdef TARGET_CAN_SPLIT_STACK_64BIT
+  if ((library >= 0) && !saw_opt_m32)
+#else
   if (library >= 0)
+#endif
     {
       generate_option (OPT_fsplit_stack, NULL, 1, CL_DRIVER,
 		       &new_decoded_options[j]);
@@ -381,13 +396,17 @@  lang_specific_driver (struct cl_decoded_option **i
     generate_option (OPT_shared_libgcc, NULL, 1, CL_DRIVER,
 		     &new_decoded_options[j++]);
 
-#ifdef TARGET_CAN_SPLIT_STACK
+#if defined(TARGET_CAN_SPLIT_STACK) || defined(TARGET_CAN_SPLIT_STACK_64BIT)
   /* libgcc wraps pthread_create to support split stack, however, due to
      relative ordering of -lpthread and -lgcc, we can't just mark
      __real_pthread_create in libgcc as non-weak.  But we need to link in
      pthread_create from pthread if we are statically linking, so we work-
      around by passing -u pthread_create to the linker. */
+#ifdef TARGET_CAN_SPLIT_STACK_64BIT
+  if (static_link && !saw_opt_m32)
+#else
   if (static_link)
+#endif
     {
       generate_option (OPT_Wl_, "-u,pthread_create", 1, CL_DRIVER,
 		       &new_decoded_options[j]);