Re: RFA: ia64 tdep patch

["J. Johnston" <jjohnstn@redhat.com>]

Kevin Buettner wrote:
> 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:
> 

You're looking at my old ChangeLog.  I revised the ChangeLog in the last update.
after cleaning up the patch alot.   The truth was that the cache->base was 
incorrect so I had to adjust it.  The 16 belongs in calculating the base.  The 
previous code for calculating the base was hoping that cache->mem_stack_size 
would be set by examine_prologue(), but that does not occur.  I found I had to 
fudge the value to make it line up with accessing the sigframe.  The new code 
adds the hard-coded 16 to the base and I have added a comment about this.

> @@ -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...)
> 

I'll explain my logic.  As you know, the VRAP address is the return address. 
AFAICT by reading the ABI and insn set, there is no information about what the 
return address is set to when the branch is in slot 0 or 1 (i.e. is the return 
address the next bundle or the next slot?).  The ip register isn't supposed to 
contain the slot number;  it is encoded in the PSR register.  When gdb gets the 
pc value, it forms it by unwinding the PSR and IP registers - getting the slot 
number from the PSR and the IP register address to form a virtual pc address.  I 
did not want to get the slot number wrong if it was encoded in the return 
address so this is why I masked it off above.  The PSR register is only used by 
gdb in unwinding the pc.

> +  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?
> 

I don't know.  I had this for safety reasons already in the 
ia64_frame_prev_register() because I didn't know if it might be called with the 
pseudo register number or not.  This code was copied.  Should it be removed in 
both places?

> Also, I'd prefer to see the following line:
> 
> +      if (regnum >= V32_REGNUM)
> 
> written as
> 
> +      if (regnum >= V32_REGNUM && regnum <= V127_REGNUM)
>

Ok.

> 
> 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.
> 

This code was copied from ia64_frame_prev_register() as it used to be called to 
do the underlying work.

The stuff at the end of examine_prologue() handles rotating GRs for the normal 
case but doesn't for floating point registers.  I would doubt very much that the 
signal trampoline uses rotating registers so I probably should remove it for the
floating-point case.

> 			......
> 
> 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.
>

Yes.

> Kevin
>