Re: Save the length of inserted breakpoints

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Sun, 16 Apr 2006 21:41:33 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Mon, Apr 17, 2006 at 01:49:23AM +0200, Mark Kettenis wrote:
> > However, my point is that we're creating needless abstraction layers
> > here, and opening the way for potentially big changes in the target
> > interface.  We currently have a well defined interface that inserts a
> > software breakpoint at a specific address, and saves the contents into
> > a buffer provided by the high-level code.  That interface has a
> > problem in the sense that it doesn't record actually how many valid
> > bytes are in that buffer.  This simplest fix is simply adding that
> > missing length to the interface.
> 
> Have you read any of my arguments as to why the abstraction is not
> needless?  I haven't seen a response to any of them, just a dismissal.
> I think I gave some useful examples; I'm a little frustrated that you
> keep ignoring them.

Sorry.  Came home to a flurry of messages about the subject and
semi-randomly chose to reply to some of them.  Citing from your
earlier message:

> I do not think the abstraction is at all unnecessary, or I wouldn't
> have added it.  I presented three reasons why it could be useful:
> one practical concern with the address modifications performed by
> the existing BREAKPOINT_FROM_PC interface, one potential use case
> for hardware breakpoints, and the considerable simplification of
> future changes to these specific functions.

I don't think the hardware breakpoints should be a concern here.  But
the BREAKPOINT_FROM_PC issue is indeed pretty much a showstopper.

> The interface is not so well defined as your paragraph suggests. In
> fact, the target MAY save the contents, not DOES.  I'm pretty sure that
> some don't.  I let this mislead me into thinking that saving the
> contents was optional.  In fact it isn't.  deprecated_read_memory_nobpt
> uses the shadow contents, and uses BREAKPOINT_FROM_PC.  Whatever we
> decide on, I'll make sure that gets fixed.

But this means that actually we should try to make the interface
stricter, instead of looser.  And the fact that the shadow contents
are needed makes it impossible to use an opaque struct.  That in
combination with the possibility that BREAKPOINT_FROM_PC adjusts the
breakpoint address means that besides the length we also need to pass
back the address.  At that point indeed passing a struct is perhaps a
better option.  And we should remove the usage of BREAKPOINT_FROM_PC
from deprecated_read_memory_nobpt() altogether.

I still think we should maintain a strict seperation between the
high-level breakpoint code and the low-level target code.  So I'd
really appreciate it if you'd be willing to change your patch such
that instead of struct bp_location, you used a different struct, which
for now would have three members: the saved contents, the length and
the address.  It's perfectly ok with me to make that new struct part
of struct bp_location to avoid all memory allocation problems.

Mark