Patch RFC: disable block partitioning with split stack

[Ian Lance Taylor]

On Fri, Jun 9, 2017 at 11:22 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Fri, Jun 9, 2017 at 3:16 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >
>> >> code compiled with -fsplit-stack, if the cold partition calls a
>> >> function that is not compiled with -fsplit-stack (such as a C library
>> >> function).  The problem is that when the linker sees a split-stack
>> >> function call a non-split-stack function, it adjusts the function
>> >> header to request more stack space.  This doesn't work if the call is
>> >> in the cold partition, as the linker doesn't know how to find the
>> >> header to adjust.  You can see this by trying to build the Go library
>> >> using the gold linker with this patch.
>> >
>> > If code does not work, I wonder why we let user to overwrite the default
>> > by hand? In other cases we drop the flag with inform message.
>>
>> My thinking here is that there is no fundamental reason that the code
>> does not work, and the actual problem does not lie in GCC but rather
>> in the linker (specifically, gold).  It's possible in principle to fix
>> gold to make this work, and someone who is using a fixed gold could
>> then direct GCC to take advantage of this optimization (and later
>> after that version of gold is wide-spread enough we can change GCC to
>> drop this patch).
>
> Thanks for explanation.  Perhaps we could have this documented, because
> otherwise people will think the option is simply broken. I guess even better
> we could have configure autodetection for the broken linker.

Committed to mainline with docs as follows.

Ian
-------------- next part --------------
Index: opts.c
===================================================================
--- opts.c	(revision 249070)
+++ opts.c	(working copy)
@@ -863,6 +863,16 @@ finish_options (struct gcc_options *opts
       opts->x_flag_reorder_blocks = 1;
     }
 
+  /* If stack splitting is turned on, and the user did not explicitly
+     request function partitioning, turn off partitioning, as it
+     confuses the linker when trying to handle partitioned split-stack
+     code that calls a non-split-stack functions.  But if partitioning
+     was turned on explicitly just hope for the best.  */
+  if (opts->x_flag_split_stack
+      && opts->x_flag_reorder_blocks_and_partition
+      && !opts_set->x_flag_reorder_blocks_and_partition)
+    opts->x_flag_reorder_blocks_and_partition = 0;
+
   if (opts->x_flag_reorder_blocks_and_partition
       && !opts_set->x_flag_reorder_functions)
     opts->x_flag_reorder_functions = 1;
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 249070)
+++ doc/invoke.texi	(working copy)
@@ -8656,7 +8656,9 @@ paging and cache locality performance.
 This optimization is automatically turned off in the presence of
 exception handling, for linkonce sections, for functions with a user-defined
 section attribute and on any architecture that does not support named
-sections.
+sections.  When @option{-fsplit-stack} is used this option is not
+enabled by default (to avoid linker errors), but may be enabled
+explicitly (if using a working linker).
 
 Enabled for x86 at levels @option{-O2}, @option{-O3}.
 
Index: go/go-lang.c
===================================================================
--- go/go-lang.c	(revision 249070)
+++ go/go-lang.c	(working copy)
@@ -304,6 +304,15 @@ go_langhook_post_options (const char **p
       && targetm_common.supports_split_stack (false, &global_options))
     global_options.x_flag_split_stack = 1;
 
+  /* If stack splitting is turned on, and the user did not explicitly
+     request function partitioning, turn off partitioning, as it
+     confuses the linker when trying to handle partitioned split-stack
+     code that calls a non-split-stack function.  */
+  if (global_options.x_flag_split_stack
+      && global_options.x_flag_reorder_blocks_and_partition
+      && !global_options_set.x_flag_reorder_blocks_and_partition)
+    global_options.x_flag_reorder_blocks_and_partition = 0;
+
   /* Returning false means that the backend should be used.  */
   return false;
 }
Index: testsuite/gcc.dg/tree-prof/split-1.c
===================================================================
--- testsuite/gcc.dg/tree-prof/split-1.c	(revision 0)
+++ testsuite/gcc.dg/tree-prof/split-1.c	(working copy)
@@ -0,0 +1,41 @@
+/* Test case that we don't get a link-time error when using
+   -fsplit-stack with -freorder-blocks-and-partition.  */
+/* { dg-require-effective-target freorder } */
+/* { dg-options "-O2 -fsplit-stack" } */
+
+extern unsigned int sleep (unsigned int);
+
+#define SIZE 10000
+
+const char *sarr[SIZE];
+const char *buf_hot;
+const char *buf_cold;
+
+__attribute__((noinline))
+void 
+foo (int path)
+{
+  int i;
+  if (path)
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_hot;
+    }
+  else
+    {
+      for (i = 0; i < SIZE; i++)
+	sarr[i] = buf_cold;
+      sleep (0);
+    }
+}
+
+int
+main (int argc, char *argv[])
+{
+  int i;
+  buf_hot =  "hello";
+  buf_cold = "world";
+  for (i = 0; i < 1000000; i++)
+    foo (argc);
+  return 0;
+}