Re: [RFA] Resurrect v850

[Daniel Jacobowitz <drow@false.org>]

On Tue, May 17, 2005 at 03:23:48PM +0200, Corinna Vinschen wrote:
> Hi Daniel,
> 
> On May 15 13:44, Daniel Jacobowitz wrote:
> > On Fri, May 13, 2005 at 01:40:16PM +0200, Corinna Vinschen wrote:
> > > Hi,
> > > 
> > > the below patch resurrects v850.  No deprecated mechanisms are used.
> > > As promised, this target now uses trad_frames ;-)
> > > 
> > > Ok to apply?
> > 
> > Some comments...
> 
> While reworking the code according to your comments, I came across three
> problems:
> 
> > > +enum {
> > > + E_R0_REGNUM,
> > > + E_R1_REGNUM,
> > > + E_R2_REGNUM, E_SAVE1_START_REGNUM = E_R2_REGNUM, E_SAVE1_END_REGNUM = E_R2_REGNUM,
> > > + E_R3_REGNUM, E_SP_REGNUM = E_R3_REGNUM,
> > 
> > Several of the lines in this list are too long.  I do see how you were
> > trying to organize it, but if you put one to a line the lines with
> > equality operators will still stand out.
> 
> Is a layout like this:
> 
>   E_R1_REGNUM,
>   E_R2_REGNUM, E_SAVE1_START_REGNUM = E_R2_REGNUM,
>                E_SAVE1_END_REGNUM = E_R2_REGNUM,
>   E_R3_REGNUM, E_SP_REGNUM = E_R3_REGNUM,
>   [...]
> 
> ok?

I was originally going to suggest that - but no, it isn't. 
gdb_indent.sh will promptly undo this the next time someone needs to
correct indentation in the file.  That's the problem with
machine-enforced standards...

> 
> > The comments in this function are seriously un-encouraging.  Is this
> > excess space necessary or not?  If it is, why?
> 
> I don't know.  The ABI is defined this way and inspecting a lot of code
> I didn't see any reason for this behaviour.  Nevertheless, gcc emits
> code which allocates these 16 byte for no apparent reason.  I removed
> useless comments and the remaining comment now read like this:
> 
>   /* The offset onto the stack at which we will start copying parameters
>      (after the registers are used up) begins at 16 rather than at zero.
>      That's how the ABI is defined, though there's no indication that these
>      16 bytes are used for anything, not even for saving incoming
>      argument registers.  */
> 
> Is that ok?

Yes, definitely.


> 
> > > +static void
> > > +v850_frame_prev_register (struct frame_info *next_frame, void **this_cache,
> > > +			  int regnum, int *optimizedp,
> > > +			  enum lval_type *lvalp, CORE_ADDR *addrp,
> > > +			  int *realnump, void *valuep)
> > > +{
> > > +  struct v850_frame_cache *cache = v850_frame_cache (next_frame, this_cache);
> > > +
> > > +  gdb_assert (regnum >= 0);
> > > +
> > > +  /* The PC of the previous frame is stored in the PR register of
> > > +     the current frame.  Frob regnum so that we pull the value from
> > > +     the correct place.  */
> > > +  if (regnum == E_PC_REGNUM)
> > > +    regnum = E_LP_REGNUM;
> > > +
> > > +  trad_frame_get_prev_register (next_frame, cache->saved_regs, regnum,
> > > +				optimizedp, lvalp, addrp, realnump, valuep);
> > > +}
> > 
> > You can do the frobbing when you record the saved registers - at the
> > end, copy the saved location.
> 
> Sorry, I don't understand this.  Can you show me some (pseudo) code what
> you mean?

I believe that you can do "cache->saved_regs[E_PC_REGNUM] =
cache->saved_regs[E_LP_REGNUM]", right by the call to
trad_frame_addr_p.  Does that work?

-- 
Daniel Jacobowitz
CodeSourcery, LLC