Re: RFA: ia64 tdep patch

["J. Johnston" <jjohnstn at redhat dot com>]

J. Johnston wrote:
> Kevin Buettner wrote:
>
> > On Oct 21,  7:03pm, J. Johnston wrote:
> >
> >
> > > You're looking at my old ChangeLog.
> >
> >
> >
> > I've looked at your most recent patch, but do not see the new ChangeLog
> > entry.  Would you mind posting it?
>
> 2003-10-20  Jeff Johnston  <jjohnstn@redhat.com>
>
>     * ia64-tdep.c: (ia64_frame_cache): Add new prev_cfm field.
>     (ia64_alloc_frame_cache):  Initialize new prev_cfm field to 0.
>     (floatformat_valid): New static routine.
>     (floatformat_ia64_ext): Add name field and set up is_valid routine
>     to floatformat_valid().
>     (examine_prologue):  For the previous cfm, use frame_unwind_register()
>     if the cfm is not stored in a register-stack register.  Save the
>     previous cfm value in the prev_cfm field.  Add debug output.
>     (ia64_frame_this_id): Use frame_id_build_special() to also register
>     the bsp.  Add debug output.
>     (ia64_sigtramp_frame_this_id): Ditto.
>     (ia64_frame_prev_register):  Look at cache saved_regs for a few more
>     registers and also add some checks for framelessness before accepting
>     current register values for fields such as return address.  For cfm,
>     use the cached prev_cfm field if available.  Add debug output.
>     (ia64_sigtramp_frame_init_saved_regs): Bump up base by 16 to get
>     sp needed for calling lower level
>     ia64_linux_sigcontext_register_address().  Also save the
>     bsp and sp address as part of initialization.
>     (ia64_sigtramp_frame_cache): Hard-code stack size as it can't be
>     calculated.  Cache the bsp and cfm values.
>     (ia64_sigtramp_frame_prev_register): Flesh this routine out instead of
>     using ia64_frame_prev_register().  The saved values for bsp and sp can
>     be taken from the cache.  Add debug output.
>     (ia64_push_dummy_call): Use frame_id_build_special() to also register
>     the bsp.
>
>
> > > > 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.
> >
> >
> >
> > Thanks for the explanation.  Could you please add a brief comment to the
> > code.
>
> Will do.

Actually, you were right, I was wrong.  The VRAP for sigtramp is the IP register 
so it won't have the PSR info in it.  The sigcontext stores the PSR so we don't 
have to reconstruct it in this case.  I have removed the code.

> > > > 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?
> >
> >
> >
> > I don't know.  I've been studying the code and am wondering why the
> > V32 ... V127 pseudo regs were introduced at all.  Could you remind
> > me of the reason?
>
> They are needed because r32 to r127 are not accessible via the PTRACE 
> interface.  They are accessed via the bsp.  Without flagging them as 
> pseudo-registers, the regcache code returns 0 for all these registers.
>
> I tell the dwarf to convert references to r32-r127 to be V32-V127 in 
> ia64_dwarf_reg_to_regnum().  I would guess that a parameter reference in 
> a previous frame would cause this conversion to occur.

I have updated the comment for the pseudo-register enums regarding the fact that 
r32 - r127 are not accessible via the ptrace register get/set interfaces.

> > > > 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.
> >
> >
> >
> > I think you're probably right.
> >
> > It'd be nice though if we could arrange for the code which handles
> > rotating registers for floating point and general regs to appear
> > in the same function.  (This can happen in a different patch though.)
> >
> > Kevin