[rfc][6/13] Eliminate read_register: read_register (invalidly) used in prologue analyzers

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hello,

while eliminating the read_register family of routines, I noticed a couple
of places where read_register is currently used in what appears to be an
invalid fashion as part of prologue analyzer routines.

Those routines are generally called from two places: the unwinder, in which
case you should examine register values from the next frame instead of
referring to global state, and the skip_prologue callback, which doesn't
refer to *any* frame, but the global register state is invalid as well,
since the callback can be invoked at any time (e.g. when re-evaluating
breakpoints).

The three problematic locations I found are:

- v850_analyze_prologue uses read_register to retrieve the CTBP register
  (call table base pointer).  As this is used *only* from within the
  unwinder, the patch adds a CTBP argument to v850_analyze_prologue and
  has the unwinder pass in the unwound value of the register.

- Similarly, sh_analyze_prologue reads the FPSCR register to find out
  whether a floating-point "push" instruction decrements the stack
  pointer by 4 or 8 bytes.  Again, I've added a FPSCR parameter and have
  the unwinder pass in the unwound register value.  However, this 
  routine is also called from the skip_prologue callback.  In this case,
  we simply do not know the contents of that flags register.

  However, the only thing we get wrong if we don't know that setting
  is the stack offset of saved registers, and skip_prologue does not
  actually care about those.  So just arbitrarily passing in 0 in that
  case should work fine.

  This all really needs to be separated out more cleanly ...

- Finally, the mips{16,32}_scan_prologue routines are called either
  from the unwinder with a next_frame argument, or from skip_prologue
  with a NULL next_frame argument.  They make use of a helper routine
  read_next_frame_reg that either unwinds from next_frame or else
  reads from the current_regcache.  That strikes me as bogus, as the
  current_regcache isn't at all related to the function being
  queried via skip_prologue ...

  However, again the only thing we'd be getting wrong should be
  stack offsets of saved registers in the case of functions using
  alloca.  So the patch below changes the scan_prologue routines to
  skip trying to handle that case if called without next_frame.
  

Bye,
Ulrich


ChangeLog:

	* mips-tdep.c (mips16_scan_prologue): Replace read_next_frame_reg
	by frame_unwind_register_signed calls.
	(mips32_scan_prologue): Likewise.  Skip analysis of alloca stack
	frame allocations when called with NULL NEXT_FRAME parameter.
	(read_next_frame_reg): Remove.

	* sh-tdep.c (sh_analyze_prologue): Add FPSCR parameter.  Use it
	instead of reading the FPSCR register.
	(sh_frame_cache): Pass unwound FPSCR register value to
	sh_analyze_prologue.
	(sh_skip_prologue): Pass dummy FPSCR value to sh_analyze_prologue.

	* v850-tdep.c (v850_analyze_prologue): Add CTBP parameter.  Use it
	instead of reading the CTBP register.
	(v850_frame_cache): Pass unwound CTBP register value to
	v850_analyze_prologue.

diff -urNp gdb-orig/gdb/mips-tdep.c gdb-head/gdb/mips-tdep.c
--- gdb-orig/gdb/mips-tdep.c	2007-06-06 18:57:49.422550908 +0200
+++ gdb-head/gdb/mips-tdep.c	2007-06-06 18:58:57.267028491 +0200
@@ -402,8 +402,6 @@ mips2_fp_compat (void)
 
 static CORE_ADDR heuristic_proc_start (CORE_ADDR);
 
-static CORE_ADDR read_next_frame_reg (struct frame_info *, int);
-
 static void reinit_frame_cache_sfunc (char *, int, struct cmd_list_element *);
 
 static struct type *mips_float_register_type (void);
@@ -1436,8 +1434,9 @@ mips16_scan_prologue (CORE_ADDR start_pc
   /* Can be called when there's no process, and hence when there's no
      NEXT_FRAME.  */
   if (next_frame != NULL)
-    sp = read_next_frame_reg (next_frame, gdbarch_num_regs (current_gdbarch)
-					   + MIPS_SP_REGNUM);
+    sp = frame_unwind_register_signed (next_frame,
+				       gdbarch_num_regs (current_gdbarch)
+				       + MIPS_SP_REGNUM);
   else
     sp = 0;
 
@@ -1757,8 +1756,9 @@ mips32_scan_prologue (CORE_ADDR start_pc
   /* Can be called when there's no process, and hence when there's no
      NEXT_FRAME.  */
   if (next_frame != NULL)
-    sp = read_next_frame_reg (next_frame, gdbarch_num_regs (current_gdbarch)
-					  + MIPS_SP_REGNUM);
+    sp = frame_unwind_register_signed (next_frame,
+				       gdbarch_num_regs (current_gdbarch)
+				       + MIPS_SP_REGNUM);
   else
     sp = 0;
 
@@ -1808,14 +1808,15 @@ restart:
 	  /* Old gcc frame, r30 is virtual frame pointer.  */
 	  if ((long) low_word != frame_offset)
 	    frame_addr = sp + low_word;
-	  else if (frame_reg == MIPS_SP_REGNUM)
+	  else if (next_frame && frame_reg == MIPS_SP_REGNUM)
 	    {
 	      unsigned alloca_adjust;
 
 	      frame_reg = 30;
-	      frame_addr = read_next_frame_reg (next_frame,
-						gdbarch_num_regs
-						  (current_gdbarch) + 30);
+	      frame_addr = frame_unwind_register_signed
+			     (next_frame,
+			      gdbarch_num_regs (current_gdbarch) + 30);
+
 	      alloca_adjust = (unsigned) (frame_addr - (sp + low_word));
 	      if (alloca_adjust > 0)
 		{
@@ -1838,14 +1839,15 @@ restart:
       else if (inst == 0x03A0F021 || inst == 0x03a0f025 || inst == 0x03a0f02d)
 	{
 	  /* New gcc frame, virtual frame pointer is at r30 + frame_size.  */
-	  if (frame_reg == MIPS_SP_REGNUM)
+	  if (next_frame && frame_reg == MIPS_SP_REGNUM)
 	    {
 	      unsigned alloca_adjust;
 
 	      frame_reg = 30;
-	      frame_addr = read_next_frame_reg (next_frame,
-						gdbarch_num_regs 
-						  (current_gdbarch) + 30);
+	      frame_addr = frame_unwind_register_signed
+			     (next_frame,
+			      gdbarch_num_regs (current_gdbarch) + 30);
+
 	      alloca_adjust = (unsigned) (frame_addr - sp);
 	      if (alloca_adjust > 0)
 	        {
@@ -2153,22 +2155,6 @@ mips_stub_frame_base_sniffer (struct fra
     return NULL;
 }
 
-static CORE_ADDR
-read_next_frame_reg (struct frame_info *fi, int regno)
-{
-  /* Always a pseudo.  */
-  gdb_assert (regno >= gdbarch_num_regs (current_gdbarch));
-  if (fi == NULL)
-    {
-      LONGEST val;
-      regcache_cooked_read_signed (current_regcache, regno, &val);
-      return val;
-    }
-  else
-    return frame_unwind_register_signed (fi, regno);
-
-}
-
 /* mips_addr_bits_remove - remove useless address bits  */
 
 static CORE_ADDR
diff -urNp gdb-orig/gdb/sh-tdep.c gdb-head/gdb/sh-tdep.c
--- gdb-orig/gdb/sh-tdep.c	2007-06-06 18:58:46.803937873 +0200
+++ gdb-head/gdb/sh-tdep.c	2007-06-06 18:58:57.322020578 +0200
@@ -508,7 +508,7 @@ gdb_print_insn_sh (bfd_vma memaddr, disa
 
 static CORE_ADDR
 sh_analyze_prologue (CORE_ADDR pc, CORE_ADDR current_pc,
-		     struct sh_frame_cache *cache)
+		     struct sh_frame_cache *cache, ULONGEST fpscr)
 {
   ULONGEST inst;
   CORE_ADDR opc;
@@ -615,7 +615,7 @@ sh_analyze_prologue (CORE_ADDR pc, CORE_
 	}
       else if (IS_FPUSH (inst))
 	{
-	  if (read_register (FPSCR_REGNUM) & FPSCR_SZ)
+	  if (fpscr & FPSCR_SZ)
 	    {
 	      cache->sp_offset += 8;
 	    }
@@ -728,7 +728,7 @@ sh_skip_prologue (CORE_ADDR start_pc)
     return max (pc, start_pc);
 
   cache.sp_offset = -4;
-  pc = sh_analyze_prologue (start_pc, (CORE_ADDR) -1, &cache);
+  pc = sh_analyze_prologue (start_pc, (CORE_ADDR) -1, &cache, 0);
   if (!cache.uses_fp)
     return start_pc;
 
@@ -2360,7 +2360,11 @@ sh_frame_cache (struct frame_info *next_
   cache->pc = frame_func_unwind (next_frame, NORMAL_FRAME);
   current_pc = frame_pc_unwind (next_frame);
   if (cache->pc != 0)
-    sh_analyze_prologue (cache->pc, current_pc, cache);
+    {
+      ULONGEST fpscr;
+      fpscr = frame_unwind_register_unsigned (next_frame, FPSCR_REGNUM);
+      sh_analyze_prologue (cache->pc, current_pc, cache, fpscr);
+    }
 
   if (!cache->uses_fp)
     {
diff -urNp gdb-orig/gdb/v850-tdep.c gdb-head/gdb/v850-tdep.c
--- gdb-orig/gdb/v850-tdep.c	2007-06-04 22:23:28.000000000 +0200
+++ gdb-head/gdb/v850-tdep.c	2007-06-06 18:58:57.364014535 +0200
@@ -456,7 +456,7 @@ v850_is_save_register (int reg)
 
 static CORE_ADDR
 v850_analyze_prologue (CORE_ADDR func_addr, CORE_ADDR pc,
-		       struct v850_frame_cache *pi)
+		       struct v850_frame_cache *pi, ULONGEST ctbp)
 {
   CORE_ADDR prologue_end, current_pc;
   struct pifsr pifsrs[E_NUM_REGS + 1];
@@ -517,7 +517,6 @@ v850_analyze_prologue (CORE_ADDR func_ad
 	}
       else if ((insn & 0xffc0) == 0x0200 && !regsave_func_p)
 	{			/* callt <imm6> */
-	  long ctbp = read_register (E_CTBP_REGNUM);
 	  long adr = ctbp + ((insn & 0x3f) << 1);
 
 	  save_pc = current_pc;
@@ -859,7 +858,11 @@ v850_frame_cache (struct frame_info *nex
   cache->pc = frame_func_unwind (next_frame, NORMAL_FRAME);
   current_pc = frame_pc_unwind (next_frame);
   if (cache->pc != 0)
-    v850_analyze_prologue (cache->pc, current_pc, cache);
+    {
+      ULONGEST ctbp;
+      ctbp = frame_unwind_register_unsigned (next_frame, E_CTBP_REGNUM);
+      v850_analyze_prologue (cache->pc, current_pc, cache, ctbp);
+    }
 
   if (!cache->uses_fp)
     {
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com