GCC 3.3 Prelease broken on s390

GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Mark,

unfortunately, the prelease is seriously broken on s390; 
exception handling does not work at all.  See
http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00365.html
Every single exception-related testcase (C++ or Java) fails.

This appears to be caused by rth's recent checkin:
http://gcc.gnu.org/ml/gcc-patches/2003-05/msg00402.html
reverting the unwind-dw2.c change introduced by this patch
appears to fix the problem.

Richard, I don't quite see how this change is supposed to work:
+  tmp_sp = (_Unwind_Ptr) context->cfa;
+  _Unwind_SetGRPtr (&orig_context, __builtin_dwarf_sp_column (), &tmp_sp);

On s390, the CFA does not have the same value as the saved stack pointer,
it is biased by 96/160 bytes relative to the stack.  While I haven't
debugged the failure in detail yet, I'd assume this will screw up
stack unwinding on s390, as the next CFA will be computed incorrectly
relative to this wrong SP value ...

Do you have any suggestions how to fix this?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Tue, May 06, 2003 at 11:47:28PM +0200, Ulrich Weigand wrote:
> On s390, the CFA does not have the same value as the saved stack pointer,
> it is biased by 96/160 bytes relative to the stack.

Eh?  There are assumptions *all over* that code that the
CFA of one frame is the stack pointer of the previous.

If s390 is doing something different... well, I don't
know how it ever worked.

I can work with you to address this, but I'm going to need
a few pointers to speed up the process.  E.g. a picture of
the stack frame between one function and the next.


r~

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Richard Henderson wrote:

> Eh?  There are assumptions *all over* that code that the
> CFA of one frame is the stack pointer of the previous.

Where?  Except for a gdb problem where it incorrectly
assumed that the DWARF-2 CFA and the DWARF-2 frame_base
must be the same (this has been fixed in the meantime),
we've never seen any problems ...

Our choice of CFA has remained unchanged since 2.95.x,
and there's a lot of existing binaries that have their
.eh_frame encoding accordingly, so I do not think this
is something we can change at this point.

> I can work with you to address this, but I'm going to need
> a few pointers to speed up the process.  E.g. a picture of
> the stack frame between one function and the next.

Assuming function f() calls g(), we have this layout:

   |                                                    | <- CFA of f ()
   |  register save area                                |
   |====================================================|
   |  locals of f ()                                    |
   |                                                    |
   |  parameter overflow area (holds parameters         |
   |  of g as called by f)                              |
   |                                                    | <- CFA of g ()
   |  register save area allocated by f ()              |
   |====================================================| <- SP of f ()
   |  locals of g ()                                    |
   |                                                    |
   |  register save area allocated by g ()              |
   ====================================================== <- SP of g ()
 
We are running with a biased stack in that every function
does not use the lowest 96/160 bytes of its stack frame,
those are reserved as register save area for use by called
routines.

When a function call from f() to g() happens, g() is
entered with SP still pointing to the bottom of the
reigster save area.  The first thing g's prolog does
is to save its registers (including the stack pointer)
into that area (which was allocated by f()).  It then
subtracts an offset from the stack pointer, thereby
allocating its stack frame plus the register save area
for use by functions called by itself.

The epilog simply restores registers from the save area,
including the stack pointer -- thereby cleaning up the
stack frame.

At typical FDE looks like this:

0000014c 0000000c 00000000 CIE
  Version:               1
  Augmentation:          ""
  Code alignment factor: 1
  Data alignment factor: -4
  Return address column: 14

  DW_CFA_def_cfa: r15 ofs 96

0000015c 00000020 00000014 FDE cie=0000014c pc=004009a0..004009fe
  DW_CFA_advance_loc: 4 to 004009a4
  DW_CFA_offset: r15 at cfa-36
  DW_CFA_offset: r14 at cfa-40
  DW_CFA_offset: r13 at cfa-44
  DW_CFA_offset: r12 at cfa-48
  DW_CFA_offset: r11 at cfa-52
  DW_CFA_offset: r10 at cfa-56
  DW_CFA_advance_loc: 22 to 004009ba
  DW_CFA_def_cfa_offset: 192
  DW_CFA_nop
  DW_CFA_nop
  DW_CFA_nop

So when we enter the function, the CFA can be found at r15 + 96
(r15 is the stack pointer).  Once the prolog is complete, the
CFA is now addressable at r15 + 192 (because we bought another
96 bytes of stack; this would be an empty frame).

In the register save area at cfa-XX the various registers are
saved and need to be restored from there for stack unwinding.

The function return address is in r14 on function entry.
The unwinder would find it from the place where it is stored
in the register area (at cfa-40) as pointed out by the FDE.

All this has been working just fine ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Wed, May 07, 2003 at 12:28:02AM +0200, Ulrich Weigand wrote:
> Where?  Except for a gdb problem where it incorrectly
> assumed that the DWARF-2 CFA and the DWARF-2 frame_base
> must be the same (this has been fixed in the meantime),
> we've never seen any problems ...

I see, because the existing code that installed a new stack pointer value
did it by differential.  I.e. the difference between one stack frame and
another, which is the same assuming the bias is constant.

Which begs the question of why my patch affects anything, because we're
*still* dealing with a difference.  This I need to understand.  Can you
debug what's going on?

(Though there was a follow-up patch for mainline that *does* assume that
the stack pointer is really really the CFA.  This allows interrupt frames
to be encoded much more efficiently (and in a way that's actually
maintainable by hand) than without the patch.)

> Our choice of CFA has remained unchanged since 2.95.x...

An unfortunate mistake.  :-(

A more "correct" typical frame would be

.LSCIE0:
        .4byte  0xffffffff      # CIE Identifier Tag
        .byte   0x1     # CIE Version
        .ascii "\0"     # CIE Augmentation
        .byte   0x1     # uleb128 0x1; CIE Code Alignment Factor
        .byte   0x7c    # sleb128 -4; CIE Data Alignment Factor
        .byte   0xe     # CIE RA Column
        .byte   0xc     # DW_CFA_def_cfa
        .byte   0xf     # uleb128 0xf
        .byte   0x0     # uleb128 0x0
        .align  4
.LECIE0:
.LSFDE0:
        .4byte  .LEFDE0-.LASFDE0        # FDE Length
.LASFDE0:
        .4byte  .Lframe0        # FDE CIE offset
        .4byte  .LFB3   # FDE initial location
        .4byte  .LFE3-.LFB3     # FDE address range
        .byte   0x4     # DW_CFA_advance_loc4
        .4byte  .LCFI0-.LFB3
        .byte   0x11    # DW_CFA_offset_extended_sf
        .byte   0xf     # uleb128 0xf
        .byte   0x71    # sleb128 -15
        .byte   0x11    # DW_CFA_offset_extended_sf
        .byte   0xe     # uleb128 0xe
        .byte   0x72    # sleb128 -14
        .byte   0x11    # DW_CFA_offset_extended_sf
        .byte   0xd     # uleb128 0xd
        .byte   0x73    # sleb128 -13
        .byte   0x4     # DW_CFA_advance_loc4
        .4byte  .LCFI1-.LCFI0
        .byte   0xe     # DW_CFA_def_cfa_offset
        .byte   0x60    # uleb128 0x60
        .align  4
.LEFDE0:

As seen with

-#define INCOMING_FRAME_SP_OFFSET STACK_POINTER_OFFSET
+#define INCOMING_FRAME_SP_OFFSET       0
+#define ARG_POINTER_CFA_OFFSET(FNDECL) (-STACK_POINTER_OFFSET)

If we really can't declare this a bug, and fix it, then...
I dunno, perhaps some new target macro can work around this.


r~

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Tue, May 06, 2003 at 04:17:01PM -0700, Richard Henderson wrote:
> I dunno, perhaps some new target macro can work around this.

Try this.  It's vs mainline, if that works, we can 
adjust it as necessary for 3.3.



r~



Index: unwind-dw2.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/unwind-dw2.c,v
retrieving revision 1.29
diff -c -p -d -u -r1.29 unwind-dw2.c
--- unwind-dw2.c	6 May 2003 17:28:37 -0000	1.29
+++ unwind-dw2.c	6 May 2003 23:36:25 -0000
@@ -54,6 +54,11 @@
 #define DWARF_REG_TO_UNWIND_COLUMN(REGNO) (REGNO)
 #endif
 
+/* Work around historic mistakes defining the CFA on some targets.  */
+#ifndef RUNTIME_SP_CFA_OFFSET
+#define RUNTIME_SP_CFA_OFFSET 0
+#endif
+
 /* This is the register and unwind state for a particular frame.  This
    provides the information necessary to unwind up past a frame and return
    to its caller.  */
@@ -1096,7 +1101,7 @@ uw_update_context_1 (struct _Unwind_Cont
      the value over from one frame to another doesn't make sense.  */
   if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ()))
     {
-      tmp_sp = (_Unwind_Ptr) context->cfa;
+      tmp_sp = (_Unwind_Ptr) context->cfa + RUNTIME_SP_CFA_OFFSET;
       _Unwind_SetGRPtr (&orig_context, __builtin_dwarf_sp_column (), &tmp_sp);
     }
   _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL);
@@ -1267,6 +1272,7 @@ uw_install_context_1 (struct _Unwind_Con
   if (_Unwind_GetGRPtr (target, __builtin_dwarf_sp_column ()))
     target_cfa = (void *)(_Unwind_Ptr)
       _Unwind_GetGR (target, __builtin_dwarf_sp_column ());
+      + RUNTIME_SP_CFA_OFFSET;
   else
     target_cfa = target->cfa;
 
Index: doc/tm.texi
===================================================================
RCS file: /cvs/gcc/gcc/gcc/doc/tm.texi,v
retrieving revision 1.216
diff -c -p -d -u -r1.216 tm.texi
--- doc/tm.texi	28 Apr 2003 20:02:28 -0000	1.216
+++ doc/tm.texi	6 May 2003 23:36:31 -0000
@@ -2979,6 +2979,20 @@ You only need to define this macro if th
 want to support call frame debugging information like that provided by
 DWARF 2.
 
+@findex RUNTIME_SP_CFA_OFFSET
+@item RUNTIME_SP_CFA_OFFSET
+A C expression that expresses a fixed offset from the stack pointer
+to the CFA of a function.
+
+Normally it is assumed that the CFA of one frame will be the stack
+pointer of the outer frame.  This should be considered canonical,
+and all new targets should do this.
+
+In the case of s390, however, the other CFA macros were set up such
+that the stack pointer is at an offset from the CFA.  But this was
+discovered too late to change, lest all new object files not 
+interoperate will old object files.
+
 @findex SMALL_STACK
 @item SMALL_STACK
 Define this macro if the stack size for the target is very small.  This
Index: config/s390/s390.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/s390/s390.h,v
retrieving revision 1.70
diff -c -p -d -u -r1.70 s390.h
--- config/s390/s390.h	29 Apr 2003 14:16:47 -0000	1.70
+++ config/s390/s390.h	6 May 2003 23:36:31 -0000
@@ -546,6 +546,7 @@ extern int current_function_outgoing_arg
 /* Describe calling conventions for DWARF-2 exception handling.  */
 #define INCOMING_RETURN_ADDR_RTX  gen_rtx_REG (Pmode, RETURN_REGNUM)
 #define INCOMING_FRAME_SP_OFFSET STACK_POINTER_OFFSET
+#define RUNTIME_SP_CFA_OFFSET (-STACK_POINTER_OFFSET)
 #define DWARF_FRAME_RETURN_COLUMN  14
 
 /* Describe how we implement __builtin_eh_return.  */

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Richard Henderson wrote:

> Try this.  It's vs mainline, if that works, we can 
> adjust it as necessary for 3.3.

Interestingly, mainline works fine without any patch.
This is because mainline contains this if:
    if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ()))

and since on s390 there is always a CFA record to restore
the stack pointer from, that new code to fiddle with the
stack pointer is never even entered ...

In any case, shouldn't these two:
> @@ -1096,7 +1101,7 @@ uw_update_context_1 (struct _Unwind_Cont
>       the value over from one frame to another doesn't make sense.  */
>    if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ()))
>      {
> -      tmp_sp = (_Unwind_Ptr) context->cfa;
> +      tmp_sp = (_Unwind_Ptr) context->cfa + RUNTIME_SP_CFA_OFFSET;
>        _Unwind_SetGRPtr (&orig_context, __builtin_dwarf_sp_column (), &tmp_sp);
>      }
>    _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL);
> @@ -1267,6 +1272,7 @@ uw_install_context_1 (struct _Unwind_Con
>    if (_Unwind_GetGRPtr (target, __builtin_dwarf_sp_column ()))
>      target_cfa = (void *)(_Unwind_Ptr)
>        _Unwind_GetGR (target, __builtin_dwarf_sp_column ());
> +      + RUNTIME_SP_CFA_OFFSET;
>    else
>      target_cfa = target->cfa;

have different signs for the offset?  The one adds to cfa in order
to get sp, the other adds to sp to get cfa ...  Which direction
is the macro supposed to represent?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Wed, May 07, 2003 at 02:24:34AM +0200, Ulrich Weigand wrote:
> Interestingly, mainline works fine without any patch.

That doesn't seem right at all.

> This is because mainline contains this if:
>     if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ()))
> 
> and since on s390 there is always a CFA record to restore
> the stack pointer from, that new code to fiddle with the
> stack pointer is never even entered ...

Yes, but then later in uw_install_context we should see the
saved stack pointer and install that instead of the CFA like
you wanted.

(Why in the world do you save the stack frame in each frame?
Just history for descriptionless unwinding?)

> In any case, shouldn't these two:
> > @@ -1096,7 +1101,7 @@ uw_update_context_1 (struct _Unwind_Cont
> >       the value over from one frame to another doesn't make sense.  */
> >    if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ()))
> >      {
> > -      tmp_sp = (_Unwind_Ptr) context->cfa;
> > +      tmp_sp = (_Unwind_Ptr) context->cfa + RUNTIME_SP_CFA_OFFSET;
> >        _Unwind_SetGRPtr (&orig_context, __builtin_dwarf_sp_column (), &tmp_sp);
> >      }
> >    _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL);
> > @@ -1267,6 +1272,7 @@ uw_install_context_1 (struct _Unwind_Con
> >    if (_Unwind_GetGRPtr (target, __builtin_dwarf_sp_column ()))
> >      target_cfa = (void *)(_Unwind_Ptr)
> >        _Unwind_GetGR (target, __builtin_dwarf_sp_column ());
> > +      + RUNTIME_SP_CFA_OFFSET;
> >    else
> >      target_cfa = target->cfa;
> 
> have different signs for the offset?  The one adds to cfa in order
> to get sp, the other adds to sp to get cfa ...  Which direction
> is the macro supposed to represent?

Um... right.  SP->CFA means the second one is right, the first one
is wrong, and the definition itself has the wrong sign.


r~

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Richard Henderson wrote:

> Which begs the question of why my patch affects anything, because we're
> *still* dealing with a difference.  This I need to understand.  Can you
> debug what's going on?

Because the new CFA is computed incorrectly by uw_update_context_1.
It should be reading the saved r15 value from the save area, and
add the offset.  Instead, it now uses the old CFA value, and adds
the offset -- which results in a different value.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

On Tue, 2003-05-06 at 14:47, Ulrich Weigand wrote:
> Mark,
> 
> unfortunately, the prelease is seriously broken on s390; 
> exception handling does not work at all.  See
> http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00365.html
> Every single exception-related testcase (C++ or Java) fails.

This is why we have prereleases. :-)

In my pre-release announcement, I wrote:

  If you find problems in this pre-release, do not send them directly to
  me.  Instead, please file a bug-report here:

    http://gcc.gnu.org/cgi-bin/gnatsweb.pl

  and add my name to the CC list for the bug report.

  If you do send me a report directly, I will simply return it to you
  with a request to put it in GNATS, as above.

Please do that.

Thanks,

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Mark Mitchell wrote:

> Please do that.

Sorry; I did it now: other/10650.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

I wrote:

> Interestingly, mainline works fine without any patch.

When your latest mainline patch to unwind-dw2.c is backported 
to 3.3, it works again as well; the patch below does this.

(A full bootstrap/regtest is still running.)

Note that I really only need the first hunk, as the
rest only changes the return value from uw_install_context_1,
which is completely ignored on s390 anyway (as SP is
restored from its stack slot, where the correct new
value was already stored by uw_install_context_1).

What do you think?

Bye,
Ulrich

ChangeLog:

        * unwind-dw2.c (uw_update_context_1): Only set cfa as sp if
        previous frame didn't save sp.  Clear sp for next frame.
        (uw_install_context_1): Honor saved sp from frame.


*** unwind-dw2.c.orig	Wed May  7 02:52:49 2003
--- unwind-dw2.c	Wed May  7 03:00:45 2003
*************** uw_update_context_1 (struct _Unwind_Cont
*** 1059,1069 ****
       In very special situations (such as unwind info for signal return),
       there may be location expressions that use the stack pointer as well.
  
!      Given that other unwind mechanisms generally won't work if you try
!      to represent stack pointer saves and restores directly, we don't
!      bother conditionalizing this at all.  */
!   tmp_sp = (_Unwind_Ptr) context->cfa;
!   orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
  
    /* Compute this frame's CFA.  */
    switch (fs->cfa_how)
--- 1059,1075 ----
       In very special situations (such as unwind info for signal return),
       there may be location expressions that use the stack pointer as well.
  
!      Do this conditionally for one frame.  This allows the unwind info
!      for one frame to save a copy of the stack pointer from the previous
!      frame, and be able to use much easier CFA mechanisms to do it.
!      Always zap the saved stack pointer value for the next frame; carrying
!      the value over from one frame to another doesn't make sense.  */
!   if (!orig_context.reg[__builtin_dwarf_sp_column ()])
!     {
!       tmp_sp = (_Unwind_Ptr) context->cfa;
!       orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
!     }
!   context->reg[__builtin_dwarf_sp_column ()] = NULL;
  
    /* Compute this frame's CFA.  */
    switch (fs->cfa_how)
*************** uw_install_context_1 (struct _Unwind_Con
*** 1201,1206 ****
--- 1207,1213 ----
  		      struct _Unwind_Context *target)
  {
    long i;
+   void *target_cfa;
  
  #if __GTHREADS
    {
*************** uw_install_context_1 (struct _Unwind_Con
*** 1222,1232 ****
  	memcpy (c, t, dwarf_reg_size_table[i]);
      }
  
    /* We adjust SP by the difference between CURRENT and TARGET's CFA.  */
    if (STACK_GROWS_DOWNWARD)
!     return target->cfa - current->cfa + target->args_size;
    else
!     return current->cfa - target->cfa - target->args_size;
  }
  
  static inline _Unwind_Ptr
--- 1229,1246 ----
  	memcpy (c, t, dwarf_reg_size_table[i]);
      }
  
+   /* If the last frame records a saved stack pointer, use it.  */
+   if (target->reg[__builtin_dwarf_sp_column ()])
+     target_cfa = (void *)(_Unwind_Ptr)
+       _Unwind_GetGR (target, __builtin_dwarf_sp_column ());
+   else
+     target_cfa = target->cfa;
+ 
    /* We adjust SP by the difference between CURRENT and TARGET's CFA.  */
    if (STACK_GROWS_DOWNWARD)
!     return target_cfa - current->cfa + target->args_size;
    else
!     return current->cfa - target_cfa - target->args_size;
  }
  
  static inline _Unwind_Ptr

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Wed, May 07, 2003 at 03:13:39AM +0200, Ulrich Weigand wrote:
> ... which is completely ignored on s390 anyway (as SP is
> restored from its stack slot, where the correct new
> value was already stored by uw_install_context_1).

Ah, now I get it.  This was the piece I couldn't understand.
And if *this* is the case, it means that it doesn't matter
where you place the CFA relative to the stack frame, just so
long as the stack pointer itself is computed properly.

> What do you think?
> 
>         * unwind-dw2.c (uw_update_context_1): Only set cfa as sp if
>         previous frame didn't save sp.  Clear sp for next frame.
>         (uw_install_context_1): Honor saved sp from frame.

If this works, great.


r~

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

> > What do you think?
> > 
> >         * unwind-dw2.c (uw_update_context_1): Only set cfa as sp if
> >         previous frame didn't save sp.  Clear sp for next frame.
> >         (uw_install_context_1): Honor saved sp from frame.
> 
> If this works, great.

Ulrich --

Take that as approval from Richard; if it fixes your problem, please
check it in and close the PR.

Thanks,

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Richard Henderson wrote:

> Ah, now I get it.  This was the piece I couldn't understand.
> And if *this* is the case, it means that it doesn't matter
> where you place the CFA relative to the stack frame, just so
> long as the stack pointer itself is computed properly.

This is what I was thinking, yes.

> >         * unwind-dw2.c (uw_update_context_1): Only set cfa as sp if
> >         previous frame didn't save sp.  Clear sp for next frame.
> >         (uw_install_context_1): Honor saved sp from frame.
> 
> If this works, great.

Unfortunately, while it fixes most of the problems, it is not a
complete solution:
http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00418.html
http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00419.html

The Java test case PR218 is still failing.  This is because
this test case throws an exception from a signal handler
through a *leaf* function that was interrupted by the
signal.

When unwinding through the leaf function, there is no CFA code
to restore the stack pointer, because the *correct* behaviour
would have been to reuse the stack pointer from the frame below
(the signal return stub frame) unchanged -- the leaf function
really did not allocate any stack space.

However, this now triggers again the new code that sets the
stack pointer to the cfa, incorrect on s390.

Would you suggest to check the above patch in anyway to make 3.3
more similar to head (apparently the patch is already in 3.2 ...),
and because it fixes at least C++?

Or should we go back to a RUNTIME_SP_CFA_OFFSET solution
on top of the current 3.3 implementation?

What about an alternate target macro that says that this target
handles SP by itself via FDE records, and doesn't need any
other SP fiddling?  This would then allow us to remove the 
unnecessary EH_RETURN_STACKADJ_RTX handling completely.
Maybe this behaviour could actually be triggered by the target
leaving EH_RETURN_STACKADJ_RTX undefined?

If no other solution works for 3.3, we could always fix it
ad-hoc with #ifdef __s390__ ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

> Unfortunately, while it fixes most of the problems, it is not a
> complete solution:

Bummer.

> Would you suggest to check the above patch in anyway to make 3.3
> more similar to head (apparently the patch is already in 3.2 ...),
> and because it fixes at least C++?

No, that doesn't seem worth it.

> Or should we go back to a RUNTIME_SP_CFA_OFFSET solution
> on top of the current 3.3 implementation?

That seems fine, if it will work.  The key criteria are:

(1) You can do it in such a way that it's obvious it doesn't affect
other platforms.

(2) You can do it quickly.

Engineering beauty is not a consideration at all, in this case.  

#ifdef __s390__ is fine, for example; if you can just do 

  #ifdef __s390__ 
  cfa += 196;
  #endif

that is absolutely fine by me.

If we can't meet those criteria, we may be forced to punt; s390 is not a
primary evaluation platform.  

Please see if you can get this fixed in the next twenty-four hours or
so.

Thanks,

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC

Re: GCC 3.3 Prelease broken on s390

[Joe Buck]

On Wed, May 07, 2003 at 08:53:40AM -0700, Mark Mitchell wrote:
> Engineering beauty is not a consideration at all, in this case.  
> 
> #ifdef __s390__ is fine, for example; if you can just do 
> 
>   #ifdef __s390__ 
>   cfa += 196;
>   #endif
> 
> that is absolutely fine by me.

If we go for such a solution, I think that it should be a one-release-only
emergency fix, to be replaced ASAP with a better one; seas of #ifdef's
tend to grow if not whacked back.

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

On Wed, 2003-05-07 at 09:01, Joe Buck wrote:
> On Wed, May 07, 2003 at 08:53:40AM -0700, Mark Mitchell wrote:
> > Engineering beauty is not a consideration at all, in this case.  
> > 
> > #ifdef __s390__ is fine, for example; if you can just do 
> > 
> >   #ifdef __s390__ 
> >   cfa += 196;
> >   #endif
> > 
> > that is absolutely fine by me.
> 
> If we go for such a solution, I think that it should be a one-release-only
> emergency fix, to be replaced ASAP with a better one; seas of #ifdef's
> tend to grow if not whacked back.

There's no way something like this will go on the mainline.

If we had to live with this through the 3.3 series, that would be OK;
that is bounded.

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Mark Mitchell wrote:

> That seems fine, if it will work.  The key criteria are:
> 
> (1) You can do it in such a way that it's obvious it doesn't affect
> other platforms.
> 
> (2) You can do it quickly.
> 
> Engineering beauty is not a consideration at all, in this case.  

I can offer the following patch, which passes bootstrap/regtest
on s390-ibm-linux and s390x-ibm-linux and restores both targets
to their previous behaviour in all cases, including that Java
leaf-function test case.  See:
http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00428.html
http://gcc.gnu.org/ml/gcc-testresults/2003-05/msg00429.html


It should be obvious that it does not affect other platforms;
the hunk in uw_update_context_1 is under #ifdef, and the hunk
in uw_init_context_1 does not affect non-s390 platforms because
for them uw_update_context_1 will immediately undo that change
(never mind that even if it didn't, the change would in fact
be correct for other platforms as well).

I will continue to work on a cleaner solution, but I cannot
guarantee that this will succeed within 24 hours (and I'd
also like to ask for Richard's opinion).  

Do you think we should use the patch below for 3.3?

Bye,
Ulrich


	PR other/10650
	* unwind-dw2.c (uw_update_context_1): Don't set sp as cfa on s390.
	(uw_init_context_1): Set initial sp to outer cfa.

Index: gcc/unwind-dw2.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/unwind-dw2.c,v
retrieving revision 1.22.2.4
diff -c -p -r1.22.2.4 unwind-dw2.c
*** gcc/unwind-dw2.c	5 May 2003 16:59:21 -0000	1.22.2.4
--- gcc/unwind-dw2.c	7 May 2003 14:54:10 -0000
*************** uw_update_context_1 (struct _Unwind_Cont
*** 1062,1069 ****
--- 1062,1071 ----
       Given that other unwind mechanisms generally won't work if you try
       to represent stack pointer saves and restores directly, we don't
       bother conditionalizing this at all.  */
+ #ifndef __s390__
    tmp_sp = (_Unwind_Ptr) context->cfa;
    orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
+ #endif
  
    /* Compute this frame's CFA.  */
    switch (fs->cfa_how)
*************** uw_init_context_1 (struct _Unwind_Contex
*** 1164,1169 ****
--- 1166,1172 ----
  
    /* Force the frame state to use the known cfa value.  */
    context->cfa = outer_cfa;
+   context->reg[__builtin_dwarf_sp_column ()] = &outer_cfa;
    fs.cfa_how = CFA_REG_OFFSET;
    fs.cfa_reg = __builtin_dwarf_sp_column ();
    fs.cfa_offset = 0;


-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Joe Buck]

On Wed, May 07, 2003 at 07:02:49PM +0200, Ulrich Weigand wrote:
> It should be obvious that it does not affect other platforms;
> the hunk in uw_update_context_1 is under #ifdef, and the hunk
> in uw_init_context_1 does not affect non-s390 platforms because
> for them uw_update_context_1 will immediately undo that change
> (never mind that even if it didn't, the change would in fact
> be correct for other platforms as well).

Given the very short time remaining for testing, if you're ifdef'ing
one hunk, you might as well ifdef them both.  The patch would then
have zero risk for other platforms.
 

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

> I will continue to work on a cleaner solution, but I cannot
> guarantee that this will succeed within 24 hours (and I'd
> also like to ask for Richard's opinion).  
> 
> Do you think we should use the patch below for 3.3?

Yes -- but please enclose the second hunk in #ifdef as well.  If we're
going to be ugly, let's not try to be smart while we're being ugly.

Please check this in, and close the PR.

Thanks,

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Mark Mitchell wrote:

> Yes -- but please enclose the second hunk in #ifdef as well.  If we're
> going to be ugly, let's not try to be smart while we're being ugly.

Fine with me.

> Please check this in, and close the PR.

For the record, I've checked in the patch below and closed the PR.

Bye,
Ulrich


Index: gcc/ChangeLog
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ChangeLog,v
retrieving revision 1.16114.2.511
diff -c -p -r1.16114.2.511 ChangeLog
*** gcc/ChangeLog	7 May 2003 06:02:01 -0000	1.16114.2.511
--- gcc/ChangeLog	7 May 2003 19:26:21 -0000
***************
*** 1,3 ****
--- 1,9 ----
+ 2003-05-06  Ulrich Weigand  <uweigand@de.ibm.com>
+ 
+ 	PR other/10650
+ 	* unwind-dw2.c (uw_update_context_1): Don't set sp as cfa on s390.
+ 	(uw_init_context_1): Set initial sp to outer cfa on s390.
+ 
  2003-05-06  Mark Mitchell  <mark@codesourcery.com>
  
  	PR other/10658
Index: gcc/unwind-dw2.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/unwind-dw2.c,v
retrieving revision 1.22.2.4
diff -c -p -r1.22.2.4 unwind-dw2.c
*** gcc/unwind-dw2.c	5 May 2003 16:59:21 -0000	1.22.2.4
--- gcc/unwind-dw2.c	7 May 2003 19:26:21 -0000
*************** uw_update_context_1 (struct _Unwind_Cont
*** 1062,1069 ****
--- 1062,1071 ----
       Given that other unwind mechanisms generally won't work if you try
       to represent stack pointer saves and restores directly, we don't
       bother conditionalizing this at all.  */
+ #ifndef __s390__
    tmp_sp = (_Unwind_Ptr) context->cfa;
    orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
+ #endif
  
    /* Compute this frame's CFA.  */
    switch (fs->cfa_how)
*************** uw_init_context_1 (struct _Unwind_Contex
*** 1164,1169 ****
--- 1166,1174 ----
  
    /* Force the frame state to use the known cfa value.  */
    context->cfa = outer_cfa;
+ #ifdef __s390__
+   context->reg[__builtin_dwarf_sp_column ()] = &outer_cfa;
+ #endif
    fs.cfa_how = CFA_REG_OFFSET;
    fs.cfa_reg = __builtin_dwarf_sp_column ();
    fs.cfa_offset = 0;


-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

On Wed, 2003-05-07 at 12:39, Ulrich Weigand wrote:
> Mark Mitchell wrote:
> 
> > Yes -- but please enclose the second hunk in #ifdef as well.  If we're
> > going to be ugly, let's not try to be smart while we're being ugly.
> 
> Fine with me.
> 
> > Please check this in, and close the PR.
> 
> For the record, I've checked in the patch below and closed the PR.

Thanks!

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com

Re: GCC 3.3 Prelease broken on s390

[Jonathan Lennox]

Ulrich Weigand writes:
> + #ifndef __s390__
>     tmp_sp = (_Unwind_Ptr) context->cfa;
>     orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
> + #endif

Doesn't __s390__ indicate the build system, not the target?

-- 
Jonathan Lennox
lennox@cs.columbia.edu

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

On Wed, 2003-05-07 at 11:19, Jonathan Lennox wrote:
> Ulrich Weigand writes:
> > + #ifndef __s390__
> >     tmp_sp = (_Unwind_Ptr) context->cfa;
> >     orig_context.reg[__builtin_dwarf_sp_column ()] = &tmp_sp;
> > + #endif
> 
> Doesn't __s390__ indicate the build system, not the target?

Yes, but this code is built with the new GCC, so I think that will work
our correctly.

-- 
Mark Mitchell <mark@codesourcery.com>
CodeSourcery, LLC

Re: GCC 3.3 Prelease broken on s390

[Jonathan Lennox]

On , May 7 2003, "Mark Mitchell" wrote to "Jonathan Lennox, Ulrich Weigand, rth@redhat.com, gcc@gcc.gnu.org" saying:

> On Wed, 2003-05-07 at 11:19, Jonathan Lennox wrote:
> > Doesn't __s390__ indicate the build system, not the target?
> 
> Yes, but this code is built with the new GCC, so I think that will work
> our correctly.

What about cross-compilers to s390, or built on it?

("They don't work" is a fine answer.) 

-- 
Jonathan Lennox
lennox@cs.columbia.edu

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

On Wed, 2003-05-07 at 11:30, Jonathan Lennox wrote:
> On , May 7 2003, "Mark Mitchell" wrote to "Jonathan Lennox, Ulrich Weigand, rth@redhat.com, gcc@gcc.gnu.org" saying:
> 
> > On Wed, 2003-05-07 at 11:19, Jonathan Lennox wrote:
> > > Doesn't __s390__ indicate the build system, not the target?
> > 
> > Yes, but this code is built with the new GCC, so I think that will work
> > our correctly.
> 
> What about cross-compilers to s390, or built on it?

They will set __s390__ if they are cross compilers to __s390__, and will
not set it if they are not, so all will be well.

And, in fact, even were this broken it would not be too bad -- it's not
like s390 is one of our most-used platforms.  The goal here is to get a
minimally intrusive patch that makes it possible for s390 people to use
GCC 3.3.

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com

Re: GCC 3.3 Prelease broken on s390

[Daniel Jacobowitz]

On Wed, May 07, 2003 at 02:30:23PM -0400, Jonathan Lennox wrote:
> On , May 7 2003, "Mark Mitchell" wrote to "Jonathan Lennox, Ulrich Weigand, rth@redhat.com, gcc@gcc.gnu.org" saying:
> 
> > On Wed, 2003-05-07 at 11:19, Jonathan Lennox wrote:
> > > Doesn't __s390__ indicate the build system, not the target?
> > 
> > Yes, but this code is built with the new GCC, so I think that will work
> > our correctly.
> 
> What about cross-compilers to s390, or built on it?
> 
> ("They don't work" is a fine answer.) 

This code is built for the target, not for the build machine.  Checking
__s390__ should be correct.  (It's part of libgcc)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: GCC 3.3 Prelease broken on s390

[Richard Henderson]

On Wed, May 07, 2003 at 04:49:12PM +0200, Ulrich Weigand wrote:
> However, this now triggers again the new code that sets the
> stack pointer to the cfa, incorrect on s390.

If I understand correctly, then this would be fixed by

  + #ifndef __s390__
      _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL);
  + #endif

correct?

> Would you suggest to check the above patch in anyway to make 3.3
> more similar to head (apparently the patch is already in 3.2 ...),
> and because it fixes at least C++?

Well, not to make it more similar to head, but because it gets
more cases demonstrably correct.  On s390 and elsewhere.


r~

Re: GCC 3.3 Prelease broken on s390

[Ulrich Weigand]

Richard Henderson wrote:

> If I understand correctly, then this would be fixed by
> 
>   + #ifndef __s390__
>       _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL);
>   + #endif
> 
> correct?

Basically yes, except that then the initial cfa is no longer
found when called from uw_init_context_1.  This is fixed by the 
second hunk of the patch I just committed ...

> > Would you suggest to check the above patch in anyway to make 3.3
> > more similar to head (apparently the patch is already in 3.2 ...),
> > and because it fixes at least C++?
> 
> Well, not to make it more similar to head, but because it gets
> more cases demonstrably correct.  On s390 and elsewhere.

We should certainly fix it properly on the head and for 3.3.1.
Whether this is something for 3.3 I have no strong opinion on
-- that's Mark's call ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: GCC 3.3 Prelease broken on s390

[Mark Mitchell]

> We should certainly fix it properly on the head and for 3.3.1.
> Whether this is something for 3.3 I have no strong opinion on
> -- that's Mark's call ...

For 3.3, we certainly don't want anything we don't absolutely need.

Thanks,

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com