Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> > One issue that I'm not sure how to address is that there are several
> > places breakpoints are inserted where a breakpoint has not been
> > constructed.  Most of these occur in tdep code which implements single
> > step with breakpoints on processors without a trace mode.
>
> Aiee! Such code is evil and must be destroyed.

It seems to me that this single_step function is actually not in use.
I don't know about WindRiver, but here we have two targets: an ISS and a
remote target that use the generic breakpoint functions before stepping.
I don't mind to remove this code.

>
> One important value of the full breakpoint machinery is that it can help
> avoid the same location being patched twice. Any time we patch the same
> instruction twice, we must un-patch it in exactly reverse order or else
> we leave a breakpoint instruction sitting in the code -- Not Good.
>
> So I would have to argue that the singlestep logic must either happen at a
> really low level (this guarantees it will patch last and un-patch first) or
> must go through the real breakpoint logic which can do duplicate detection.
>

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:

jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

Stephane> It seems to me that this single_step function is actually
Stephane> not in use.  I don't know about WindRiver, but here we have
Stephane> two targets: an ISS and a remote target that use the generic
Stephane> breakpoint functions before stepping.  I don't mind to
Stephane> remove this code.

I assume you are refering to the ARC port?

I see three GDB ports that do single step in software: arc, rs6000,
and sparc.  The arc and sparc ports, insert breakpoints with target-
_insert_breakpoint().  The rs6000 port by reading and writing memory
directly.  Of the two methods, I prefer target_insert_breakpoint().

However, if the breakpoint insert/remove API was changed to require a
struct breakpoint pointer, we couldn't call target_insert_breakpoint()
in the foo_single_step() functions without creating a real breakpoint
object to pass through that first pointer.   

Another reason we might want a real breakpoint for this is that when
my memory region attribute code is complete we'll be able to say that
breakpoints in a region should be done with hardware breakpoints.  A
user can debug his/her own code in read-only memory by using hbreak,
but there is no way to tell GDB to use hardware breakpoints for step,
next, continue, finish, until, etc.  One thing that surprised me was
that the single step code used multiple breakpoints.  That would put a
lot of pressure on hardware breakpoint registers.  I suspect that GDB
has all the information in order to insert a single breakpoint (by
evaluating which way a conditional branch will go, etc.).


Since you say you can remove the single step code, I assume that the
ISS target and remote protocol agents can perform the single step by
themselves?  If so, it would be advantageous to disable GDB's single
step support.  GDB would then issue a single "step" command instead of
"insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
latency of a step should be greatly improved.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:

jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

Stephane> It seems to me that this single_step function is actually
Stephane> not in use.  I don't know about WindRiver, but here we have
Stephane> two targets: an ISS and a remote target that use the generic
Stephane> breakpoint functions before stepping.  I don't mind to
Stephane> remove this code.

I assume you are refering to the ARC port?

I see three GDB ports that do single step in software: arc, rs6000,
and sparc.  The arc and sparc ports, insert breakpoints with target-
_insert_breakpoint().  The rs6000 port by reading and writing memory
directly.  Of the two methods, I prefer target_insert_breakpoint().

However, if the breakpoint insert/remove API was changed to require a
struct breakpoint pointer, we couldn't call target_insert_breakpoint()
in the foo_single_step() functions without creating a real breakpoint
object to pass through that first pointer.   

Another reason we might want a real breakpoint for this is that when
my memory region attribute code is complete we'll be able to say that
breakpoints in a region should be done with hardware breakpoints.  A
user can debug his/her own code in read-only memory by using hbreak,
but there is no way to tell GDB to use hardware breakpoints for step,
next, continue, finish, until, etc.  One thing that surprised me was
that the single step code used multiple breakpoints.  That would put a
lot of pressure on hardware breakpoint registers.  I suspect that GDB
has all the information in order to insert a single breakpoint (by
evaluating which way a conditional branch will go, etc.).


Since you say you can remove the single step code, I assume that the
ISS target and remote protocol agents can perform the single step by
themselves?  If so, it would be advantageous to disable GDB's single
step support.  GDB would then issue a single "step" command instead of
"insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
latency of a step should be greatly improved.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> > One issue that I'm not sure how to address is that there are several
> > places breakpoints are inserted where a breakpoint has not been
> > constructed.  Most of these occur in tdep code which implements single
> > step with breakpoints on processors without a trace mode.
>
> Aiee! Such code is evil and must be destroyed.

It seems to me that this single_step function is actually not in use.
I don't know about WindRiver, but here we have two targets: an ISS and a
remote target that use the generic breakpoint functions before stepping.
I don't mind to remove this code.

>
> One important value of the full breakpoint machinery is that it can help
> avoid the same location being patched twice. Any time we patch the same
> instruction twice, we must un-patch it in exactly reverse order or else
> we leave a breakpoint instruction sitting in the code -- Not Good.
>
> So I would have to argue that the singlestep logic must either happen at a
> really low level (this guarantees it will patch last and un-patch first) or
> must go through the real breakpoint logic which can do duplicate detection.
>

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Jim Kingdon]

> I noticed ARM RDI target also has breakpoints with "handles" and
> maintains a local database mapping handles to breakpoint.  I'm
> pleased to see this, because this change could be taken advantage by
> a target already in the tree.

So you'd add a void* to the struct breakpoint for the target to do
with what it wants (together with a new deallocation function in the
target vector which delete_breakpoint would call)?

> My current thinking about how to handle these cases is to create a new
> breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
> create "real" breakpoints.  

Well, I can't quite resist mentioning the contrary idea which has been
floated which is to put all the single stepping stuff down into the
wait/resume level (I guess a new level between the existing
handle_inferior_event and friends, and the target vector).  Having
mentioned it I'm not sure an extra level is really warranted here.

So bp_singlestep might work nicely too - of course it would be added
to the list of types which insert_breakpoints doesn't mess with.  And
this has a further consequence - that you can't just detect this as a
normal breakpoint, you need to keep the
singlestep_breakpoints_inserted_p machinery in infrun.c

I realize I haven't really put much of a recommendation in there, but
but it sounds like going ahead with bp_singlestep might be the best
plan.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> >>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
>
> jtc> One issue that I'm not sure how to address is that there are several
> jtc> places breakpoints are inserted where a breakpoint has not been
> jtc> constructed.  Most of these occur in tdep code which implements single
> jtc> step with breakpoints on processors without a trace mode.
>
> Todd> Aiee! Such code is evil and must be destroyed.
>
> Stephane> It seems to me that this single_step function is actually
> Stephane> not in use.  I don't know about WindRiver, but here we have
> Stephane> two targets: an ISS and a remote target that use the generic
> Stephane> breakpoint functions before stepping.  I don't mind to
> Stephane> remove this code.
>
> I assume you are refering to the ARC port?

yes. But I was actually wrong in my last email, the single step function in
arc-tdep is
used, I was thinking to another function, sorry.
What do you mean by breakpoint without a trace mode?

>
> I see three GDB ports that do single step in software: arc, rs6000,
> and sparc.  The arc and sparc ports, insert breakpoints with target-
> _insert_breakpoint().  The rs6000 port by reading and writing memory
> directly.  Of the two methods, I prefer target_insert_breakpoint().
>
> However, if the breakpoint insert/remove API was changed to require a
> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
> in the foo_single_step() functions without creating a real breakpoint
> object to pass through that first pointer.

I don't know what are the foo_single_step functions.

I also use the breakpoint structure as an extern variable. I needed
it to implement the set_hw_breakpoint routine for the remote protocol.
I think it's not the nicer way to do but ....

> Another reason we might want a real breakpoint for this is that when
> my memory region attribute code is complete we'll be able to say that
> breakpoints in a region should be done with hardware breakpoints.
> A user can debug his/her own code in read-only memory by using hbreak,
> but there is no way to tell GDB to use hardware breakpoints for step,
> next, continue, finish, until, etc.  One thing that surprised me was
> that the single step code used multiple breakpoints.  That would put a
> lot of pressure on hardware breakpoint registers.  I suspect that GDB
> has all the information in order to insert a single breakpoint (by
> evaluating which way a conditional branch will go, etc.).
>

What would be a "real breakpoint"? a pointer to the breakpoint structure?

>
> Since you say you can remove the single step code, I assume that the
> ISS target and remote protocol agents can perform the single step by
> themselves?  If so, it would be advantageous to disable GDB's single
> step support.  GDB would then issue a single "step" command instead of
> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> latency of a step should be greatly improved.

I will implement this for the remote target only since the hardware supports
single-stepping.
I'm not sure if this feasible in the ISS - Yuri?

If not feasible I won't disable the GDB's single step support (thus enabling a
call to single_step())
but I will implement a single_step call to gdbstub in remote_resume().

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
> jtc> I was planning to propose that the breakpoint pointer itself be passed
> jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
> jtc> is as good of time as any.
> 
> Todd> I say Just Do It. I am sitting on some local code here that tracks
> Todd> breakpoints added to the target by a third party, and I ended up needing
> Todd> the breakpoint shadow field to be available to those functions.
> 
> I spent some time yesterday investigating what would be necessary to
> change the target_{insert,remove}_breakpoint() API to pass a pointer
> to struct breakpoint.
> 
> What I have so far is change the API from:
>         int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
> to:
>         int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

J.T.,

One aspect of this gives me cold feet and sweety palms.  You're giving
the target code access to the entire bp struct.  While I don't have any
problems with handing the code a breakpoint handle, I have strong
reservations towards any moves that give the target unfettered access to
the entire ``struct breakpoint''.  We'll be spending the next 10 years
trying to get control back again :-)

I'd prefer to see something that tightens rather than loosens access to
``struct breakpoint''.  Perhaphs something along the lines of multi-arch
where the target is notified of breakpoint create, insert, remove,
delete operations.

	Andrew

breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> I was planning to propose that the breakpoint pointer itself be passed
jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
jtc> is as good of time as any.  

Todd> I say Just Do It. I am sitting on some local code here that tracks
Todd> breakpoints added to the target by a third party, and I ended up needing
Todd> the breakpoint shadow field to be available to those functions.

I spent some time yesterday investigating what would be necessary to
change the target_{insert,remove}_breakpoint() API to pass a pointer
to struct breakpoint.

What I have so far is change the API from:
        int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
to:
        int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

I would have prefered to pass only the breakpoint pointer, but the
overlay support code in breakpoint.c calls target_insert_breakpoint()
twice with different addresses; once for the VMA and once for the LMA.
I don't want to address that, at least not yet.

For the most part, this change was just tedious grunt work.  All those
targets to change...  However, while I was editing my way through GDBs
files, I noticed ARM RDI target also has breakpoints with "handles"
and maintains a local database mapping handles to breakpoint.  I'm
pleased to see this, because this change could be taken advantage by a
target already in the tree.

One issue that I'm not sure how to address is that there are several
places breakpoints are inserted where a breakpoint has not been
constructed.  Most of these occur in tdep code which implements single
step with breakpoints on processors without a trace mode.

For example, from arc-tdep.c:
 
      /* Always set a breakpoint for the insn after the branch.  */
      next_pc = pc + ((type == NORMAL8 || type == BRANCH8) ? 8 : 4);
      target_insert_breakpoint (next_pc, break_mem[0]);

My current thinking about how to handle these cases is to create a new
breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
create "real" breakpoints.  

I'd appreciate if anyone can share their thoughts on this matter.

        --jtc
   
-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

>
> Stephane> I also use the breakpoint structure as an extern variable. I
> Stephane> needed it to implement the set_hw_breakpoint routine for the
> Stephane> remote protocol.  I think it's not the nicer way to do but
> Stephane> ....
>
> Can you explain?  I don't see any use of struct breakpoint in the
> current arc-tdep.c, nor do I see a set_hw_breakpoint function?

The version at FSF is a very old version out of date.
I am currently extending gdb for ARC to support user extensions.

>
> >> Since you say you can remove the single step code, I assume that the
> >> ISS target and remote protocol agents can perform the single step by
> >> themselves?  If so, it would be advantageous to disable GDB's single
> >> step support.  GDB would then issue a single "step" command instead of
> >> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> >> latency of a step should be greatly improved.
>
> Stephane> I will implement this for the remote target only since the
> Stephane> hardware supports single-stepping.  I'm not sure if this
> Stephane> feasible in the ISS - Yuri?
>
> Stephane> If not feasible I won't disable the GDB's single step
> Stephane> support (thus enabling a call to single_step()) but I will
> Stephane> implement a single_step call to gdbstub in remote_resume().
>
> If your ISS target cannot support single step, but the remote protocol
> can, perhaps the best thing is to make software single step a target
> specific option.

By creating a target specific command?
The single step mechanism is a not a step-over-calls mechanism.

>
> I don't think that the arc is the only processor that would benefit
> from such a change.  For example, the sparclet ROM monitor supports a
> single step command, but I do not know if it is ever issued because
> the SPARC target uses software single step.  Perhaps it inserts the
> trap instructions needed for single step and issues the monitor step
> command?
>
> I do not think it's possible for any change to remote_resume() to be
> the right solution.
>
You're right, I've tried and if does not work.
Thanks for your explanations anyway.

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
Andrew> One aspect of this gives me cold feet and sweety palms.
Andrew> You're giving the target code access to the entire bp struct.
Andrew> While I don't have any problems with handing the code a
Andrew> breakpoint handle, I have strong reservations towards any
Andrew> moves that give the target unfettered access to the entire
Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
Andrew> trying to get control back again :-)

I appreciate this argument, and agree that the target interface should
be as robust as possible.  After reading your message, I thought about
hiding the fact that the argument is a struct breakpoint pointer and
providing macros to access "public" fields, but any subversion we do
can be undone by a clever programmer.  I'm still working on something 
that is simple, elegant, and efficent.

However, I think it's a bit of a stretch to say such changes in and of
themselves will lead to GDB being out of control.  IMHO, this can only
happen if the GDB maintainers fail to do their jobs and integrate code
that disrupts GDB's architectural integrity.  I'll stipulate that
we've done a rather poor job of this in the past, but I'm hopeful that
attitudes have changed.

Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

I don't quite follow.  Care to elaborate?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

I don't think it's that bad.  

Todd> One important value of the full breakpoint machinery is that it
Todd> can help avoid the same location being patched twice. Any time
Todd> we patch the same instruction twice, we must un-patch it in
Todd> exactly reverse order or else we leave a breakpoint instruction
Todd> sitting in the code -- Not Good.

Todd> So I would have to argue that the singlestep logic must either
Todd> happen at a really low level (this guarantees it will patch last
Todd> and un-patch first) or must go through the real breakpoint logic
Todd> which can do duplicate detection.

The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
it inserts and remove trap instructions without effecting GDB's high-
level breakpoints.  So I think we're OK.  As a result, I wouldn't be
suprised if steping into a breakpoint didn't behave the same as on a
machine with hardware single-step.  

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One issue that I'm not sure how to address is that there are several
> places breakpoints are inserted where a breakpoint has not been
> constructed.  Most of these occur in tdep code which implements single
> step with breakpoints on processors without a trace mode.

Aiee! Such code is evil and must be destroyed.

One important value of the full breakpoint machinery is that it can help
avoid the same location being patched twice. Any time we patch the same
instruction twice, we must un-patch it in exactly reverse order or else
we leave a breakpoint instruction sitting in the code -- Not Good.

So I would have to argue that the singlestep logic must either happen at a
really low level (this guarantees it will patch last and un-patch first) or
must go through the real breakpoint logic which can do duplicate detection.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> The reason I was passing the breakpoint pointer (with the intention of
> adding a "void *private_data" field), is that I don't want to preclude
> any target implementation.  A 32 bit integer ID is probably the most
> common breakpoint handle, but you never can tell...

I didn't pretend that this was general enough for everyone, I only said
this was what I used. In my case the UINT32 is dictated by the WTX layer
that assigns the ID's.

> I don't think your is_foreign flag is suitable for the target specific
> data, because it changes the behavior of generic breakpoint code.

Hmm, actually yeah that's right. It probably should be generic.

> You didn't describe is_global, so I'm not sure about it.  How is it
> different from (bp->thread == -1)?

It probably isn't, and is more an artifact of my implementation.

The foreign/global stuff was added together, and happened late enough
in a development cycle that I needed to keep it isolated and easy to
compile back out if we had to cancel it. I had two distinct #ifdef's
so I could pinpoint all the changes; I've never taken those out.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
> Andrew> One aspect of this gives me cold feet and sweety palms.
> Andrew> You're giving the target code access to the entire bp struct.
> Andrew> While I don't have any problems with handing the code a
> Andrew> breakpoint handle, I have strong reservations towards any
> Andrew> moves that give the target unfettered access to the entire
> Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
> Andrew> trying to get control back again :-)
> 
> I appreciate this argument, and agree that the target interface should
> be as robust as possible.  After reading your message, I thought about
> hiding the fact that the argument is a struct breakpoint pointer and
> providing macros to access "public" fields, but any subversion we do
> can be undone by a clever programmer.  I'm still working on something
> that is simple, elegant, and efficent.

In the case of gdbarch, everything is manipulated via methods.  Each
legacy macro is backed by a C function.  The programmer does not have
direct access to the gdbarch object.

GDBARCH set a very very high standard (possibly too high).  However,
whats interesting is the problems so far identified have related to
things like defaults and not to the actual interface.

> However, I think it's a bit of a stretch to say such changes in and of
> themselves will lead to GDB being out of control.  IMHO, this can only
> happen if the GDB maintainers fail to do their jobs and integrate code
> that disrupts GDB's architectural integrity.  I'll stipulate that
> we've done a rather poor job of this in the past, but I'm hopeful that
> attitudes have changed.

I'm looking at it from the view of what is involved in making a change
to ``struct breakpoint''.  If targets directly depend on random bits
than that change becomes that much harder.

If target code can poke around with ``struct breakpoint'' internals then
there is that extra bit of pressure to let through that patch which
solves the problem that everyone is concerned about but no one has a
clean fix for.

> Andrew> I'd prefer to see something that tightens rather than loosens
> Andrew> access to ``struct breakpoint''.  Perhaphs something along the
> Andrew> lines of multi-arch where the target is notified of breakpoint
> Andrew> create, insert, remove, delete operations.

Possibly a side issue.  When a new architecture is created.  gdbarch
notifies all interested parties so that they can update their local
architecture specific information.  See gdbtypes.c for a very good
example.

If the target wants to know about breakpoint changes then notify it
(mumble something about the observer pattern).

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Richard Earnshaw]

> "J.T. Conklin" wrote:
> 
> > Todd> So I would have to argue that the singlestep logic must either
> > Todd> happen at a really low level (this guarantees it will patch last
> > Todd> and un-patch first) or must go through the real breakpoint logic
> > Todd> which can do duplicate detection.
> > 
> > The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> > it inserts and remove trap instructions without effecting GDB's high-
> > level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> > suprised if steping into a breakpoint didn't behave the same as on a
> > machine with hardware single-step.
> 
> Its got long-term problems.
> 
> The code assumes that it is going to retain control, blocked on the
> target, while the actual step is performed.
> This doesn't work well within a pure event model where control is ment
> to return to the event-loop when ever the target is resumed.

It doesn't work properly with INSTRUCTION_NULLIFIED either -- at least it 
didn't last time I tried it...

R.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:

> Todd> So I would have to argue that the singlestep logic must either
> Todd> happen at a really low level (this guarantees it will patch last
> Todd> and un-patch first) or must go through the real breakpoint logic
> Todd> which can do duplicate detection.
> 
> The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> it inserts and remove trap instructions without effecting GDB's high-
> level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> suprised if steping into a breakpoint didn't behave the same as on a
> machine with hardware single-step.

Its got long-term problems.

The code assumes that it is going to retain control, blocked on the
target, while the actual step is performed.
This doesn't work well within a pure event model where control is ment
to return to the event-loop when ever the target is resumed.

	enjoy,
		Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
Stephane> What do you mean by breakpoint without a trace mode?

Processors that support hardware single step often call it instruction
trace mode or something similar.  Processors without it have to resort
to inserting trap/breakpoint/invalid instructions.

>> However, if the breakpoint insert/remove API was changed to require a
>> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
>> in the foo_single_step() functions without creating a real breakpoint
>> object to pass through that first pointer.

Stephane> I don't know what are the foo_single_step functions.

I was attempting to refer to those functions that implement single step
on all targets: arc_software_single_step(), rs6000_software_single_step,
and sparc_software_single_step().  It might have made more sense if I 
didn't forget "software_" and called it foo_software_single_step() or
maybe *_software_single_step().

Stephane> I also use the breakpoint structure as an extern variable. I
Stephane> needed it to implement the set_hw_breakpoint routine for the
Stephane> remote protocol.  I think it's not the nicer way to do but
Stephane> ....

Can you explain?  I don't see any use of struct breakpoint in the
current arc-tdep.c, nor do I see a set_hw_breakpoint function?

>> Another reason we might want a real breakpoint for this is that when
>> my memory region attribute code is complete we'll be able to say that
>> breakpoints in a region should be done with hardware breakpoints.
>> A user can debug his/her own code in read-only memory by using hbreak,
>> but there is no way to tell GDB to use hardware breakpoints for step,
>> next, continue, finish, until, etc.  One thing that surprised me was
>> that the single step code used multiple breakpoints.  That would put a
>> lot of pressure on hardware breakpoint registers.  I suspect that GDB
>> has all the information in order to insert a single breakpoint (by
>> evaluating which way a conditional branch will go, etc.).
>> 

Stephane> What would be a "real breakpoint"? a pointer to the
Stephane> breakpoint structure?

Yes, it's a a pointer to a breakpoint struct, but it's more than that.
I mean a breakpoint created by one of the breakpoint creation functions 
in breakpoint.c.

>> Since you say you can remove the single step code, I assume that the
>> ISS target and remote protocol agents can perform the single step by
>> themselves?  If so, it would be advantageous to disable GDB's single
>> step support.  GDB would then issue a single "step" command instead of
>> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
>> latency of a step should be greatly improved.

Stephane> I will implement this for the remote target only since the
Stephane> hardware supports single-stepping.  I'm not sure if this
Stephane> feasible in the ISS - Yuri?

Stephane> If not feasible I won't disable the GDB's single step
Stephane> support (thus enabling a call to single_step()) but I will
Stephane> implement a single_step call to gdbstub in remote_resume().

If your ISS target cannot support single step, but the remote protocol
can, perhaps the best thing is to make software single step a target
specific option.  

I don't think that the arc is the only processor that would benefit
from such a change.  For example, the sparclet ROM monitor supports a
single step command, but I do not know if it is ever issued because
the SPARC target uses software single step.  Perhaps it inserts the
trap instructions needed for single step and issues the monitor step
command?

I do not think it's possible for any change to remote_resume() to be
the right solution.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One aspect of this gives me cold feet and sweety palms.  You're giving
> the target code access to the entire bp struct.  While I don't have any
> problems with handing the code a breakpoint handle, I have strong
> reservations towards any moves that give the target unfettered access to
> the entire ``struct breakpoint''.  We'll be spending the next 10 years
> trying to get control back again :-)
...
> I'd prefer to see something that tightens rather than loosens access to
> ``struct breakpoint''.  Perhaphs something along the lines of multi-arch
> where the target is notified of breakpoint create, insert, remove,
> delete operations.

Basically what I need is the moral equivalent of a "user" field tacked
onto every breakpoint struct.

Based on my existing code, the contents of this field would be:

	UINT32	breakpointID;	/* WTX-supplied handle for this breakpoint */
	bool	is_foreign;	/* true if breakpoint was set by someone else */
	bool	is_global;	/* true if breakpoint active for all tasks */

The is_foreign field is an interesting concept which I don't think anyone
else has implemented; it signifies a breakpoint that GDB does not "own".
One of the events I can receive from the target is "foreign breakpoint
created" and I find a safe time to call "break *<address>" in order to
let the breakpoint show up properly in GDB.

Foreign breakpoints look like normal breakpoints to GDB users except that
if they aren't recreated (new breakpoint struct allocated) by GDB commands,
then they are still marked "foreign" and when GDB exits they will not be
automatically removed. This is because some other command session usually
still exists, which created that breakpoint, and expects it to remain set
unless explicitly deleted by a user.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

Todd> Basically what I need is the moral equivalent of a "user" field
Todd> tacked onto every breakpoint struct.
Todd>
Todd> Based on my existing code, the contents of this field would be:
Todd> 	UINT32	breakpointID; /* WTX-supplied handle for this breakpoint */
Todd> 	bool	is_foreign;   /* true if breakpoint was set by someone else */
Todd> 	bool	is_global;    /* true if breakpoint active for all tasks */

The reason I was passing the breakpoint pointer (with the intention of
adding a "void *private_data" field), is that I don't want to preclude
any target implementation.  A 32 bit integer ID is probably the most
common breakpoint handle, but you never can tell...

I don't think your is_foreign flag is suitable for the target specific
data, because it changes the behavior of generic breakpoint code.  You
didn't describe is_global, so I'm not sure about it.  How is it
different from (bp->thread == -1)?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Richard Earnshaw]

> "J.T. Conklin" wrote:
> 
> > Todd> So I would have to argue that the singlestep logic must either
> > Todd> happen at a really low level (this guarantees it will patch last
> > Todd> and un-patch first) or must go through the real breakpoint logic
> > Todd> which can do duplicate detection.
> > 
> > The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> > it inserts and remove trap instructions without effecting GDB's high-
> > level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> > suprised if steping into a breakpoint didn't behave the same as on a
> > machine with hardware single-step.
> 
> Its got long-term problems.
> 
> The code assumes that it is going to retain control, blocked on the
> target, while the actual step is performed.
> This doesn't work well within a pure event model where control is ment
> to return to the event-loop when ever the target is resumed.

It doesn't work properly with INSTRUCTION_NULLIFIED either -- at least it 
didn't last time I tried it...

R.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:

> Todd> So I would have to argue that the singlestep logic must either
> Todd> happen at a really low level (this guarantees it will patch last
> Todd> and un-patch first) or must go through the real breakpoint logic
> Todd> which can do duplicate detection.
> 
> The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> it inserts and remove trap instructions without effecting GDB's high-
> level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> suprised if steping into a breakpoint didn't behave the same as on a
> machine with hardware single-step.

Its got long-term problems.

The code assumes that it is going to retain control, blocked on the
target, while the actual step is performed.
This doesn't work well within a pure event model where control is ment
to return to the event-loop when ever the target is resumed.

	enjoy,
		Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
> Andrew> One aspect of this gives me cold feet and sweety palms.
> Andrew> You're giving the target code access to the entire bp struct.
> Andrew> While I don't have any problems with handing the code a
> Andrew> breakpoint handle, I have strong reservations towards any
> Andrew> moves that give the target unfettered access to the entire
> Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
> Andrew> trying to get control back again :-)
> 
> I appreciate this argument, and agree that the target interface should
> be as robust as possible.  After reading your message, I thought about
> hiding the fact that the argument is a struct breakpoint pointer and
> providing macros to access "public" fields, but any subversion we do
> can be undone by a clever programmer.  I'm still working on something
> that is simple, elegant, and efficent.

In the case of gdbarch, everything is manipulated via methods.  Each
legacy macro is backed by a C function.  The programmer does not have
direct access to the gdbarch object.

GDBARCH set a very very high standard (possibly too high).  However,
whats interesting is the problems so far identified have related to
things like defaults and not to the actual interface.

> However, I think it's a bit of a stretch to say such changes in and of
> themselves will lead to GDB being out of control.  IMHO, this can only
> happen if the GDB maintainers fail to do their jobs and integrate code
> that disrupts GDB's architectural integrity.  I'll stipulate that
> we've done a rather poor job of this in the past, but I'm hopeful that
> attitudes have changed.

I'm looking at it from the view of what is involved in making a change
to ``struct breakpoint''.  If targets directly depend on random bits
than that change becomes that much harder.

If target code can poke around with ``struct breakpoint'' internals then
there is that extra bit of pressure to let through that patch which
solves the problem that everyone is concerned about but no one has a
clean fix for.

> Andrew> I'd prefer to see something that tightens rather than loosens
> Andrew> access to ``struct breakpoint''.  Perhaphs something along the
> Andrew> lines of multi-arch where the target is notified of breakpoint
> Andrew> create, insert, remove, delete operations.

Possibly a side issue.  When a new architecture is created.  gdbarch
notifies all interested parties so that they can update their local
architecture specific information.  See gdbtypes.c for a very good
example.

If the target wants to know about breakpoint changes then notify it
(mumble something about the observer pattern).

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

>
> Stephane> I also use the breakpoint structure as an extern variable. I
> Stephane> needed it to implement the set_hw_breakpoint routine for the
> Stephane> remote protocol.  I think it's not the nicer way to do but
> Stephane> ....
>
> Can you explain?  I don't see any use of struct breakpoint in the
> current arc-tdep.c, nor do I see a set_hw_breakpoint function?

The version at FSF is a very old version out of date.
I am currently extending gdb for ARC to support user extensions.

>
> >> Since you say you can remove the single step code, I assume that the
> >> ISS target and remote protocol agents can perform the single step by
> >> themselves?  If so, it would be advantageous to disable GDB's single
> >> step support.  GDB would then issue a single "step" command instead of
> >> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> >> latency of a step should be greatly improved.
>
> Stephane> I will implement this for the remote target only since the
> Stephane> hardware supports single-stepping.  I'm not sure if this
> Stephane> feasible in the ISS - Yuri?
>
> Stephane> If not feasible I won't disable the GDB's single step
> Stephane> support (thus enabling a call to single_step()) but I will
> Stephane> implement a single_step call to gdbstub in remote_resume().
>
> If your ISS target cannot support single step, but the remote protocol
> can, perhaps the best thing is to make software single step a target
> specific option.

By creating a target specific command?
The single step mechanism is a not a step-over-calls mechanism.

>
> I don't think that the arc is the only processor that would benefit
> from such a change.  For example, the sparclet ROM monitor supports a
> single step command, but I do not know if it is ever issued because
> the SPARC target uses software single step.  Perhaps it inserts the
> trap instructions needed for single step and issues the monitor step
> command?
>
> I do not think it's possible for any change to remote_resume() to be
> the right solution.
>
You're right, I've tried and if does not work.
Thanks for your explanations anyway.

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
Stephane> What do you mean by breakpoint without a trace mode?

Processors that support hardware single step often call it instruction
trace mode or something similar.  Processors without it have to resort
to inserting trap/breakpoint/invalid instructions.

>> However, if the breakpoint insert/remove API was changed to require a
>> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
>> in the foo_single_step() functions without creating a real breakpoint
>> object to pass through that first pointer.

Stephane> I don't know what are the foo_single_step functions.

I was attempting to refer to those functions that implement single step
on all targets: arc_software_single_step(), rs6000_software_single_step,
and sparc_software_single_step().  It might have made more sense if I 
didn't forget "software_" and called it foo_software_single_step() or
maybe *_software_single_step().

Stephane> I also use the breakpoint structure as an extern variable. I
Stephane> needed it to implement the set_hw_breakpoint routine for the
Stephane> remote protocol.  I think it's not the nicer way to do but
Stephane> ....

Can you explain?  I don't see any use of struct breakpoint in the
current arc-tdep.c, nor do I see a set_hw_breakpoint function?

>> Another reason we might want a real breakpoint for this is that when
>> my memory region attribute code is complete we'll be able to say that
>> breakpoints in a region should be done with hardware breakpoints.
>> A user can debug his/her own code in read-only memory by using hbreak,
>> but there is no way to tell GDB to use hardware breakpoints for step,
>> next, continue, finish, until, etc.  One thing that surprised me was
>> that the single step code used multiple breakpoints.  That would put a
>> lot of pressure on hardware breakpoint registers.  I suspect that GDB
>> has all the information in order to insert a single breakpoint (by
>> evaluating which way a conditional branch will go, etc.).
>> 

Stephane> What would be a "real breakpoint"? a pointer to the
Stephane> breakpoint structure?

Yes, it's a a pointer to a breakpoint struct, but it's more than that.
I mean a breakpoint created by one of the breakpoint creation functions 
in breakpoint.c.

>> Since you say you can remove the single step code, I assume that the
>> ISS target and remote protocol agents can perform the single step by
>> themselves?  If so, it would be advantageous to disable GDB's single
>> step support.  GDB would then issue a single "step" command instead of
>> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
>> latency of a step should be greatly improved.

Stephane> I will implement this for the remote target only since the
Stephane> hardware supports single-stepping.  I'm not sure if this
Stephane> feasible in the ISS - Yuri?

Stephane> If not feasible I won't disable the GDB's single step
Stephane> support (thus enabling a call to single_step()) but I will
Stephane> implement a single_step call to gdbstub in remote_resume().

If your ISS target cannot support single step, but the remote protocol
can, perhaps the best thing is to make software single step a target
specific option.  

I don't think that the arc is the only processor that would benefit
from such a change.  For example, the sparclet ROM monitor supports a
single step command, but I do not know if it is ever issued because
the SPARC target uses software single step.  Perhaps it inserts the
trap instructions needed for single step and issues the monitor step
command?

I do not think it's possible for any change to remote_resume() to be
the right solution.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> >>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
>
> jtc> One issue that I'm not sure how to address is that there are several
> jtc> places breakpoints are inserted where a breakpoint has not been
> jtc> constructed.  Most of these occur in tdep code which implements single
> jtc> step with breakpoints on processors without a trace mode.
>
> Todd> Aiee! Such code is evil and must be destroyed.
>
> Stephane> It seems to me that this single_step function is actually
> Stephane> not in use.  I don't know about WindRiver, but here we have
> Stephane> two targets: an ISS and a remote target that use the generic
> Stephane> breakpoint functions before stepping.  I don't mind to
> Stephane> remove this code.
>
> I assume you are refering to the ARC port?

yes. But I was actually wrong in my last email, the single step function in
arc-tdep is
used, I was thinking to another function, sorry.
What do you mean by breakpoint without a trace mode?

>
> I see three GDB ports that do single step in software: arc, rs6000,
> and sparc.  The arc and sparc ports, insert breakpoints with target-
> _insert_breakpoint().  The rs6000 port by reading and writing memory
> directly.  Of the two methods, I prefer target_insert_breakpoint().
>
> However, if the breakpoint insert/remove API was changed to require a
> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
> in the foo_single_step() functions without creating a real breakpoint
> object to pass through that first pointer.

I don't know what are the foo_single_step functions.

I also use the breakpoint structure as an extern variable. I needed
it to implement the set_hw_breakpoint routine for the remote protocol.
I think it's not the nicer way to do but ....

> Another reason we might want a real breakpoint for this is that when
> my memory region attribute code is complete we'll be able to say that
> breakpoints in a region should be done with hardware breakpoints.
> A user can debug his/her own code in read-only memory by using hbreak,
> but there is no way to tell GDB to use hardware breakpoints for step,
> next, continue, finish, until, etc.  One thing that surprised me was
> that the single step code used multiple breakpoints.  That would put a
> lot of pressure on hardware breakpoint registers.  I suspect that GDB
> has all the information in order to insert a single breakpoint (by
> evaluating which way a conditional branch will go, etc.).
>

What would be a "real breakpoint"? a pointer to the breakpoint structure?

>
> Since you say you can remove the single step code, I assume that the
> ISS target and remote protocol agents can perform the single step by
> themselves?  If so, it would be advantageous to disable GDB's single
> step support.  GDB would then issue a single "step" command instead of
> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> latency of a step should be greatly improved.

I will implement this for the remote target only since the hardware supports
single-stepping.
I'm not sure if this feasible in the ISS - Yuri?

If not feasible I won't disable the GDB's single step support (thus enabling a
call to single_step())
but I will implement a single_step call to gdbstub in remote_resume().

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> The reason I was passing the breakpoint pointer (with the intention of
> adding a "void *private_data" field), is that I don't want to preclude
> any target implementation.  A 32 bit integer ID is probably the most
> common breakpoint handle, but you never can tell...

I didn't pretend that this was general enough for everyone, I only said
this was what I used. In my case the UINT32 is dictated by the WTX layer
that assigns the ID's.

> I don't think your is_foreign flag is suitable for the target specific
> data, because it changes the behavior of generic breakpoint code.

Hmm, actually yeah that's right. It probably should be generic.

> You didn't describe is_global, so I'm not sure about it.  How is it
> different from (bp->thread == -1)?

It probably isn't, and is more an artifact of my implementation.

The foreign/global stuff was added together, and happened late enough
in a development cycle that I needed to keep it isolated and easy to
compile back out if we had to cancel it. I had two distinct #ifdef's
so I could pinpoint all the changes; I've never taken those out.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

Todd> Basically what I need is the moral equivalent of a "user" field
Todd> tacked onto every breakpoint struct.
Todd>
Todd> Based on my existing code, the contents of this field would be:
Todd> 	UINT32	breakpointID; /* WTX-supplied handle for this breakpoint */
Todd> 	bool	is_foreign;   /* true if breakpoint was set by someone else */
Todd> 	bool	is_global;    /* true if breakpoint active for all tasks */

The reason I was passing the breakpoint pointer (with the intention of
adding a "void *private_data" field), is that I don't want to preclude
any target implementation.  A 32 bit integer ID is probably the most
common breakpoint handle, but you never can tell...

I don't think your is_foreign flag is suitable for the target specific
data, because it changes the behavior of generic breakpoint code.  You
didn't describe is_global, so I'm not sure about it.  How is it
different from (bp->thread == -1)?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

I don't think it's that bad.  

Todd> One important value of the full breakpoint machinery is that it
Todd> can help avoid the same location being patched twice. Any time
Todd> we patch the same instruction twice, we must un-patch it in
Todd> exactly reverse order or else we leave a breakpoint instruction
Todd> sitting in the code -- Not Good.

Todd> So I would have to argue that the singlestep logic must either
Todd> happen at a really low level (this guarantees it will patch last
Todd> and un-patch first) or must go through the real breakpoint logic
Todd> which can do duplicate detection.

The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
it inserts and remove trap instructions without effecting GDB's high-
level breakpoints.  So I think we're OK.  As a result, I wouldn't be
suprised if steping into a breakpoint didn't behave the same as on a
machine with hardware single-step.  

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
Andrew> One aspect of this gives me cold feet and sweety palms.
Andrew> You're giving the target code access to the entire bp struct.
Andrew> While I don't have any problems with handing the code a
Andrew> breakpoint handle, I have strong reservations towards any
Andrew> moves that give the target unfettered access to the entire
Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
Andrew> trying to get control back again :-)

I appreciate this argument, and agree that the target interface should
be as robust as possible.  After reading your message, I thought about
hiding the fact that the argument is a struct breakpoint pointer and
providing macros to access "public" fields, but any subversion we do
can be undone by a clever programmer.  I'm still working on something 
that is simple, elegant, and efficent.

However, I think it's a bit of a stretch to say such changes in and of
themselves will lead to GDB being out of control.  IMHO, this can only
happen if the GDB maintainers fail to do their jobs and integrate code
that disrupts GDB's architectural integrity.  I'll stipulate that
we've done a rather poor job of this in the past, but I'm hopeful that
attitudes have changed.

Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

I don't quite follow.  Care to elaborate?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One issue that I'm not sure how to address is that there are several
> places breakpoints are inserted where a breakpoint has not been
> constructed.  Most of these occur in tdep code which implements single
> step with breakpoints on processors without a trace mode.

Aiee! Such code is evil and must be destroyed.

One important value of the full breakpoint machinery is that it can help
avoid the same location being patched twice. Any time we patch the same
instruction twice, we must un-patch it in exactly reverse order or else
we leave a breakpoint instruction sitting in the code -- Not Good.

So I would have to argue that the singlestep logic must either happen at a
really low level (this guarantees it will patch last and un-patch first) or
must go through the real breakpoint logic which can do duplicate detection.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One aspect of this gives me cold feet and sweety palms.  You're giving
> the target code access to the entire bp struct.  While I don't have any
> problems with handing the code a breakpoint handle, I have strong
> reservations towards any moves that give the target unfettered access to
> the entire ``struct breakpoint''.  We'll be spending the next 10 years
> trying to get control back again :-)
...
> I'd prefer to see something that tightens rather than loosens access to
> ``struct breakpoint''.  Perhaphs something along the lines of multi-arch
> where the target is notified of breakpoint create, insert, remove,
> delete operations.

Basically what I need is the moral equivalent of a "user" field tacked
onto every breakpoint struct.

Based on my existing code, the contents of this field would be:

	UINT32	breakpointID;	/* WTX-supplied handle for this breakpoint */
	bool	is_foreign;	/* true if breakpoint was set by someone else */
	bool	is_global;	/* true if breakpoint active for all tasks */

The is_foreign field is an interesting concept which I don't think anyone
else has implemented; it signifies a breakpoint that GDB does not "own".
One of the events I can receive from the target is "foreign breakpoint
created" and I find a safe time to call "break *<address>" in order to
let the breakpoint show up properly in GDB.

Foreign breakpoints look like normal breakpoints to GDB users except that
if they aren't recreated (new breakpoint struct allocated) by GDB commands,
then they are still marked "foreign" and when GDB exits they will not be
automatically removed. This is because some other command session usually
still exists, which created that breakpoint, and expects it to remain set
unless explicitly deleted by a user.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
> jtc> I was planning to propose that the breakpoint pointer itself be passed
> jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
> jtc> is as good of time as any.
> 
> Todd> I say Just Do It. I am sitting on some local code here that tracks
> Todd> breakpoints added to the target by a third party, and I ended up needing
> Todd> the breakpoint shadow field to be available to those functions.
> 
> I spent some time yesterday investigating what would be necessary to
> change the target_{insert,remove}_breakpoint() API to pass a pointer
> to struct breakpoint.
> 
> What I have so far is change the API from:
>         int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
> to:
>         int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

J.T.,

One aspect of this gives me cold feet and sweety palms.  You're giving
the target code access to the entire bp struct.  While I don't have any
problems with handing the code a breakpoint handle, I have strong
reservations towards any moves that give the target unfettered access to
the entire ``struct breakpoint''.  We'll be spending the next 10 years
trying to get control back again :-)

I'd prefer to see something that tightens rather than loosens access to
``struct breakpoint''.  Perhaphs something along the lines of multi-arch
where the target is notified of breakpoint create, insert, remove,
delete operations.

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Jim Kingdon]

> I noticed ARM RDI target also has breakpoints with "handles" and
> maintains a local database mapping handles to breakpoint.  I'm
> pleased to see this, because this change could be taken advantage by
> a target already in the tree.

So you'd add a void* to the struct breakpoint for the target to do
with what it wants (together with a new deallocation function in the
target vector which delete_breakpoint would call)?

> My current thinking about how to handle these cases is to create a new
> breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
> create "real" breakpoints.  

Well, I can't quite resist mentioning the contrary idea which has been
floated which is to put all the single stepping stuff down into the
wait/resume level (I guess a new level between the existing
handle_inferior_event and friends, and the target vector).  Having
mentioned it I'm not sure an extra level is really warranted here.

So bp_singlestep might work nicely too - of course it would be added
to the list of types which insert_breakpoints doesn't mess with.  And
this has a further consequence - that you can't just detect this as a
normal breakpoint, you need to keep the
singlestep_breakpoints_inserted_p machinery in infrun.c

I realize I haven't really put much of a recommendation in there, but
but it sounds like going ahead with bp_singlestep might be the best
plan.

breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> I was planning to propose that the breakpoint pointer itself be passed
jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
jtc> is as good of time as any.  

Todd> I say Just Do It. I am sitting on some local code here that tracks
Todd> breakpoints added to the target by a third party, and I ended up needing
Todd> the breakpoint shadow field to be available to those functions.

I spent some time yesterday investigating what would be necessary to
change the target_{insert,remove}_breakpoint() API to pass a pointer
to struct breakpoint.

What I have so far is change the API from:
        int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
to:
        int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

I would have prefered to pass only the breakpoint pointer, but the
overlay support code in breakpoint.c calls target_insert_breakpoint()
twice with different addresses; once for the VMA and once for the LMA.
I don't want to address that, at least not yet.

For the most part, this change was just tedious grunt work.  All those
targets to change...  However, while I was editing my way through GDBs
files, I noticed ARM RDI target also has breakpoints with "handles"
and maintains a local database mapping handles to breakpoint.  I'm
pleased to see this, because this change could be taken advantage by a
target already in the tree.

One issue that I'm not sure how to address is that there are several
places breakpoints are inserted where a breakpoint has not been
constructed.  Most of these occur in tdep code which implements single
step with breakpoints on processors without a trace mode.

For example, from arc-tdep.c:
 
      /* Always set a breakpoint for the insn after the branch.  */
      next_pc = pc + ((type == NORMAL8 || type == BRANCH8) ? 8 : 4);
      target_insert_breakpoint (next_pc, break_mem[0]);

My current thinking about how to handle these cases is to create a new
breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
create "real" breakpoints.  

I'd appreciate if anyone can share their thoughts on this matter.

        --jtc
   
-- 
J.T. Conklin
RedBack Networks