Re: RFA: ia64 tdep patch

[Kevin Buettner <kevinb@redhat.com>]

On Oct 20,  5:55pm, J. Johnston wrote:

> 2003-10-20  Jeff Johnston  <jjohnstn@redhat.com>
> 
> 	* ia64-tdep.c: (ia64_frame_cache): Add new prev_cfm field.
[...]
> 	(ia64_sigtramp_frame_init_saved_regs): Bump up base by 16 to get
> 	sp needed for calling lower level

The entry for ia64_sigtramp_frame_init_saved_regs() doesn't describe
what was done there.  AFAICT, the bumping by 16 part of the patch
has been removed.  You still do the following though:

@@ -1869,10 +1913,8 @@ ia64_sigtramp_frame_init_saved_regs (str
 	SIGCONTEXT_REGISTER_ADDRESS (cache->base, IA64_CFM_REGNUM);
       cache->saved_regs[IA64_PSR_REGNUM] = 
 	SIGCONTEXT_REGISTER_ADDRESS (cache->base, IA64_PSR_REGNUM);
-#if 0
       cache->saved_regs[IA64_BSP_REGNUM] = 
-	SIGCONTEXT_REGISTER_ADDRESS (frame->frame, IA64_BSP_REGNUM);
-#endif
+	SIGCONTEXT_REGISTER_ADDRESS (cache->base, IA64_BSP_REGNUM);
       cache->saved_regs[IA64_RNAT_REGNUM] = 
 	SIGCONTEXT_REGISTER_ADDRESS (cache->base, IA64_RNAT_REGNUM);
       cache->saved_regs[IA64_CCV_REGNUM] = 
@@ -1886,9 +1928,8 @@ ia64_sigtramp_frame_init_saved_regs (str
       cache->saved_regs[IA64_LC_REGNUM] = 
 	SIGCONTEXT_REGISTER_ADDRESS (cache->base, IA64_LC_REGNUM);
       for (regno = IA64_GR1_REGNUM; regno <= IA64_GR31_REGNUM; regno++)
-	if (regno != sp_regnum)
-	  cache->saved_regs[regno] =
-	    SIGCONTEXT_REGISTER_ADDRESS (cache->base, regno);
+	cache->saved_regs[regno] =
+	  SIGCONTEXT_REGISTER_ADDRESS (cache->base, regno);
       for (regno = IA64_BR0_REGNUM; regno <= IA64_BR7_REGNUM; regno++)
 	cache->saved_regs[regno] =
 	  SIGCONTEXT_REGISTER_ADDRESS (cache->base, regno);

			......

The code (below) in ia64_sigtramp_frame_prev_register() which computes
PSR doesn't look right to me.  Could you check it?  (If it is right,
please explain it...)

+  else if (regnum == IA64_PSR_REGNUM)
+    {
+      ULONGEST slot_num = 0;
+      CORE_ADDR pc= 0;
+      CORE_ADDR psr = 0;
+      CORE_ADDR addr = cache->saved_regs[IA64_VRAP_REGNUM];
+
+      if (addr != 0)
+	{
+	  *lvalp = lval_memory;
+	  *addrp = addr;
+	  read_memory (addr, buf, register_size (current_gdbarch, IA64_IP_REGNUM));
+	  pc = extract_unsigned_integer (buf, 8);
+	}
+      psr &= ~(3LL << 41);
+      slot_num = pc & 0x3LL;
+      psr |= (CORE_ADDR)slot_num << 41;
+      store_unsigned_integer (valuep, 8, psr);
+    }

			......

Regarding this hunk of code in ia64_sigtramp_frame_prev_register()...

+ else if ((regnum >= IA64_GR32_REGNUM && regnum <= IA64_GR127_REGNUM) ||
+	   (regnum >= V32_REGNUM && regnum <= V127_REGNUM))
+    {
+      CORE_ADDR addr = 0;
+      if (regnum >= V32_REGNUM)
+	regnum = IA64_GR32_REGNUM + (regnum - V32_REGNUM);
+      addr = cache->saved_regs[regnum];
+      if (addr != 0)
+	{
+	  *lvalp = lval_memory;
+	  *addrp = addr;
+	  read_memory (addr, valuep, register_size (current_gdbarch, regnum));
+	}
+    }

Could you add a comment explaining why the normal method of computing
V32 (via ia64_pseudo_register_read()) is inadequate?

Also, I'd prefer to see the following line:

+      if (regnum >= V32_REGNUM)

written as

+      if (regnum >= V32_REGNUM && regnum <= V127_REGNUM)


Hmm... doesn't this hunk of code also need to be concerned with
register renames?  (I.e, the rotating register stuff...)  I'm
wondering why the floating point registers need it, but the
GRs don't.

			......

Regarding...

+  else
+    {
+      CORE_ADDR addr = 0;
+      if (IA64_FR32_REGNUM <= regnum && regnum <= IA64_FR127_REGNUM)
+	{
+	  /* Fetch floating point register rename base from current
+	     frame marker for this frame.  */
+	  int rrb_fr = (cache->cfm >> 25) & 0x7f;
+
+	  /* Adjust the floating point register number to account for
+	     register rotation.  */
+	  regnum = IA64_FR32_REGNUM
+	         + ((regnum - IA64_FR32_REGNUM) + rrb_fr) % 96;
+	}
+
+      /* If we have stored a memory address, access the register.  */
+      addr = cache->saved_regs[regnum];
+      if (addr != 0)
+	{
+	  *lvalp = lval_memory;
+	  *addrp = addr;
+	  read_memory (addr, valuep, register_size (current_gdbarch, regnum));
+	}
+    }

...could you add a comment at the top of the block which says that
it's intended to handle all the other registers (not handled by the
previous clauses), floating point included.  When I first looked at
this, I saw the floating point register rename stuff and figured it
was for just floating point.

Kevin