Patch RFC: disable block partitioning with split stack

Patch RFC: disable block partitioning with split stack

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

This recent patch
https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00518.html turned on
block partitioning even when profiling is not enabled.  This can break
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.

This patch fixes the problem by disabling an implicit
-freorder-blocks-and-partition when -fsplit-stack is turned on.  If
the user explicitly requested -freorder-blocks-and-partition we leave
it alone, assuming that they know what they are doing.  But if it was
only turned it on implicitly because of the optimization level we turn
it off.

This is done in both the main option handling code and in the Go
frontend option handling, because the Go frontend implicitly turns on
-fsplit-stack itself.

The test case in the patch fails to build when using the gold linker
without the patch.

Bootstrapped and ran split-stack test cases on x86_64-pc-linux-gnu.

I plan to commit this tomorrow unless I hear objections.

Ian


2017-06-08  Ian Lance Taylor  <iant@golang.org>

* opts.c (finish_options): If -fsplit-stack, disable implicit
-forder-blocks-and-partition.

2017-06-08  Ian Lance Taylor  <iant@golang.org>

* go-lang.c (go_langhook_post_options): If -fsplit-stack is turned
on, disable implicit -forder-blocks-and-partition.

2017-06-08  Ian Lance Taylor  <iant@golang.org>

* gcc.dg/tree-prof/split-1.c: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2833 bytes --]

Index: opts.c
===================================================================
--- opts.c	(revision 249028)
+++ 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: go/go-lang.c
===================================================================
--- go/go-lang.c	(revision 249028)
+++ 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;
+}

Re: Patch RFC: disable block partitioning with split stack

[Richard Biener]

On Fri, Jun 9, 2017 at 7:14 AM, Ian Lance Taylor <iant@golang.org> wrote:
> This recent patch
> https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00518.html turned on
> block partitioning even when profiling is not enabled.  This can break
> 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.
>
> This patch fixes the problem by disabling an implicit
> -freorder-blocks-and-partition when -fsplit-stack is turned on.  If
> the user explicitly requested -freorder-blocks-and-partition we leave
> it alone, assuming that they know what they are doing.  But if it was
> only turned it on implicitly because of the optimization level we turn
> it off.
>
> This is done in both the main option handling code and in the Go
> frontend option handling, because the Go frontend implicitly turns on
> -fsplit-stack itself.
> The test case in the patch fails to build when using the gold linker
> without the patch.
>
> Bootstrapped and ran split-stack test cases on x86_64-pc-linux-gnu.
>
> I plan to commit this tomorrow unless I hear objections.

Ok.

> Ian
>
>
> 2017-06-08  Ian Lance Taylor  <iant@golang.org>
>
> * opts.c (finish_options): If -fsplit-stack, disable implicit
> -forder-blocks-and-partition.
>
> 2017-06-08  Ian Lance Taylor  <iant@golang.org>
>
> * go-lang.c (go_langhook_post_options): If -fsplit-stack is turned
> on, disable implicit -forder-blocks-and-partition.
>
> 2017-06-08  Ian Lance Taylor  <iant@golang.org>
>
> * gcc.dg/tree-prof/split-1.c: New test.

Re: Patch RFC: disable block partitioning with split stack

[Jan Hubicka]

> This recent patch
> https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00518.html turned on
> block partitioning even when profiling is not enabled.  This can break

I am heading towards enabling block partitioning by default (because we now
have infrastructure to identify likely cold BBs), but I must admit I did
not notice I made it happen by that patch.  reorder_blocks_and_partition
is not enabled in opts.c but I see it is hidden within
common/config/i386/i386-common.c that I did not know even existed.

> 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.

Also bb-reorder knows how to prevent landing pads to go to different sections,
so perhaps same machinery can be used to prevent splitting blocks having
calls that needs linker adjustments?

But if fixing this is involved, I am fine with how it is done in your patch.

Honza

Re: Patch RFC: disable block partitioning with split stack

[Ian Lance Taylor]

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).


> Also bb-reorder knows how to prevent landing pads to go to different sections,
> so perhaps same machinery can be used to prevent splitting blocks having
> calls that needs linker adjustments?

Unfortunately I don't see how that is possible in general, as the code
that needs adjustment is cases where code compiled with -fsplit-stack
calls functions compiled without -fsplit-stack.  By definition those
calls are to functions defined in other compilation units, and the
compiler simply doesn't know whether they will be compiled with
-fsplit-stack or not.  Only the linker knows.

Ian

Re: Patch RFC: disable block partitioning with split stack

[Jan Hubicka]

> 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.
> 
> 
> > Also bb-reorder knows how to prevent landing pads to go to different sections,
> > so perhaps same machinery can be used to prevent splitting blocks having
> > calls that needs linker adjustments?
> 
> Unfortunately I don't see how that is possible in general, as the code
> that needs adjustment is cases where code compiled with -fsplit-stack
> calls functions compiled without -fsplit-stack.  By definition those
> calls are to functions defined in other compilation units, and the
> compiler simply doesn't know whether they will be compiled with
> -fsplit-stack or not.  Only the linker knows.

I see.  We could stil block offlining all blocks that contains calls to
functions that does not bind to current defs, but I guess that would prevent
most of useful code plitting anyway.

Thank you!
Honza
> 
> Ian

Re: Patch RFC: disable block partitioning with split stack

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

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

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3516 bytes --]

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;
+}

Re: Patch RFC: disable block partitioning with split stack

[Jan Hubicka]

> >
> > 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.

Thanks, the patch however disables rerodering unconditionally because 
flag_split_stack is set to -1 at that time.  I have comitted the following

2017-06-10  Jan Hubicka  <hubicka@ucw.cz>

        * opts.c (finish_options): Move test for flag_split_stack after
        it has been initialized.


--- trunk/gcc/opts.c	2017/06/11 05:29:34	249104
+++ trunk/gcc/opts.c	2017/06/11 09:33:22	249105
@@ -863,19 +863,6 @@
       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;
 
   /* Pipelining of outer loops is only possible when general pipelining
      capabilities are requested.  */
@@ -927,6 +914,20 @@
 	}
     }
 
+  /* 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;
+
   /* Tune vectorization related parametees according to cost model.  */
   if (opts->x_flag_vect_cost_model == VECT_COST_MODEL_CHEAP)
     {

[PATCH] Fix new split-1.c testcase

[Segher Boessenkool]

The new split-1.c testcase fails on targets that do not support split
stack (like 32-bit PowerPC Linux).  This patch fixes it by only running
the testcase if split stack is supported.  It also adds the reorder
flag to the options, so that the test actually tests what it says it
tests.

Is this okay for trunk?


Segher


2017-06-11  Segher Boessenkool  <segher@kernel.crashing.org>

gcc/testsuite/
	* gcc.dg/tree-prof/split-1.c: Require effective target split_stack.
	Add -freorder-blocks-and-partition to options.

---
 gcc/testsuite/gcc.dg/tree-prof/split-1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/tree-prof/split-1.c b/gcc/testsuite/gcc.dg/tree-prof/split-1.c
index a42fccf..4de1123 100644
--- a/gcc/testsuite/gcc.dg/tree-prof/split-1.c
+++ b/gcc/testsuite/gcc.dg/tree-prof/split-1.c
@@ -1,7 +1,8 @@
 /* Test case that we don't get a link-time error when using
    -fsplit-stack with -freorder-blocks-and-partition.  */
+/* { dg-require-effective-target split_stack } */
 /* { dg-require-effective-target freorder } */
-/* { dg-options "-O2 -fsplit-stack" } */
+/* { dg-options "-O2 -fsplit-stack -freorder-blocks-and-partition" } */
 
 extern unsigned int sleep (unsigned int);
 
-- 
1.9.3

Re: [PATCH] Fix new split-1.c testcase

[Ian Lance Taylor]

On Sun, Jun 11, 2017 at 4:40 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> The new split-1.c testcase fails on targets that do not support split
> stack (like 32-bit PowerPC Linux).  This patch fixes it by only running
> the testcase if split stack is supported.  It also adds the reorder
> flag to the options, so that the test actually tests what it says it
> tests.
>
> Is this okay for trunk?

Whoops, sorry about that.

Adding dg-require-effective-target split_stack is fine.  Adding an
explicit -freorder-blocks-and-partition option is not.  Adding the
explicit option will cause the test to fail when using gold, as the
two options are not compatible.  The point of the test is to test that
using -fsplit-stack disables the default enabling of
-freorder-blocks-and-partition.

Thanks.

Ian

> 2017-06-11  Segher Boessenkool  <segher@kernel.crashing.org>
>
> gcc/testsuite/
>         * gcc.dg/tree-prof/split-1.c: Require effective target split_stack.
>         Add -freorder-blocks-and-partition to options.
>
> ---
>  gcc/testsuite/gcc.dg/tree-prof/split-1.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.dg/tree-prof/split-1.c b/gcc/testsuite/gcc.dg/tree-prof/split-1.c
> index a42fccf..4de1123 100644
> --- a/gcc/testsuite/gcc.dg/tree-prof/split-1.c
> +++ b/gcc/testsuite/gcc.dg/tree-prof/split-1.c
> @@ -1,7 +1,8 @@
>  /* Test case that we don't get a link-time error when using
>     -fsplit-stack with -freorder-blocks-and-partition.  */
> +/* { dg-require-effective-target split_stack } */
>  /* { dg-require-effective-target freorder } */
> -/* { dg-options "-O2 -fsplit-stack" } */
> +/* { dg-options "-O2 -fsplit-stack -freorder-blocks-and-partition" } */
>
>  extern unsigned int sleep (unsigned int);
>
> --
> 1.9.3
>

Re: Patch RFC: disable block partitioning with split stack

[Ian Lance Taylor]

On Sun, Jun 11, 2017 at 2:38 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >
>> > 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.
>
> Thanks, the patch however disables rerodering unconditionally because
> flag_split_stack is set to -1 at that time.  I have comitted the following
>
> 2017-06-10  Jan Hubicka  <hubicka@ucw.cz>
>
>         * opts.c (finish_options): Move test for flag_split_stack after
>         it has been initialized.

Argh, sorry about that.  Thanks for fixing it.

Ian

Re: [PATCH] Fix new split-1.c testcase

[Segher Boessenkool]

On Sun, Jun 11, 2017 at 07:38:04PM -0700, Ian Lance Taylor wrote:
> On Sun, Jun 11, 2017 at 4:40 AM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > The new split-1.c testcase fails on targets that do not support split
> > stack (like 32-bit PowerPC Linux).  This patch fixes it by only running
> > the testcase if split stack is supported.  It also adds the reorder
> > flag to the options, so that the test actually tests what it says it
> > tests.
> >
> > Is this okay for trunk?
> 
> Whoops, sorry about that.
> 
> Adding dg-require-effective-target split_stack is fine.  Adding an
> explicit -freorder-blocks-and-partition option is not.  Adding the
> explicit option will cause the test to fail when using gold, as the
> two options are not compatible.  The point of the test is to test that
> using -fsplit-stack disables the default enabling of
> -freorder-blocks-and-partition.

Ah, I see.  Could you change the comment then, to say what we are
really testing?

> >  /* 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 } */

And this line isn't required, in that case -- removing it is less
confusing and allows the test to run in more places ;-)

[ Paul Hua sent a patch adding split_stack already, it was OKed, but
it is not committed yet, fwiw ].


Segher

Re: [PATCH] Fix new split-1.c testcase

[Paul Hua]

>
> [ Paul Hua sent a patch adding split_stack already, it was OKed, but
> it is not committed yet, fwiw ].
>

I saw this, so not commit my patch.

Paul.

Re: [PATCH] Fix new split-1.c testcase

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Mon, Jun 12, 2017 at 3:38 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Sun, Jun 11, 2017 at 07:38:04PM -0700, Ian Lance Taylor wrote:
>> On Sun, Jun 11, 2017 at 4:40 AM, Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>> >
>> > The new split-1.c testcase fails on targets that do not support split
>> > stack (like 32-bit PowerPC Linux).  This patch fixes it by only running
>> > the testcase if split stack is supported.  It also adds the reorder
>> > flag to the options, so that the test actually tests what it says it
>> > tests.
>> >
>> > Is this okay for trunk?
>>
>> Whoops, sorry about that.
>>
>> Adding dg-require-effective-target split_stack is fine.  Adding an
>> explicit -freorder-blocks-and-partition option is not.  Adding the
>> explicit option will cause the test to fail when using gold, as the
>> two options are not compatible.  The point of the test is to test that
>> using -fsplit-stack disables the default enabling of
>> -freorder-blocks-and-partition.
>
> Ah, I see.  Could you change the comment then, to say what we are
> really testing?

Sure.  Updated as follows.  Committed to mainline.

Ian

2017-06-12  Ian Lance Taylor  <iant@golang.org>

* gcc.dg/tree-prof/split-1.c: Require split_stack, don't require
freorder.  Update comment to explain test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 657 bytes --]

Index: gcc.dg/tree-prof/split-1.c
===================================================================
--- gcc.dg/tree-prof/split-1.c	(revision 249128)
+++ gcc.dg/tree-prof/split-1.c	(working copy)
@@ -1,6 +1,6 @@
-/* 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 } */
+/* Test that we don't get a link-time error when using -fsplit-stack
+   due to implicit enabling of -freorder-blocks-and-partition.  */
+/* { dg-require-effective-target split_stack } */
 /* { dg-options "-O2 -fsplit-stack" } */
 
 extern unsigned int sleep (unsigned int);

Re: [PATCH] Fix new split-1.c testcase

[Segher Boessenkool]

On Mon, Jun 12, 2017 at 09:16:32AM -0700, Ian Lance Taylor wrote:
> On Mon, Jun 12, 2017 at 3:38 AM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > Ah, I see.  Could you change the comment then, to say what we are
> > really testing?
> 
> Sure.  Updated as follows.  Committed to mainline.

Thanks!


Segher