This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [patch/rfa:rs6000] Don't use ->prev
- From: Michael Snyder <msnyder at redhat dot com>
- To: Andrew Cagney <ac131313 at cygnus dot com>
- Cc: gdb-patches at sources dot redhat dot com, Kevin Buettner <kevinb at redhat dot com>
- Date: Wed, 24 Apr 2002 14:56:59 -0700
- Subject: Re: [patch/rfa:rs6000] Don't use ->prev
- Organization: Red Hat, Inc.
- References: <3CB9B3B1.90007@cygnus.com>
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);