[RFA] Rewrite mips_get_saved_register()

[RFA] Rewrite mips_get_saved_register()

[Kevin Buettner]

This patch depends on the following (as of yet) unapproved patches:

    http://sources.redhat.com/ml/gdb-patches/2002-08/msg00189.html
    http://sources.redhat.com/ml/gdb-patches/2002-08/msg00195.html
    http://sources.redhat.com/ml/gdb-patches/2002-08/msg00196.html

Okay to commit?

	* mips-tdep.c (mips_get_saved_register): Rewrite to use
	frame_register_unwind() instead of find_saved_register().

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.93
diff -u -p -r1.93 mips-tdep.c
--- mips-tdep.c	8 Aug 2002 23:32:52 -0000	1.93
+++ mips-tdep.c	9 Aug 2002 00:26:00 -0000
@@ -4241,53 +4246,48 @@ mips_get_saved_register (char *raw_buffe
 			 int regnum,
 			 enum lval_type *lval)
 {
-  CORE_ADDR addr;
+  int realnum;
+  int scratch_optimized;
+  enum lval_type scratch_lval;
+  CORE_ADDR scratch_addr;
 
   if (!target_has_registers)
     error ("No registers.");
 
-  /* Normal systems don't optimize out things with register numbers.  */
-  if (optimized != NULL)
-    *optimized = 0;
-  addr = find_saved_register (frame, regnum);
-  if (addr != 0)
-    {
-      if (lval != NULL)
-	*lval = lval_memory;
-      if (regnum == SP_REGNUM)
-	{
-	  if (raw_buffer != NULL)
-	    {
-	      /* Put it back in target format.  */
-	      store_address (raw_buffer, REGISTER_RAW_SIZE (regnum),
-			     (LONGEST) addr);
-	    }
-	  if (addrp != NULL)
-	    *addrp = 0;
-	  return;
-	}
-      if (raw_buffer != NULL)
-	{
-	  LONGEST val;
-	  if (regnum < 32)
-	    /* Only MIPS_SAVED_REGSIZE bytes of GP registers are
-               saved. */
-	    val = read_memory_integer (addr, MIPS_SAVED_REGSIZE);
-	  else
-	    val = read_memory_integer (addr, REGISTER_RAW_SIZE (regnum));
-	  store_address (raw_buffer, REGISTER_RAW_SIZE (regnum), val);
-	}
-    }
-  else
-    {
-      if (lval != NULL)
-	*lval = lval_register;
-      addr = REGISTER_BYTE (regnum);
-      if (raw_buffer != NULL)
-	read_register_gen (regnum, raw_buffer);
+  /* frame_register_unwind() requires ``optimized'', ``lval'',  ``addrp'',
+     and ``realnum'' to be non-NULL.  So use the scratch variables, if
+     necessary.  (Note that realnum isn't a problem since we pass the
+     address of local storage directly.)  */
+  if (optimized == NULL)
+    optimized = &scratch_optimized;
+  if (addrp == NULL)
+    addrp = &scratch_addr;
+  if (lval == NULL)
+    lval = &scratch_lval;
+
+  /* Fetch register REGNUM from an inner (callee) frame.  */
+  frame_register_unwind (get_next_frame (frame), regnum, optimized,
+                         lval, addrp, &realnum, raw_buffer);
+
+  /* Only MIPS_SAVED_REGSIZE bytes of GP registers are saved on the
+     stack.  If this differs from the raw size, then frame_register_unwind()
+     fetched too much.  Adjust value fetched and save back into the buffer.  
+     (The excess is thus discarded.)
+     
+     The SP register is handled specially.  Its value will have been
+     set correctly by frame_register_unwind(), so don't attempt to adjust
+     it.  */
+  if (*addrp != 0 			/* Register must be saved on stack,  */
+      && regnum < 32			/* must be a GP register,  */
+      && regnum != SP_REGNUM		/* but not SP.  */
+      && MIPS_SAVED_REGSIZE < REGISTER_RAW_SIZE (regnum))
+    {
+      LONGEST val;
+      
+      val = extract_signed_integer (raw_buffer, MIPS_SAVED_REGSIZE);
+      store_signed_integer (raw_buffer, REGISTER_RAW_SIZE (regnum),
+                            val);
     }
-  if (addrp != NULL)
-    *addrp = addr;
 }
 
 /* Immediately after a function call, return the saved pc.

Re: [RFA] Rewrite mips_get_saved_register()

[Andrew Cagney]

> This patch depends on the following (as of yet) unapproved patches:
> 
>     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00189.html
>     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00195.html
>     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00196.html
> 
> Okay to commit?
> 
> 	* mips-tdep.c (mips_get_saved_register): Rewrite to use
> 	frame_register_unwind() instead of find_saved_register().

Sorry, not like this.  Can you please, for the moment, just in-line the 
call to find_saved_register().

(The two other bugs you fixed were definitly needed and thanks for 
finding them.  How the code got away with not setting the SP I don't know.)

-      if (raw_buffer != NULL)
- 
{
- 
   LONGEST val;
- 
   if (regnum < 32)
- 
     /* Only MIPS_SAVED_REGSIZE bytes of GP registers are
-               saved. */
- 
     val = read_memory_integer (addr, MIPS_SAVED_REGSIZE);
- 
   else
- 
     val = read_memory_integer (addr, REGISTER_RAW_SIZE (regnum));
- 
   store_address (raw_buffer, REGISTER_RAW_SIZE (regnum), val);
- 
}

In theory (and emphasis on the theory) things need to be changed so that:

- 32 32 bit nameless pseudo-registers are added to the cooked register space
- the 32 64 bit gprs get given a 64 bit virtual type so that they have 
identical raw and virtual sizes
- The debug info, for a 32 bit ABI, maps the gpr register numbers onto 
the 32 bit pseudo register range
- The gdbarch pseudo register read/write function maps the 32 bit 
pseudo-registers onto the 64 bit gprs.
- For init saved regs, dependant on the size of the register saved, 
either the the address of the 64 bit GPR or the address of the 32 bit 
pseudo-register is set
- a custom mips register unwind function maps the requested register (64 
bit gpr or 32 bit pseudo) onto: the 32 bit pseudo, the 64 bit gpr, or a 
further recursive unwind call.  If it has to do a 32/64 mapping then it 
sets not-an-lval.
- you find you have to yank all sorts of register converible code

But like I said, it is theory, the mips suffers from one hack (like the 
above) piled on top of another (the register convertable stuff, the 
register raw/virtual size being different, ...).  I don't know if now is 
the time to be experimenting with theories :-)

Andrew

Re: [RFA] Rewrite mips_get_saved_register()

[Kevin Buettner]

On Aug 9, 12:12pm, Andrew Cagney wrote:

> > This patch depends on the following (as of yet) unapproved patches:
> > 
> >     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00189.html
> >     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00195.html
> >     http://sources.redhat.com/ml/gdb-patches/2002-08/msg00196.html
> > 
> > Okay to commit?
> > 
> > 	* mips-tdep.c (mips_get_saved_register): Rewrite to use
> > 	frame_register_unwind() instead of find_saved_register().
> 
> Sorry, not like this.  Can you please, for the moment, just in-line the 
> call to find_saved_register().
> 
> (The two other bugs you fixed were definitly needed and thanks for 
> finding them.  How the code got away with not setting the SP I don't know.)
> 
> -      if (raw_buffer != NULL)
> - 
> {
> - 
>    LONGEST val;
> - 
>    if (regnum < 32)
> - 
>      /* Only MIPS_SAVED_REGSIZE bytes of GP registers are
> -               saved. */
> - 
>      val = read_memory_integer (addr, MIPS_SAVED_REGSIZE);
> - 
>    else
> - 
>      val = read_memory_integer (addr, REGISTER_RAW_SIZE (regnum));
> - 
>    store_address (raw_buffer, REGISTER_RAW_SIZE (regnum), val);
> - 
> }
> 
> In theory (and emphasis on the theory) things need to be changed so that:
> 
> - 32 32 bit nameless pseudo-registers are added to the cooked register space
> - the 32 64 bit gprs get given a 64 bit virtual type so that they have 
> identical raw and virtual sizes
> - The debug info, for a 32 bit ABI, maps the gpr register numbers onto 
> the 32 bit pseudo register range
> - The gdbarch pseudo register read/write function maps the 32 bit 
> pseudo-registers onto the 64 bit gprs.
> - For init saved regs, dependant on the size of the register saved, 
> either the the address of the 64 bit GPR or the address of the 32 bit 
> pseudo-register is set
> - a custom mips register unwind function maps the requested register (64 
> bit gpr or 32 bit pseudo) onto: the 32 bit pseudo, the 64 bit gpr, or a 
> further recursive unwind call.  If it has to do a 32/64 mapping then it 
> sets not-an-lval.
> - you find you have to yank all sorts of register converible code
> 
> But like I said, it is theory, the mips suffers from one hack (like the 
> above) piled on top of another (the register convertable stuff, the 
> register raw/virtual size being different, ...).  I don't know if now is 
> the time to be experimenting with theories :-)

Yes, fixing all of the above is quite a lot more work than I have time
for at the moment.

But, assuming that my rewrite is functionally equivalent to the
original, what is the reason for preferring to inline
find_saved_register() over using the new interface?

I should think that using frame_register_unwind() would:

    - allow it to be better tested.  E.g, I wouldn't have found
      the bugs that I did recently had I not gone through this
      exercise.
    - shorten the time needed to reexamine mips_get_saved_register()
      when it finally does come time to reimplement or eliminate it.

Kevin

Re: [RFA] Rewrite mips_get_saved_register()

[Andrew Cagney]

>> In theory (and emphasis on the theory) things need to be changed so that:
>> 
>> - 32 32 bit nameless pseudo-registers are added to the cooked register space
>> - the 32 64 bit gprs get given a 64 bit virtual type so that they have 
>> identical raw and virtual sizes
>> - The debug info, for a 32 bit ABI, maps the gpr register numbers onto 
>> the 32 bit pseudo register range
>> - The gdbarch pseudo register read/write function maps the 32 bit 
>> pseudo-registers onto the 64 bit gprs.
>> - For init saved regs, dependant on the size of the register saved, 
>> either the the address of the 64 bit GPR or the address of the 32 bit 
>> pseudo-register is set
>> - a custom mips register unwind function maps the requested register (64 
>> bit gpr or 32 bit pseudo) onto: the 32 bit pseudo, the 64 bit gpr, or a 
>> further recursive unwind call.  If it has to do a 32/64 mapping then it 
>> sets not-an-lval.
>> - you find you have to yank all sorts of register converible code
>> 
>> But like I said, it is theory, the mips suffers from one hack (like the 
>> above) piled on top of another (the register convertable stuff, the 
>> register raw/virtual size being different, ...).  I don't know if now is 
>> the time to be experimenting with theories :-)
> 
> 
> Yes, fixing all of the above is quite a lot more work than I have time
> for at the moment.
> 
> But, assuming that my rewrite is functionally equivalent to the
> original, what is the reason for preferring to inline
> find_saved_register() over using the new interface?

Until the MIPS is ready, I think it is better to keep it and the unwind 
code well separated.  We otherwize run the real risk of having 
unwind/cfi changes going off the rails because they break the (already 
broken) MIPS.

The patch assumes, for instance, that the unwind address returned is the 
exact same address that was computed by init_saved_regs(), for CFI, that 
won't hold.

enjoy,
Andrew