[patch/rfa:rs6000] Don't use ->prev

[patch/rfa:rs6000] Don't use ->prev

[Andrew Cagney]

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

Hello,

A number of targets contained code that tested ``->prev'' as part of 
doing their frame analysis.  I think this ``cheating''.  The objective 
of the frame analysis code is to create the ->prev frame using 
information from the current frame, not to use the still being created 
->prev frame :-)

Two cases - frame_chain for z8k and s390 - were, I think, simply wrong. 
  The deleted z8k comment is telling - someone was 180 degrees out! :-)

The other two cases - get_saved_regs for SPARC and rs6000 - are more 
interesting.

The SPARC code clearly relies on there already being a ->prev frame so 
I've used get_prev_frame().  Since ->prev must exist, there is no risk 
of recursion - get_prev_frame() calling get_saved_regs() (yes, grotty).

The rs6000 has me puzzled. I think the ->frame contains the address of 
the wrong end of the frame!  If ->frame pointed at the frame's start, 
the code below wouldn't even be needed.  Anyway, I've changed it to use 
frame_chain() (I don't see regressions on NetBSD/PPC.  The other 
possability would be to risk a (recursive) get_prev_frame() call.

Anyway, is the rs6000 ok?

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 4154 bytes --]

2002-04-14  Andrew Cagney  <ac131313@redhat.com>

	* sparc-tdep.c (sparc_get_saved_register): Use get_prev_frame
	instead of ->prev.
	* z8k-tdep.c (z8k_frame_chain): Do not use ->prev.
	* s390-tdep.c (s390_frame_chain): Do not use ->prev.
	* rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
	instead of ->prev.

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.55
diff -u -r1.55 rs6000-tdep.c
--- rs6000-tdep.c	12 Apr 2002 19:48:36 -0000	1.55
+++ rs6000-tdep.c	14 Apr 2002 16:48:56 -0000
@@ -1388,10 +1388,12 @@
       && fdatap->cr_offset == 0
       && fdatap->vr_offset == 0)
     frame_addr = 0;
-  else if (fi->prev && fi->prev->frame)
-    frame_addr = fi->prev->frame;
   else
-    frame_addr = read_memory_addr (fi->frame, wordsize);
+    /* NOTE: cagney/2002-04-14: The ->frame points to the inner-most
+       address of the current frame.  Things might be easier if the
+       ->frame pointed to the outer-most address of the frame.  In the
+       mean time, the address of the prev frame is used.  */
+    frame_addr = rs6000_frame_chain (fi);
 
   /* if != -1, fdatap->saved_fpr is the smallest number of saved_fpr.
      All fpr's from saved_fpr to fp31 are saved.  */
Index: s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.43
diff -u -r1.43 s390-tdep.c
--- s390-tdep.c	6 Apr 2002 00:02:50 -0000	1.43
+++ s390-tdep.c	14 Apr 2002 16:49:11 -0000
@@ -1009,9 +1009,7 @@
 {
   CORE_ADDR prev_fp = 0;
 
-  if (thisframe->prev && thisframe->prev->frame)
-    prev_fp = thisframe->prev->frame;
-  else if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
+  if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
     return generic_read_register_dummy (thisframe->pc, thisframe->frame,
                                         S390_SP_REGNUM);
   else
Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.26
diff -u -r1.26 sparc-tdep.c
--- sparc-tdep.c	12 Apr 2002 18:18:57 -0000	1.26
+++ sparc-tdep.c	14 Apr 2002 16:49:23 -0000
@@ -829,11 +829,11 @@
 	    addr = frame1->frame + (regnum - G0_REGNUM) * SPARC_INTREG_SIZE
 	      - (FP_REGISTER_BYTES + 8 * SPARC_INTREG_SIZE);
 	  else if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
-	    addr = (frame1->prev->extra_info->bottom
+	    addr = (get_prev_frame (frame1)->extra_info->bottom
 		    + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
 		    + FRAME_SAVED_I0);
 	  else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
-	    addr = (frame1->prev->extra_info->bottom
+	    addr = (get_prev_frame (frame1)->extra_info->bottom
 		    + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
 		    + FRAME_SAVED_L0);
 	  else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
@@ -875,11 +875,11 @@
 	{
 	  /* Normal frame.  Local and In registers are saved on stack.  */
 	  if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
-	    addr = (frame1->prev->extra_info->bottom
+	    addr = (get_prev_frame (frame1)->extra_info->bottom
 		    + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
 		    + FRAME_SAVED_I0);
 	  else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
-	    addr = (frame1->prev->extra_info->bottom
+	    addr = (get_prev_frame (frame1)->extra_info->bottom
 		    + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
 		    + FRAME_SAVED_L0);
 	  else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
Index: z8k-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/z8k-tdep.c,v
retrieving revision 1.7
diff -u -r1.7 z8k-tdep.c
--- z8k-tdep.c	12 Apr 2002 18:18:57 -0000	1.7
+++ z8k-tdep.c	14 Apr 2002 16:49:25 -0000
@@ -160,10 +160,6 @@
 CORE_ADDR
 z8k_frame_chain (struct frame_info *thisframe)
 {
-  if (thisframe->prev == 0)
-    {
-      /* This is the top of the stack, let's get the sp for real */
-    }
   if (!inside_entry_file (thisframe->pc))
     {
       return read_memory_pointer (thisframe->frame);

Re: [patch/rfa:rs6000] Don't use ->prev

[Kevin Buettner]

On Apr 14, 12:52pm, Andrew Cagney wrote:

> The rs6000 has me puzzled. I think the ->frame contains the address of 
> the wrong end of the frame!  If ->frame pointed at the frame's start, 
> the code below wouldn't even be needed.  Anyway, I've changed it to use 
> frame_chain() (I don't see regressions on NetBSD/PPC.  The other 
> possability would be to risk a (recursive) get_prev_frame() call.
> 
> Anyway, is the rs6000 ok?
> 
[...]
> 	* rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
> 	instead of ->prev.

I think that your patch is okay so long as you use FRAME_CHAIN()
instead of rs6000_frame_chain().  Note that FRAME_CHAIN() may be set
to something other than rs6000_frame_chain() depending upon the ABI / OS.

At the moment though, it doesn't really matter too much since the
case -- signal handler trampolines -- that calling FRAME_CHAIN gets
right is broken in frame_get_saved_regs() anyway.  The Linux/PPC
target works around this deficiency by interposing some code which
computes the correct saved register locations for signal handler
callers instead of calling the rs6000_* version which invokes
rs6000_get_saved_register().

Kevin

Re: [patch/rfa:rs6000] Don't use ->prev

[Andrew Cagney]

>  * rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
>> instead of ->prev.
> 
> 
> I think that your patch is okay so long as you use FRAME_CHAIN()
> instead of rs6000_frame_chain().  Note that FRAME_CHAIN() may be set
> to something other than rs6000_frame_chain() depending upon the ABI / OS.

Ah, good catch, I've made the adjustment.  Thanks.

> At the moment though, it doesn't really matter too much since the
> case -- signal handler trampolines -- that calling FRAME_CHAIN gets
> right is broken in frame_get_saved_regs() anyway.  The Linux/PPC
> target works around this deficiency by interposing some code which
> computes the correct saved register locations for signal handler
> callers instead of calling the rs6000_* version which invokes
> rs6000_get_saved_register().


:-)

> 2002-04-14  Andrew Cagney  <ac131313@redhat.com>
> 
> * sparc-tdep.c (sparc_get_saved_register): Use get_prev_frame
> 	instead of ->prev.
> 	* z8k-tdep.c (z8k_frame_chain): Do not use ->prev.
> 	* s390-tdep.c (s390_frame_chain): Do not use ->prev.
> 	* rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
> 	instead of ->prev.
> 

I've checked this in.

Andrew

Re: [patch/rfa:rs6000] Don't use ->prev

[Michael Snyder]

Andrew Cagney wrote:
> 
> Hello,
> 
> A number of targets contained code that tested ``->prev'' as part of
> doing their frame analysis.  I think this ``cheating''.  The objective
> of the frame analysis code is to create the ->prev frame using
> information from the current frame, not to use the still being created
> ->prev frame :-)
> 
> Two cases - frame_chain for z8k and s390 - were, I think, simply wrong.
>   The deleted z8k comment is telling - someone was 180 degrees out! :-)

I notice that your patch does not preserve the functionality on s390 --
if the condition is false, the old code would set prev_fp, but yours
will not.

> The other two cases - get_saved_regs for SPARC and rs6000 - are more
> interesting.
> 
> The SPARC code clearly relies on there already being a ->prev frame so
> I've used get_prev_frame().  Since ->prev must exist, there is no risk
> of recursion - get_prev_frame() calling get_saved_regs() (yes, grotty).

Maybe a comment to that effect?
Otherwise the sparc bit is (belatedly) approved.  ;-)


> The rs6000 has me puzzled. I think the ->frame contains the address of
> the wrong end of the frame!  If ->frame pointed at the frame's start,
> the code below wouldn't even be needed.  Anyway, I've changed it to use
> frame_chain() (I don't see regressions on NetBSD/PPC.  The other
> possability would be to risk a (recursive) get_prev_frame() call.
> 
> Anyway, is the rs6000 ok?
> 
> Andrew
> 
>   ------------------------------------------------------------------------
> 2002-04-14  Andrew Cagney  <ac131313@redhat.com>
> 
>         * sparc-tdep.c (sparc_get_saved_register): Use get_prev_frame
>         instead of ->prev.
>         * z8k-tdep.c (z8k_frame_chain): Do not use ->prev.
>         * s390-tdep.c (s390_frame_chain): Do not use ->prev.
>         * rs6000-tdep.c (frame_get_saved_regs): Use rs6000_frame_chain()
>         instead of ->prev.
> 
> Index: rs6000-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
> retrieving revision 1.55
> diff -u -r1.55 rs6000-tdep.c
> --- rs6000-tdep.c       12 Apr 2002 19:48:36 -0000      1.55
> +++ rs6000-tdep.c       14 Apr 2002 16:48:56 -0000
> @@ -1388,10 +1388,12 @@
>        && fdatap->cr_offset == 0
>        && fdatap->vr_offset == 0)
>      frame_addr = 0;
> -  else if (fi->prev && fi->prev->frame)
> -    frame_addr = fi->prev->frame;
>    else
> -    frame_addr = read_memory_addr (fi->frame, wordsize);
> +    /* NOTE: cagney/2002-04-14: The ->frame points to the inner-most
> +       address of the current frame.  Things might be easier if the
> +       ->frame pointed to the outer-most address of the frame.  In the
> +       mean time, the address of the prev frame is used.  */
> +    frame_addr = rs6000_frame_chain (fi);
> 
>    /* if != -1, fdatap->saved_fpr is the smallest number of saved_fpr.
>       All fpr's from saved_fpr to fp31 are saved.  */
> Index: s390-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/s390-tdep.c,v
> retrieving revision 1.43
> diff -u -r1.43 s390-tdep.c
> --- s390-tdep.c 6 Apr 2002 00:02:50 -0000       1.43
> +++ s390-tdep.c 14 Apr 2002 16:49:11 -0000
> @@ -1009,9 +1009,7 @@
>  {
>    CORE_ADDR prev_fp = 0;
> 
> -  if (thisframe->prev && thisframe->prev->frame)
> -    prev_fp = thisframe->prev->frame;
> -  else if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
> +  if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
>      return generic_read_register_dummy (thisframe->pc, thisframe->frame,
>                                          S390_SP_REGNUM);
>    else
> Index: sparc-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
> retrieving revision 1.26
> diff -u -r1.26 sparc-tdep.c
> --- sparc-tdep.c        12 Apr 2002 18:18:57 -0000      1.26
> +++ sparc-tdep.c        14 Apr 2002 16:49:23 -0000
> @@ -829,11 +829,11 @@
>             addr = frame1->frame + (regnum - G0_REGNUM) * SPARC_INTREG_SIZE
>               - (FP_REGISTER_BYTES + 8 * SPARC_INTREG_SIZE);
>           else if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_I0);
>           else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_L0);
>           else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
> @@ -875,11 +875,11 @@
>         {
>           /* Normal frame.  Local and In registers are saved on stack.  */
>           if (regnum >= I0_REGNUM && regnum < I0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - I0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_I0);
>           else if (regnum >= L0_REGNUM && regnum < L0_REGNUM + 8)
> -           addr = (frame1->prev->extra_info->bottom
> +           addr = (get_prev_frame (frame1)->extra_info->bottom
>                     + (regnum - L0_REGNUM) * SPARC_INTREG_SIZE
>                     + FRAME_SAVED_L0);
>           else if (regnum >= O0_REGNUM && regnum < O0_REGNUM + 8)
> Index: z8k-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/z8k-tdep.c,v
> retrieving revision 1.7
> diff -u -r1.7 z8k-tdep.c
> --- z8k-tdep.c  12 Apr 2002 18:18:57 -0000      1.7
> +++ z8k-tdep.c  14 Apr 2002 16:49:25 -0000
> @@ -160,10 +160,6 @@
>  CORE_ADDR
>  z8k_frame_chain (struct frame_info *thisframe)
>  {
> -  if (thisframe->prev == 0)
> -    {
> -      /* This is the top of the stack, let's get the sp for real */
> -    }
>    if (!inside_entry_file (thisframe->pc))
>      {
>        return read_memory_pointer (thisframe->frame);

Re: [patch/rfa:rs6000] Don't use ->prev

[Andrew Cagney]

> Andrew Cagney wrote:
> 
>> 
>> Hello,
>> 
>> A number of targets contained code that tested ``->prev'' as part of
>> doing their frame analysis.  I think this ``cheating''.  The objective
>> of the frame analysis code is to create the ->prev frame using
>> information from the current frame, not to use the still being created
>> ->prev frame :-)
>> 
>> Two cases - frame_chain for z8k and s390 - were, I think, simply wrong.
>>   The deleted z8k comment is telling - someone was 180 degrees out! :-)
> 
> 
> I notice that your patch does not preserve the functionality on s390 --
> if the condition is false, the old code would set prev_fp, but yours
> will not.

Yes.  That shouldn't matter.

  {
    CORE_ADDR prev_fp = 0;

-  if (thisframe->prev && thisframe->prev->frame)
-    prev_fp = thisframe->prev->frame;
-  else if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
+  if (generic_find_dummy_frame (thisframe->pc, thisframe->frame))
      return generic_read_register_dummy (thisframe->pc,

The function (frame_chain()) will simply compute prev_fp using thisframe.


>> The other two cases - get_saved_regs for SPARC and rs6000 - are more
>> interesting.
>> 
>> The SPARC code clearly relies on there already being a ->prev frame so
>> I've used get_prev_frame().  Since ->prev must exist, there is no risk
>> of recursion - get_prev_frame() calling get_saved_regs() (yes, grotty).
> 
> 
> Maybe a comment to that effect?
> Otherwise the sparc bit is (belatedly) approved.  ;-)

I've added this comment:

	    /* NOTE: cagney/2002-05-04: The call to get_prev_frame()
                is safe/cheap - there will always be a prev frame.
                This is because frame1 is initialized to frame->next
                (frame1->prev == frame) and is then advanced towards
                the innermost (next) frame.  */


Andrew