Patchwork Do not disable -fomit-frame-pointer on !ACCUMULATE_OUTGOING_ARGS targets

login
register
mail settings
Submitter Eric Botcazou
Date March 25, 2013, 11:26 a.m.
Message ID <1376739.Jx8uRcIPDP@polaris>
Download mbox | patch
Permalink /patch/230639/
State New
Headers show

Comments

Eric Botcazou - March 25, 2013, 11:26 a.m.
Hi,

process_options has had these lines for a couple of releases:

  /* ??? Unwind info is not correct around the CFG unless either a frame
     pointer is present or A_O_A is set.  Fixing this requires rewriting
     unwind info generation to be aware of the CFG and propagating states
     around edges.  */
  if (flag_unwind_tables && !ACCUMULATE_OUTGOING_ARGS
      && flag_omit_frame_pointer)
    {
      warning (0, "unwind tables currently require a frame pointer "
	       "for correctness");
      flag_omit_frame_pointer = 0;
    }

I think that's too broad: for example, it's a common pattern for a target to 
really enable -fomit-frame-pointer only if the stack pointer is unchanging in 
the function; in this case, the unwind info will be correct even if !A_O_A.

So I'm proposing to disable -fomit-frame-pointer on a per-function basis 
instead in ira_setup_eliminable_regset.

Tested on x86_64-suse-linux, OK for the mainline?


2013-03-25  Eric Botcazou  <ebotcazou@adacore.com>

	* toplev.c (process_options): Do not disable -fomit-frame-pointer on a
	general basis if unwind info is requested and ACCUMULATE_OUTGOING_ARGS
	is not enabled.
	* ira.c (ira_setup_eliminable_regset): Instead disable it only on a per
	function basis if the stack pointer is not unchanging in the function.
Richard Henderson - March 25, 2013, 3:31 p.m.
On 03/25/2013 04:26 AM, Eric Botcazou wrote:
> process_options has had these lines for a couple of releases:
> 
>   /* ??? Unwind info is not correct around the CFG unless either a frame
>      pointer is present or A_O_A is set.  Fixing this requires rewriting
>      unwind info generation to be aware of the CFG and propagating states
>      around edges.  */

Heh.  We've actually fixed this now -- unwind info generation aware of
the cfg is exactly what pass_dwarf2_frame does.  So I guess this comment
has been out of date since gcc 4.7.


r~
Eric Botcazou - March 26, 2013, 10:29 p.m.
> Heh.  We've actually fixed this now -- unwind info generation aware of
> the cfg is exactly what pass_dwarf2_frame does.  So I guess this comment
> has been out of date since gcc 4.7.

I see, thanks.  So what do you suggest doing at this point?  Entirely remove 
the block of code?  But AFAICS pass_dwarf2_frame isn't always enabled.  Apply 
the patch anyway with the appropriate additional conditions?
Richard Henderson - March 27, 2013, 3:08 p.m.
On 03/26/2013 03:29 PM, Eric Botcazou wrote:
>> Heh.  We've actually fixed this now -- unwind info generation aware of
>> the cfg is exactly what pass_dwarf2_frame does.  So I guess this comment
>> has been out of date since gcc 4.7.
> 
> I see, thanks.  So what do you suggest doing at this point?  Entirely remove 
> the block of code?  But AFAICS pass_dwarf2_frame isn't always enabled.  Apply 
> the patch anyway with the appropriate additional conditions?
> 

Isn't pass_dwarf2_frame enabled whenever we're generating any unwind info?


r~
Eric Botcazou - March 27, 2013, 5:39 p.m.
> Isn't pass_dwarf2_frame enabled whenever we're generating any unwind info?

Essentially, yes:

static bool
gate_dwarf2_frame (void)
{
#ifndef HAVE_prologue
  /* Targets which still implement the prologue in assembler text
     cannot use the generic dwarf2 unwinding.  */
  return false;
#endif

  /* ??? What to do for UI_TARGET unwinding?  They might be able to benefit
     from the optimized shrink-wrapping annotations that we will compute.
     For now, only produce the CFI notes for dwarf2.  */
  return dwarf2out_do_frame ();
}

Patch

Index: ira.c
===================================================================
--- ira.c	(revision 196816)
+++ ira.c	(working copy)
@@ -1875,6 +1875,15 @@  ira_setup_eliminable_regset (bool from_i
        || crtl->stack_realign_needed
        || targetm.frame_pointer_required ());
 
+  /* ??? Unwind info is not correct around the CFG unless either a frame
+     pointer is present or A_O_A is set or the stack pointer is unchanging.
+     Fixing this requires rewriting unwind info generation to be aware of
+     the CFG and propagating states around edges.  */
+  if (flag_unwind_tables
+      && !ACCUMULATE_OUTGOING_ARGS
+      && !crtl->sp_is_unchanging)
+    frame_pointer_needed = true;
+
   if (from_ira_p && ira_use_lra_p)
     /* It can change FRAME_POINTER_NEEDED.  We call it only from IRA
        because it is expensive.  */
Index: toplev.c
===================================================================
--- toplev.c	(revision 196816)
+++ toplev.c	(working copy)
@@ -1527,18 +1527,6 @@  process_options (void)
   if (!flag_stack_protect)
     warn_stack_protect = 0;
 
-  /* ??? Unwind info is not correct around the CFG unless either a frame
-     pointer is present or A_O_A is set.  Fixing this requires rewriting
-     unwind info generation to be aware of the CFG and propagating states
-     around edges.  */
-  if (flag_unwind_tables && !ACCUMULATE_OUTGOING_ARGS
-      && flag_omit_frame_pointer)
-    {
-      warning (0, "unwind tables currently require a frame pointer "
-	       "for correctness");
-      flag_omit_frame_pointer = 0;
-    }
-
   /* Address Sanitizer needs porting to each target architecture.  */
   if (flag_asan
       && (targetm.asan_shadow_offset == NULL