Re: [patch] Fix PR target/67265
> 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
PR target/67265
* config/i386/i386.c (ix86_adjust_stack_and_probe): Remove obsolete
assertion on the CFA register.
2015-11-12 Eric Botcazou
* gcc.target/i386/pr67265-2.c: New test.
--
Eric BotcazouIndex: 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. */
/* { dg-do compile } */
/* { dg-options "-O -fstack-check" } */
void foo (int n)
{
volatile char arr[64 * 1024];
arr[n] = 1;
}
Re: [patch] Fix PR target/67265
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
> 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 BotcazouIndex: 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
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
[patch] Fix PR target/67265
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
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
* gcc.target/i386/pr67265.c: New test.
--
Eric BotcazouIndex: 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
/* PR target/67265 */
/* Reduced testcase by Johannes Dewender */
/* { 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));
}
