This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Save the length of inserted breakpoints
> Date: Thu, 13 Apr 2006 14:57:27 -0700
> From: Michael Snyder <msnyder@redhat.com>
>
> Daniel Jacobowitz wrote:
> > On Wed, Apr 12, 2006 at 08:37:22PM +0200, Mark Kettenis wrote:
> >
> >>>Would a new "struct bp_target_info", defined and allocated centrally
> >>>for convenience, allay this concern? [Conveniently I can do the bulk
> >>>of the changes for that with sed :-)]
> >>
> >>Why hide things away if all you're going to need is a buffer and a
> >>length? I've tried really hard to see why one would need more than
> >>that, but failed completely.
> >>
> >>So I think we should have:
> >>
> >>int target_insert_breakpoint(CORE_ADDR addr, gdb_byte *buf, int *size);
> >>int target_remove_breakpoint(CORE_ADDR addr, gdb_byte *buf, int size);
> >
> >
> > And then if you come up with a reason, you're going to need to hand
> > edit every one of these targets again. It's not a bundle of fun. Is
> > that really necessary?
> >
> > You need an address because the address at which the breakpoint is
> > inserted may not match the requested address. This happens in several
> > different places in the breakpoint infrastructure (I believe I counted
> > three disjoint hooks for it), but I am particularly looking at
> > BREAKPOINT_FROM_PC, which takes the PC by reference. In the ARM case,
> > given 0x4001, it strips the low bit off and returns a two byte
> > breakpoint. If we don't allow the target to save the
> > actually-inserted-at address, then it has to call BREAKPOINT_FROM_PC
> > again. It feels much more robust to me to save this address when we
> > initially adjust it. Here's where we inserted the breakpoint, so
> > that's where we should remove it from.
> >
> > I can think of plenty of other places where another constant might
> > be useful. You might want to record which hardware breakpoint
> > registers were used, for instance, instead of digging around
> > to figure out which ones to clear. Adding a new member to
> > "struct bp_target" for that would be easy.
>
> I haven't followed this discussion closely, so forgive me
> if I'm recapitulating something that's already been said.
>
> What about something like "void *target_data" in the breakpoint struct?
> The target can add whatever it likes, and the core breakpoint code
> doesn't need to know what it is. If it's non-null when the bp is
> freed, then the target should be given an opportunity to delete it.
And I presume only TARGET_DATA would be passed down to the target.
That's certainly better than passing down "struct bp_location" to the
target, exposing the internals of the high-level breakpoint code to
the low-level target code.
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.
Mark