Re: [PATCH/mips] mips16 frame code simplification

[Joel Brobecker <brobecker@gnat.com>]

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

[grrr, sorry, resending with the patch]

(Andrew, if you could look at this change, just to make sure there's
no obvious screwup. Thanks!)

This is the mips16 counterpart of a simplification I made on 2004-09-14
for mips32:

        http://sources.redhat.com/ml/gdb-patches/2004-09/msg00245.html

2004-10-10  Joel Brobecker  <brobecker@gnat.com>

        * mips-tdep.c (mips16_decode_reg_save): Delete, no longer used.
        (mips_insn16_frame_cache): Pass frame cache in call to
        heuristic_proc_desc. Move some code to mips16_heuristic_proc_desc.
        Remove code that became redundant as a consequence.
        (mips32_heuristic_proc_desc): No longer compute a fake
        procedure descriptor. Compute the full frame cache instead.
        Some minor comment reformatting.

Not having a mips16 target handy, I couldn't test this change.
However, it just basically did the same work than I did for mips32,
with extra care as to what I was doing. And I also verified that this
code still builds.

This should open the door to further cleanups in this file.

Comitted,
-- 
Joel

[-- Attachment #2: mips-tdep.c.diff --]
[-- Type: text/plain, Size: 14091 bytes --]

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.327
retrieving revision 1.328
diff -u -p -r1.327 -r1.328
--- mips-tdep.c	7 Oct 2004 17:17:08 -0000	1.327
+++ mips-tdep.c	11 Oct 2004 01:00:57 -0000	1.328
@@ -945,30 +945,6 @@ after_prologue (CORE_ADDR pc)
   return 0;
 }
 
-/* Decode a MIPS16 instruction that saves a register in the stack, and
-   set the appropriate bit in the general register or float register mask
-   to indicate which register is saved.  This is a helper function
-   for mips_find_saved_regs.  */
-
-static void
-mips16_decode_reg_save (t_inst inst, unsigned long *gen_mask)
-{
-  if ((inst & 0xf800) == 0xd000)	/* sw reg,n($sp) */
-    {
-      int reg = mips16_to_32_reg[(inst & 0x700) >> 8];
-      *gen_mask |= (1 << reg);
-    }
-  else if ((inst & 0xff00) == 0xf900)	/* sd reg,n($sp) */
-    {
-      int reg = mips16_to_32_reg[(inst & 0xe0) >> 5];
-      *gen_mask |= (1 << reg);
-    }
-  else if ((inst & 0xff00) == 0x6200	/* sw $ra,n($sp) */
-	   || (inst & 0xff00) == 0xfa00)	/* sd $ra,n($sp) */
-    *gen_mask |= (1 << RA_REGNUM);
-}
-
-
 /* Fetch and return instruction from the specified location.  If the PC
    is odd, assume it's a MIPS16 instruction; otherwise MIPS32.  */
 
@@ -1765,154 +1741,9 @@ mips_insn16_frame_cache (struct frame_in
     if (start_addr == 0)
       start_addr = heuristic_proc_start (pc);
 
-#ifdef NOT_YET
     proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, *this_cache);
-#else
-    proc_desc = heuristic_proc_desc (start_addr, pc, next_frame, NULL);
-#endif
   }
   
-  /* Extract the frame's base.  */
-  cache->base = (frame_unwind_register_signed (next_frame, NUM_REGS + PROC_FRAME_REG (proc_desc))
-		 + PROC_FRAME_OFFSET (proc_desc) - PROC_FRAME_ADJUST (proc_desc));
-
-  kernel_trap = PROC_REG_MASK (proc_desc) & 1;
-  gen_mask = kernel_trap ? 0xFFFFFFFF : PROC_REG_MASK (proc_desc);
-  float_mask = kernel_trap ? 0xFFFFFFFF : PROC_FREG_MASK (proc_desc);
-  
-  /* In any frame other than the innermost or a frame interrupted by a
-     signal, we assume that all registers have been saved.  This
-     assumes that all register saves in a function happen before the
-     first function call.  */
-  if (in_prologue (frame_pc_unwind (next_frame), PROC_LOW_ADDR (proc_desc))
-      /* Not sure exactly what kernel_trap means, but if it means the
-	 kernel saves the registers without a prologue doing it, we
-	 better not examine the prologue to see whether registers
-	 have been saved yet.  */
-      && !kernel_trap)
-    {
-      /* We need to figure out whether the registers that the
-         proc_desc claims are saved have been saved yet.  */
-
-      CORE_ADDR addr;
-
-      /* Bitmasks; set if we have found a save for the register.  */
-      unsigned long gen_save_found = 0;
-      unsigned long float_save_found = 0;
-      int mips16;
-
-      /* If the address is odd, assume this is MIPS16 code.  */
-      addr = PROC_LOW_ADDR (proc_desc);
-
-      /* Scan through this function's instructions preceding the
-         current PC, and look for those that save registers.  */
-      while (addr < frame_pc_unwind (next_frame))
-	{
-          mips16_decode_reg_save (mips16_fetch_instruction (addr),
-                                  &gen_save_found);
-          addr += MIPS16_INSTLEN;
-	}
-      gen_mask = gen_save_found;
-      float_mask = float_save_found;
-    }
-
-  /* Fill in the offsets for the registers which gen_mask says were
-     saved.  */
-  {
-    CORE_ADDR reg_position = (cache->base
-			      + PROC_REG_OFFSET (proc_desc));
-    int ireg;
-    for (ireg = MIPS_NUMREGS - 1; gen_mask; --ireg, gen_mask <<= 1)
-      if (gen_mask & 0x80000000)
-	{
-	  cache->saved_regs[NUM_REGS + ireg].addr = reg_position;
-	  reg_position -= mips_abi_regsize (gdbarch);
-	}
-  }
-
-  /* The MIPS16 entry instruction saves $s0 and $s1 in the reverse
-     order of that normally used by gcc.  Therefore, we have to fetch
-     the first instruction of the function, and if it's an entry
-     instruction that saves $s0 or $s1, correct their saved addresses.  */
-    {
-      ULONGEST inst = mips16_fetch_instruction (PROC_LOW_ADDR (proc_desc));
-      if ((inst & 0xf81f) == 0xe809 && (inst & 0x700) != 0x700)
-	/* entry */
-	{
-	  int reg;
-	  int sreg_count = (inst >> 6) & 3;
-
-	  /* Check if the ra register was pushed on the stack.  */
-	  CORE_ADDR reg_position = (cache->base
-				    + PROC_REG_OFFSET (proc_desc));
-	  if (inst & 0x20)
-	    reg_position -= mips_abi_regsize (gdbarch);
-
-	  /* Check if the s0 and s1 registers were pushed on the
-	     stack.  */
-	  /* NOTE: cagney/2004-02-08: Huh?  This is doing no such
-             check.  */
-	  for (reg = 16; reg < sreg_count + 16; reg++)
-	    {
-	      cache->saved_regs[NUM_REGS + reg].addr = reg_position;
-	      reg_position -= mips_abi_regsize (gdbarch);
-	    }
-	}
-    }
-
-  /* Fill in the offsets for the registers which float_mask says were
-     saved.  */
-  {
-    CORE_ADDR reg_position = (cache->base
-			      + PROC_FREG_OFFSET (proc_desc));
-    int ireg;
-    /* Fill in the offsets for the float registers which float_mask
-       says were saved.  */
-    for (ireg = MIPS_NUMREGS - 1; float_mask; --ireg, float_mask <<= 1)
-      if (float_mask & 0x80000000)
-	{
-	  if (mips_abi_regsize (gdbarch) == 4
-	      && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
-	    {
-	      /* On a big endian 32 bit ABI, floating point registers
-	         are paired to form doubles such that the most
-	         significant part is in $f[N+1] and the least
-	         significant in $f[N] vis: $f[N+1] ||| $f[N].  The
-	         registers are also spilled as a pair and stored as a
-	         double.
-
-	         When little-endian the least significant part is
-	         stored first leading to the memory order $f[N] and
-	         then $f[N+1].
-
-	         Unfortunately, when big-endian the most significant
-	         part of the double is stored first, and the least
-	         significant is stored second.  This leads to the
-	         registers being ordered in memory as firt $f[N+1] and
-	         then $f[N].
-
-	         For the big-endian case make certain that the
-	         addresses point at the correct (swapped) locations
-	         $f[N] and $f[N+1] pair (keep in mind that
-	         reg_position is decremented each time through the
-	         loop).  */
-	      if ((ireg & 1))
-		cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
-		  .addr = reg_position - mips_abi_regsize (gdbarch);
-	      else
-		cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
-		  .addr = reg_position + mips_abi_regsize (gdbarch);
-	    }
-	  else
-	    cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->fp0 + ireg]
-	      .addr = reg_position;
-	  reg_position -= mips_abi_regsize (gdbarch);
-	}
-
-    cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
-      = cache->saved_regs[NUM_REGS + RA_REGNUM];
-  }
-
   /* SP_REGNUM, contains the value and not the address.  */
   trad_frame_set_value (cache->saved_regs, NUM_REGS + MIPS_SP_REGNUM, cache->base);
 
@@ -2445,14 +2276,13 @@ mips16_heuristic_proc_desc (CORE_ADDR st
 {
   CORE_ADDR cur_pc;
   CORE_ADDR frame_addr = 0;	/* Value of $r17, used as frame pointer */
+  long frame_offset = 0;        /* Size of stack frame.  */
+  long frame_adjust = 0;        /* Offset of FP from SP.  */
+  int frame_reg = MIPS_SP_REGNUM;
   unsigned short prev_inst = 0;	/* saved copy of previous instruction */
   unsigned inst = 0;		/* current instruction */
   unsigned entry_inst = 0;	/* the entry instruction */
   int reg, offset;
-  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
-
-  PROC_FRAME_OFFSET (&temp_proc_desc) = 0;	/* size of stack frame */
-  PROC_FRAME_ADJUST (&temp_proc_desc) = 0;	/* offset of FP from SP */
 
   for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += MIPS16_INSTLEN)
     {
@@ -2467,7 +2297,7 @@ mips16_heuristic_proc_desc (CORE_ADDR st
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 8, 8, 1);
 	  if (offset < 0)	/* negative stack adjustment? */
-	    PROC_FRAME_OFFSET (&temp_proc_desc) -= offset;
+	    frame_offset -= offset;
 	  else
 	    /* Exit loop if a positive stack adjustment is found, which
 	       usually means that the stack cleanup code in the function
@@ -2478,55 +2308,50 @@ mips16_heuristic_proc_desc (CORE_ADDR st
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 8, 4, 0);
 	  reg = mips16_to_32_reg[(inst & 0x700) >> 8];
-	  PROC_REG_MASK (&temp_proc_desc) |= (1 << reg);
 	  set_reg_offset (this_cache, reg, sp + offset);
 	}
       else if ((inst & 0xff00) == 0xf900)	/* sd reg,n($sp) */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 5, 8, 0);
 	  reg = mips16_to_32_reg[(inst & 0xe0) >> 5];
-	  PROC_REG_MASK (&temp_proc_desc) |= (1 << reg);
 	  set_reg_offset (this_cache, reg, sp + offset);
 	}
       else if ((inst & 0xff00) == 0x6200)	/* sw $ra,n($sp) */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 8, 4, 0);
-	  PROC_REG_MASK (&temp_proc_desc) |= (1 << RA_REGNUM);
 	  set_reg_offset (this_cache, RA_REGNUM, sp + offset);
 	}
       else if ((inst & 0xff00) == 0xfa00)	/* sd $ra,n($sp) */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 8, 8, 0);
-	  PROC_REG_MASK (&temp_proc_desc) |= (1 << RA_REGNUM);
 	  set_reg_offset (this_cache, RA_REGNUM, sp + offset);
 	}
       else if (inst == 0x673d)	/* move $s1, $sp */
 	{
 	  frame_addr = sp;
-	  PROC_FRAME_REG (&temp_proc_desc) = 17;
+	  frame_reg = 17;
 	}
       else if ((inst & 0xff00) == 0x0100)	/* addiu $s1,sp,n */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 8, 4, 0);
 	  frame_addr = sp + offset;
-	  PROC_FRAME_REG (&temp_proc_desc) = 17;
-	  PROC_FRAME_ADJUST (&temp_proc_desc) = offset;
+	  frame_reg = 17;
+	  frame_adjust = offset;
 	}
       else if ((inst & 0xFF00) == 0xd900)	/* sw reg,offset($s1) */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 5, 4, 0);
 	  reg = mips16_to_32_reg[(inst & 0xe0) >> 5];
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, reg, frame_addr + offset);
 	}
       else if ((inst & 0xFF00) == 0x7900)	/* sd reg,offset($s1) */
 	{
 	  offset = mips16_get_imm (prev_inst, inst, 5, 8, 0);
 	  reg = mips16_to_32_reg[(inst & 0xe0) >> 5];
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, reg, frame_addr + offset);
 	}
-      else if ((inst & 0xf81f) == 0xe809 && (inst & 0x700) != 0x700)	/* entry */
+      else if ((inst & 0xf81f) == 0xe809
+               && (inst & 0x700) != 0x700)	/* entry */
 	entry_inst = inst;	/* save for later processing */
       else if ((inst & 0xf800) == 0x1800)	/* jal(x) */
 	cur_pc += MIPS16_INSTLEN;	/* 32-bit instruction */
@@ -2544,16 +2369,15 @@ mips16_heuristic_proc_desc (CORE_ADDR st
       int sreg_count = (entry_inst >> 6) & 3;
 
       /* The entry instruction always subtracts 32 from the SP.  */
-      PROC_FRAME_OFFSET (&temp_proc_desc) += 32;
+      frame_offset += 32;
 
       /* Now we can calculate what the SP must have been at the
          start of the function prologue.  */
-      sp += PROC_FRAME_OFFSET (&temp_proc_desc);
+      sp += frame_offset;
 
       /* Check if a0-a3 were saved in the caller's argument save area.  */
       for (reg = 4, offset = 0; reg < areg_count + 4; reg++)
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, reg, sp + offset);
 	  offset += mips_abi_regsize (current_gdbarch);
 	}
@@ -2562,7 +2386,6 @@ mips16_heuristic_proc_desc (CORE_ADDR st
       offset = -4;
       if (entry_inst & 0x20)
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << RA_REGNUM;
 	  set_reg_offset (this_cache, RA_REGNUM, sp + offset);
 	  offset -= mips_abi_regsize (current_gdbarch);
 	}
@@ -2570,11 +2393,52 @@ mips16_heuristic_proc_desc (CORE_ADDR st
       /* Check if the s0 and s1 registers were pushed on the stack.  */
       for (reg = 16; reg < sreg_count + 16; reg++)
 	{
-	  PROC_REG_MASK (&temp_proc_desc) |= 1 << reg;
 	  set_reg_offset (this_cache, reg, sp + offset);
 	  offset -= mips_abi_regsize (current_gdbarch);
 	}
     }
+
+  if (this_cache != NULL)
+    {
+      this_cache->base =
+        (frame_unwind_register_signed (next_frame, NUM_REGS + frame_reg)
+         + frame_offset - frame_adjust);
+      /* FIXME: brobecker/2004-10-10: Just as in the mips32 case, we should
+         be able to get rid of the assignment below, evetually. But it's
+         still needed for now.  */
+      this_cache->saved_regs[NUM_REGS + mips_regnum (current_gdbarch)->pc]
+        = this_cache->saved_regs[NUM_REGS + RA_REGNUM];
+    }
+
+  /* The MIPS16 entry instruction saves $s0 and $s1 in the reverse
+     order of that normally used by gcc.  Therefore, we have to fetch
+     the first instruction of the function, and if it's an entry
+     instruction that saves $s0 or $s1, correct their saved addresses.  */
+  /* FIXME: brobecker/2004-10-10: This code was moved here from
+     mips_insn16_frame_cache(), but can be merged with the block above
+     handling entry_inst.  Will be done in a separate pass, to make changes
+     more atomic.  Actually, this code seems completely redundant!  */
+    {
+      ULONGEST inst = mips16_fetch_instruction (start_pc);
+      if ((inst & 0xf81f) == 0xe809 && (inst & 0x700) != 0x700) /* entry */
+	{
+	  int reg;
+	  int sreg_count = (inst >> 6) & 3;
+	  CORE_ADDR reg_position = (this_cache->base);
+
+	  /* Check if the ra register was pushed on the stack.  */
+	  if (inst & 0x20)
+	    reg_position -= mips_abi_regsize (current_gdbarch);
+
+	  /* Check if the s0 and s1 registers were pushed on the stack.  */
+	  /* NOTE: cagney/2004-02-08: Huh?  This is doing no such check.  */
+	  for (reg = 16; reg < sreg_count + 16; reg++)
+	    {
+	      this_cache->saved_regs[NUM_REGS + reg].addr = reg_position;
+	      reg_position -= mips_abi_regsize (current_gdbarch);
+	    }
+	}
+    }
 }
 
 /* Mark all the registers as unset in the saved_regs array