[patch] Fix PR target/67265

[patch] Fix PR target/67265

[Eric Botcazou]

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

Hi,

this is an ICE on an asm statement requiring a lot of registers, when compiled 
in 32-bit mode on x86/Linux with -O -fstack-check -fPIC:

pr67265.c:10:3: error: 'asm' operand has impossible constraints

The issue is that, since stack checking defines STACK_CHECK_MOVING_SP on this 
platform, the frame pointer is necessary in order to be able to propagate 
exceptions raised on stack overflow.  But this is required only in Ada so we 
can certainly avoid doing it in C or C++.

Tested on x86_64-suse-linux, OK for all active branches ? (that's a regression 
wrt the old stack checking implementation)


2015-11-11  Eric Botcazou  <ebotcazou@adacore.com>

	PR target/67265
	* ira.c (ira_setup_eliminable_regset): Do not necessarily create the
	frame pointer for stack checking if non-call exceptions aren't used.


2015-11-11  Eric Botcazou  <ebotcazou@adacore.com>
	
	* gcc.target/i386/pr67265.c: New test.

-- 
Eric Botcazou

[-- Attachment #2: pr67265.diff --]
[-- Type: text/x-patch, Size: 719 bytes --]

Index: ira.c
===================================================================
--- ira.c	(revision 230146)
+++ ira.c	(working copy)
@@ -2261,7 +2261,10 @@ ira_setup_eliminable_regset (void)
        || (cfun->calls_alloca && EXIT_IGNORE_STACK)
        /* We need the frame pointer to catch stack overflow exceptions
 	  if the stack pointer is moving.  */
-       || (flag_stack_check && STACK_CHECK_MOVING_SP)
+       || (STACK_CHECK_MOVING_SP
+	   && flag_stack_check
+	   && flag_exceptions
+	   && cfun->can_throw_non_call_exceptions)
        || crtl->accesses_prior_frames
        || (SUPPORTS_STACK_ALIGNMENT && crtl->stack_realign_needed)
        /* We need a frame pointer for all Cilk Plus functions that use

[-- Attachment #3: pr67265.c --]
[-- Type: text/x-csrc, Size: 276 bytes --]

/* PR target/67265 */
/* Reduced testcase by Johannes Dewender <gnu@JonnyJD.net> */

/* { dg-do compile } */
/* { dg-options "-O -fstack-check -fPIC" } */

int a, b, c, d, e;

void foo (void)
{
  __asm__("" : "+r"(c), "+r"(e), "+r"(d), "+r"(a) : ""(b), "mg"(foo), "mm"(c));
}

Re: [patch] Fix PR target/67265

[Bernd Schmidt]

On 11/11/2015 12:38 PM, Eric Botcazou wrote:
> this is an ICE on an asm statement requiring a lot of registers, when compiled
> in 32-bit mode on x86/Linux with -O -fstack-check -fPIC:
>
> pr67265.c:10:3: error: 'asm' operand has impossible constraints
>
> The issue is that, since stack checking defines STACK_CHECK_MOVING_SP on this
> platform, the frame pointer is necessary in order to be able to propagate
> exceptions raised on stack overflow.  But this is required only in Ada so we
> can certainly avoid doing it in C or C++.

>         /* We need the frame pointer to catch stack overflow exceptions
>  	  if the stack pointer is moving.  */
> -       || (flag_stack_check && STACK_CHECK_MOVING_SP)
> +       || (STACK_CHECK_MOVING_SP
> +	   && flag_stack_check
> +	   && flag_exceptions
> +	   && cfun->can_throw_non_call_exceptions)

This piece of code along doesn't tell me exactly why the frame pointer 
is needed. I was looking for an explicit use, but I now guess that if 
you have multiple adjusts of the frame pointer you can't easily undo 
them in the error case (the function behaves as-if using alloca). Is 
that it? And without exceptions I assume you just get a call to abort so 
it doesn't matter? If I understood all that right, then this is ok.

In i386.c I see a code block with a similar condition,

   /* If the only reason for frame_pointer_needed is that we conservatively
      assumed stack realignment might be needed, but in the end nothing that
      needed the stack alignment had been spilled, clear 
frame_pointer_needed
      and say we don't need stack realignment.  */

and the condition has

       && !(flag_stack_check && STACK_CHECK_MOVING_SP)

Should that be changed too?


Bernd

Re: [patch] Fix PR target/67265

[Eric Botcazou]

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

> This piece of code along doesn't tell me exactly why the frame pointer
> is needed. I was looking for an explicit use, but I now guess that if
> you have multiple adjusts of the [stack] pointer you can't easily undo
> them in the error case (the function behaves as-if using alloca). Is
> that it?

Yes, exactly, the analogy with alloca is correct.

> And without exceptions I assume you just get a call to abort so
> it doesn't matter? If I understood all that right, then this is ok.

If you don't care about exceptions on stack overflow, then the signal will 
very likely terminate the program instead of overwriting stack contents, which 
is good enough.  In Ada, the language requires you to exit gracefully or even 
to resume regular execution (at least in theory for the latter).

> In i386.c I see a code block with a similar condition,
> 
>    /* If the only reason for frame_pointer_needed is that we conservatively
>       assumed stack realignment might be needed, but in the end nothing that
> needed the stack alignment had been spilled, clear
> frame_pointer_needed
>       and say we don't need stack realignment.  */
> 
> and the condition has
> 
>        && !(flag_stack_check && STACK_CHECK_MOVING_SP)
> 
> Should that be changed too?

Yes, it probably should, thanks for spotting it, revised patch attached.


        PR target/67265
        * ira.c (ira_setup_eliminable_regset): Do not necessarily create the
        frame pointer for stack checking if non-call exceptions aren't used.
	* config/i386/i386.c (ix86_finalize_stack_realign_flags): Likewise.


-- 
Eric Botcazou

[-- Attachment #2: pr67265-2.diff --]
[-- Type: text/x-patch, Size: 1627 bytes --]

Index: ira.c
===================================================================
--- ira.c	(revision 230146)
+++ ira.c	(working copy)
@@ -2259,9 +2259,12 @@ ira_setup_eliminable_regset (void)
   frame_pointer_needed
     = (! flag_omit_frame_pointer
        || (cfun->calls_alloca && EXIT_IGNORE_STACK)
-       /* We need the frame pointer to catch stack overflow exceptions
-	  if the stack pointer is moving.  */
-       || (flag_stack_check && STACK_CHECK_MOVING_SP)
+       /* We need the frame pointer to catch stack overflow exceptions if
+	  the stack pointer is moving (as for the alloca case just above).  */
+       || (STACK_CHECK_MOVING_SP
+	   && flag_stack_check
+	   && flag_exceptions
+	   && cfun->can_throw_non_call_exceptions)
        || crtl->accesses_prior_frames
        || (SUPPORTS_STACK_ALIGNMENT && crtl->stack_realign_needed)
        /* We need a frame pointer for all Cilk Plus functions that use
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 230146)
+++ config/i386/i386.c	(working copy)
@@ -12470,7 +12466,11 @@ ix86_finalize_stack_realign_flags (void)
       && !crtl->accesses_prior_frames
       && !cfun->calls_alloca
       && !crtl->calls_eh_return
-      && !(flag_stack_check && STACK_CHECK_MOVING_SP)
+      /* See ira_setup_eliminable_regset for the rationale.  */
+      && !(STACK_CHECK_MOVING_SP
+	   && flag_stack_check
+	   && flag_exceptions
+	   && cfun->can_throw_non_call_exceptions)
       && !ix86_frame_pointer_required ()
       && get_frame_size () == 0
       && ix86_nsaved_sseregs () == 0

Re: [patch] Fix PR target/67265

[Bernd Schmidt]

On 11/11/2015 01:31 PM, Eric Botcazou wrote:
> Yes, it probably should, thanks for spotting it, revised patch attached.
>
>
>          PR target/67265
>          * ira.c (ira_setup_eliminable_regset): Do not necessarily create the
>          frame pointer for stack checking if non-call exceptions aren't used.
> 	* config/i386/i386.c (ix86_finalize_stack_realign_flags): Likewise.

Ok if it passes testing. Should have thought of it earlier, but if you 
want to, you can also make a fp_needed_for_stack_checking_p function 
containing the four tests.


Bernd

Re: [patch] Fix PR target/67265

[Eric Botcazou]

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

> Ok if it passes testing.

Thanks, it did so I installed the fix yesterday but further testing then 
revealed an oversight: the following assertion in ix86_adjust_stack_and_probe

  gcc_assert (cfun->machine->fs.cfa_reg != stack_pointer_rtx);

will now evidently trigger (simple testcase attached).

I can sched some light on it here since I wrote the code: the initial version 
of ix86_adjust_stack_and_probe didn't bother generating CFI because it only 
manipulates the stack pointer and the CFA register was guaranteed to be the 
frame pointer until yesterday, so I put the assertion to check this guarantee.
Then Richard H. enhanced the CFI machinery to always track stack adjustments 
(IIRC this was a prerequisite for your implementation of shrink-wrapping) so I 
added code to generate CFI:

  /* Even if the stack pointer isn't the CFA register, we need to correctly
     describe the adjustments made to it, in particular differentiate the
     frame-related ones from the frame-unrelated ones.  */
  if (size > 0)

To sum up, I think that the assertion is obsolete and can be removed without 
further ado; once done, the compiler generates correct CFI for the testcase.
So I installed the following one-liner as obvious after testing on x86-64.


2015-11-12  Eric Botcazou  <ebotcazou@adacore.com>

        PR target/67265
	* config/i386/i386.c (ix86_adjust_stack_and_probe): Remove obsolete
	assertion on the CFA register.


2015-11-12  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.target/i386/pr67265-2.c: New test.

-- 
Eric Botcazou

[-- Attachment #2: pr67265-2b.diff --]
[-- Type: text/x-patch, Size: 573 bytes --]

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 230204)
+++ config/i386/i386.c	(working copy)
@@ -12245,8 +12245,6 @@ ix86_adjust_stack_and_probe (const HOST_
       release_scratch_register_on_entry (&sr);
     }
 
-  gcc_assert (cfun->machine->fs.cfa_reg != stack_pointer_rtx);
-
   /* Even if the stack pointer isn't the CFA register, we need to correctly
      describe the adjustments made to it, in particular differentiate the
      frame-related ones from the frame-unrelated ones.  */

[-- Attachment #3: pr67265-2.c --]
[-- Type: text/x-csrc, Size: 133 bytes --]

/* { dg-do compile } */
/* { dg-options "-O -fstack-check" } */

void foo (int n)
{
  volatile char arr[64 * 1024];

  arr[n] = 1;
}