Re: Save the length of inserted breakpoints

[Daniel Jacobowitz <drow at false dot org>]

On Fri, Mar 03, 2006 at 12:18:44AM +0100, Mark Kettenis wrote:
> > > Yuck!  It really is ugly.  For one thing, I think it is a bit
> > > pointless, to add a the BREAKPOINT_FROM_PC() to targets where we know
> > > the length of a breakpoint instruction is fixed.

Several of the touched targets either have variable length breakpoints,
or conceivably could in an architecture revision (e.g. I'm pretty sure
there's such an extension for PPC on paper, don't know if it's deployed
anywhere).  I felt it better to be consistent.

> > > Another thing is that I think the order of the arguments of
> > > target_remove_breakpoint() is wrong.  I think it makes sense to see
> > > your "len" argument as the length of the saved memory.  Then it is
> > > more logical to make "len" the last argument of
> > > target_remove_breakpoint().
> > > 
> > > However, doesn't it make more sense to have target_insert_breakpoint()
> > > save the length instead of using BREAKPOINT_FROM_PC() to ask for it?
> > 
> > If you want me to do that, I'll do that instead.  It requires touching
> > twice as many target functions.  Writing the changelog for this one
> > took long enough, so forgive me if I wait a while before trying it
> > again :-)
> 
> You're touching a fairly fundamental piece of the breakpoint
> infrastructure here.  I think it is worth thinking about this for a
> bit longer.  My comments certainly weren't "demands", so I'm perfectly
> fine with discussing this a bit more before you rush towards changing
> your patch ;-).

Nah, I agree that making it symmetric would be good.  The problem is
that this entire area is riddled with hacks, duplicate ways to
accomplish the same goal, and other forms of quirk; I didn't want to
get too far into cleaning it up, especially since a third of the files
I touched are for targets that probably ought to be deleted anyway.

-- 
Daniel Jacobowitz
CodeSourcery