Re: [RFA] Resurrect v850

[Corinna Vinschen <vinschen@redhat.com>]

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?

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

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


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat, Inc.