[PATCH] Fix computed gotos on m68k

[PATCH] Fix computed gotos on m68k

[Julian Brown]

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

Hi,

This patch fixes computed gotos on m68k, and probably other targets too
(the fix is in the middle end). Several tests fail at present with "-O3
-fomit-frame-pointer".

One example of erroneous behaviour is as follows: the
function 'x' in comp-goto-1.c compiles to:

x:
        lea (-104,%sp),%sp
        lea (104,%sp),%a0
        move.l %a0,92(%sp)
        fmovem #63,44(%sp)
        movem.l #31996,(%sp)
        move.l %sp,96(%sp)
        move.l 108(%sp),-(%sp)
        lea (96,%sp),%a0
        jsr y.1217
.L3:
.L12:  
        move.l 112(%sp),%d0   <--- wrong offset, should be 108.
        movem.l (%sp),#31996
        fmovem 44(%sp),#63
        lea (104,%sp),%sp
        rts

Where the source code looks like:

  int
  x(a)
  {
    __label__ xlab;
    void y(a)
      {
        void *x = &&llab;
        if (a==-1)
	  goto *x;
        if (a==0)
	  goto xlab;
      llab:
        y (a-1);
      }
    y (a);
   xlab:;
    return a;
  }

The offset is intended to load the argument value 'a' for the "return
a;" statement, but actually loads one word beyond that.

This has been broken because since mainline revision r160124, which
(correctly) causes 'y' to be flagged as noreturn -- y never returns via
the normal function return route. If ipa-pure-const.c is hacked to
check that a function does not perform non-local gotos before marking
it noreturn, the original (correct) offset is restored -- but that is
simply papering over the problem.

Prior to r160124, or with the aforementioned patch to inhibit the
"noreturn" attribute for the function y, the call to y from x (just
before "xlab:") has a goto to the following label emitted at expand
time. Before/after that patch is applied, the diff of 148r.expand
around the relevant part looks like:

   # BLOCK 2 freq:10000
   # PRED: ENTRY [100.0%]  (fallthru,exec)
   y (a_1(D)); [static-chain: &FRAME.0]
-  # SUCC: 3 [100.0%]  (ab,exec)
+  goto <bb 4> (xlab);
+  # SUCC: 3 [61.0%]  (ab,exec) 4 [39.0%]  (fallthru,exec)
 
-  # BLOCK 3 freq:10000
-  # PRED: 2 [100.0%]  (ab,exec)
+  # BLOCK 3 freq:6100
+  # PRED: 2 [61.0%]  (ab,exec)
 <L0>: [non-local]
   # SUCC: 4 [100.0%]  (fallthru,exec)
 
   # BLOCK 4 freq:10000
-  # PRED: 3 [100.0%]  (fallthru,exec)
+  # PRED: 2 [39.0%]  (fallthru,exec) 3 [100.0%]  (fallthru,exec)
 xlab:

Now, this is important because when the "goto <bb 4>" is emitted, a
stack adjustment is also emitted to pop y's arguments (via
dojump.c:do_pending_stack_adjust). Without that (i.e. the present state
with no patches), when later passes try to figure out elimination
offsets (from arg pointer to frame pointer, frame pointer to stack
pointer and so on) at the labels L0/xlab, they get the wrong answer.
(This happens in reload, called from ira.)

So, on to the attached fix. As mentioned previously, GCC tracks
elimination offsets throughout each function, which can vary as e.g.
other functions are called. There is a concept of an "initial" offset:
I believe that when nonlocal gotos are executed, and hence at all the
labels a nonlocal goto may reach, the elimination offsets must have
their "initial" values.

We can find out if a label is the target of a nonlocal goto (and hence
force the use of initial offsets) in a somewhat roundabout way, by
looking up its containing basic block, seeing if that BB is a nonlocal
goto target, then seeing if the label is the first instruction in the
block. This seems slightly clumsy, but I didn't find another way of
doing it.

Interestingly perhaps, the comment near the fix in
reload1.c:set_label_offsets hints at a possible issue which may be
related:

    case CODE_LABEL:
      /* If we know nothing about this label, set the desired offsets.
    Note that this sets the offset at a label to be the offset before a
    label if we don't know anything about the label.  This is not
    correct for the label after a BARRIER, but is the best guess we can
    make.  If we guessed wrong, we will suppress an elimination that
    might have been possible had we been able to guess correctly.  */

But obviously, in our case, rather than just failing to make an
elimination which may have been possible, we get wrong code instead
(there is a barrier before our non-local-goto-receiving label).

Anyway. The attached patch appears to work fine, and we get the
following changes in test results (cross-building and testing to
ColdFire Linux):

FAIL -> PASS: gcc.c-torture/execute/920501-7.c execution,  -O3 -fomit-frame-pointer 
FAIL -> PASS: gcc.c-torture/execute/comp-goto-2.c execution,  -O3 -fomit-frame-pointer 
FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c  -O3 -fomit-frame-pointer  execution test
FAIL -> PASS: gcc.dg/torture/stackalign/comp-goto-1.c  -O3 -fomit-frame-pointer  execution test [#2]
FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c  -O3 -fomit-frame-pointer  execution test
FAIL -> PASS: gcc.dg/torture/stackalign/non-local-goto-4.c  -O3 -fomit-frame-pointer  execution test [#2]

Any comments, or OK to apply?

Thanks,

Julian

ChangeLog

    gcc/
    * reload1.c (set_label_offsets): Force initial_p for labels
    receiving non-local gotos.

[-- Attachment #2: nonlocal-goto-target-initial-offset-6.diff --]
[-- Type: text/x-patch, Size: 793 bytes --]

Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	(revision 179967)
+++ gcc/reload1.c	(working copy)
@@ -2371,6 +2371,19 @@ set_label_offsets (rtx x, rtx insn, int 
 
       if (! offsets_known_at[CODE_LABEL_NUMBER (x) - first_label_num])
 	{
+	  if (x == insn)
+	    {
+	      basic_block bb;
+
+	      bb = BLOCK_FOR_INSN (insn);
+
+	      /* If the label is the target of a non-local GOTO, we must use
+	         the initial elimination offsets.  */
+	      if (bb && BB_HEAD (bb) == insn
+		  && (bb->flags & BB_NON_LOCAL_GOTO_TARGET))
+		initial_p = true;
+	    }
+	  
 	  for (i = 0; i < NUM_ELIMINABLE_REGS; i++)
 	    offsets_at[CODE_LABEL_NUMBER (x) - first_label_num][i]
 	      = (initial_p ? reg_eliminate[i].initial_offset

Re: [PATCH] Fix computed gotos on m68k

[Mikael Pettersson]

Julian Brown writes:
 > Hi,
 > 
 > This patch fixes computed gotos on m68k, and probably other targets too
 > (the fix is in the middle end). Several tests fail at present with "-O3
 > -fomit-frame-pointer".

This is the same bug as PR47918.  As described in that PR trail, the bug
can be triggered also on i386.

Thanks for debugging this very nasty issue!

> [...]

/Mikael

Re: [PATCH] Fix computed gotos on m68k

[Eric Botcazou]

> This has been broken because since mainline revision r160124, which
> (correctly) causes 'y' to be flagged as noreturn -- y never returns via
> the normal function return route. If ipa-pure-const.c is hacked to
> check that a function does not perform non-local gotos before marking
> it noreturn, the original (correct) offset is restored -- but that is
> simply papering over the problem.

Agreed.

> So, on to the attached fix. As mentioned previously, GCC tracks
> elimination offsets throughout each function, which can vary as e.g.
> other functions are called. There is a concept of an "initial" offset:
> I believe that when nonlocal gotos are executed, and hence at all the
> labels a nonlocal goto may reach, the elimination offsets must have
> their "initial" values.

Agreed.

> We can find out if a label is the target of a nonlocal goto (and hence
> force the use of initial offsets) in a somewhat roundabout way, by
> looking up its containing basic block, seeing if that BB is a nonlocal
> goto target, then seeing if the label is the first instruction in the
> block. This seems slightly clumsy, but I didn't find another way of
> doing it.

These labels are on the nonlocal_goto_handler_labels chain.  You presumably 
just need to apply the same treatment to them in set_initial_label_offsets as 
the one applied to forced labels.

> Any comments, or OK to apply?

OK for the adjusted patch if it works, mainline and 4.6 branch once it reopens.
Please mention PR rtl-optimization/47918 in the ChangeLog.

Thanks for fixing this.

-- 
Eric Botcazou

Re: [PATCH] Fix computed gotos on m68k

[Julian Brown]

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

On Tue, 25 Oct 2011 14:49:09 +0200
Eric Botcazou <ebotcazou@adacore.com> wrote:

> These labels are on the nonlocal_goto_handler_labels chain.  You
> presumably just need to apply the same treatment to them in
> set_initial_label_offsets as the one applied to forced labels.

> OK for the adjusted patch if it works, mainline and 4.6 branch once
> it reopens. Please mention PR rtl-optimization/47918 in the ChangeLog.

Thanks -- I've committed this (much cleaner) version to mainline,
after re-testing. I'll see about testing it on 4.6 also.

Cheers,

Julian

[-- Attachment #2: nonlocal-goto-target-initial-offset-9.diff --]
[-- Type: text/x-patch, Size: 970 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 180610)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,10 @@
+2011-10-28  Julian Brown  <julian@codesourcery.com>
+
+	PR rtl-optimization/47918
+
+	* reload1.c (set_initial_label_offsets): Use initial offsets
+	for labels on the nonlocal_goto_handler_labels chain.
+
 2011-10-28  Iain Sandoe  <iains@gcc.gnu.org>
 
 	* config/rs6000/t-darwin (LIB2FUNCS_STATIC_EXTRA): 
Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	(revision 180610)
+++ gcc/reload1.c	(working copy)
@@ -3918,6 +3918,10 @@ set_initial_label_offsets (void)
     if (XEXP (x, 0))
       set_label_offsets (XEXP (x, 0), NULL_RTX, 1);
 
+  for (x = nonlocal_goto_handler_labels; x; x = XEXP (x, 1))
+    if (XEXP (x, 0))
+      set_label_offsets (XEXP (x, 0), NULL_RTX, 1);
+
   for_each_eh_label (set_initial_eh_label_offset);
 }
 

Re: [PATCH] Fix computed gotos on m68k

[Julian Brown]

On Fri, 28 Oct 2011 11:53:41 +0100
Julian Brown <julian@codesourcery.com> wrote:

> On Tue, 25 Oct 2011 14:49:09 +0200
> Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
> > These labels are on the nonlocal_goto_handler_labels chain.  You
> > presumably just need to apply the same treatment to them in
> > set_initial_label_offsets as the one applied to forced labels.
> 
> > OK for the adjusted patch if it works, mainline and 4.6 branch once
> > it reopens. Please mention PR rtl-optimization/47918 in the
> > ChangeLog.
> 
> Thanks -- I've committed this (much cleaner) version to mainline,
> after re-testing. I'll see about testing it on 4.6 also.

I've now committed the patch on 4.6 also. I did need to apply the
following patch from Bernd in order to test the 4.6 branch tip
successfully, since without it my build blew up in glibc with an error
in final.c:

  http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00454.html

Maybe that patch should be applied to 4.6 also?

Thanks,

Julian

Re: [PATCH] Fix computed gotos on m68k

[Eric Botcazou]

> I've now committed the patch on 4.6 also. I did need to apply the
> following patch from Bernd in order to test the 4.6 branch tip
> successfully, since without it my build blew up in glibc with an error
> in final.c:
>
>   http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00454.html
>
> Maybe that patch should be applied to 4.6 also?

Fine with me, this is a regression; please backport the testcase as well.  TIA.

-- 
Eric Botcazou