Re: [RFA] Resurrect v850

Daniel Jacobowitz
2005-05-17T13:32:16Z

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