[patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Hello,

This patch renames the ``not_a_breakpoint'' parameter of 
bpstat_stop_status() to the more correct ``not_a_sw_breakpoint'' so that 
it is clear that it is indicating nothing about hardware breakpoints.

committed,
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2737 bytes --]

2002-08-16  Andrew Cagney  <ac131313@redhat.com>

	* breakpoint.c (bpstat_stop_status): Rename not_a_breakpoint to
	not_a_sw_breakpoint.
	* breakpoint.h (bpstat_stop_status): Add parameter names.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.78
diff -u -r1.78 breakpoint.c
--- breakpoint.c	26 Jun 2002 05:20:04 -0000	1.78
+++ breakpoint.c	16 Aug 2002 15:34:19 -0000
@@ -2390,7 +2390,7 @@
 
 /* Get a bpstat associated with having just stopped at address *PC
    and frame address CORE_ADDRESS.  Update *PC to point at the
-   breakpoint (if we hit a breakpoint).  NOT_A_BREAKPOINT is nonzero
+   breakpoint (if we hit a breakpoint).  NOT_A_SW_BREAKPOINT is nonzero
    if this is known to not be a real breakpoint (it could still be a
    watchpoint, though).  */
 
@@ -2409,7 +2409,7 @@
    commands, FIXME??? fields.  */
 
 bpstat
-bpstat_stop_status (CORE_ADDR *pc, int not_a_breakpoint)
+bpstat_stop_status (CORE_ADDR *pc, int not_a_sw_breakpoint)
 {
   register struct breakpoint *b, *temp;
   CORE_ADDR bp_addr;
@@ -2423,13 +2423,13 @@
   "Error evaluating expression for watchpoint %d\n";
   char message[sizeof (message1) + 30 /* slop */ ];
 
-  /* Get the address where the breakpoint would have been.  
-     The "not_a_breakpoint" argument is meant to distinguish 
-     between a breakpoint trap event and a trace/singlestep
-     trap event.  For a trace/singlestep trap event, we would
-     not want to subtract DECR_PC_AFTER_BREAK from the PC. */
+  /* Get the address where the breakpoint would have been.  The
+     "not_a_sw_breakpoint" argument is meant to distinguish between a
+     breakpoint trap event and a trace/singlestep trap event.  For a
+     trace/singlestep trap event, we would not want to subtract
+     DECR_PC_AFTER_BREAK from the PC. */
 
-  bp_addr = *pc - (not_a_breakpoint && !SOFTWARE_SINGLE_STEP_P () ? 
+  bp_addr = *pc - (not_a_sw_breakpoint && !SOFTWARE_SINGLE_STEP_P () ? 
                    0 : DECR_PC_AFTER_BREAK);
 
   ALL_BREAKPOINTS_SAFE (b, temp)
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.12
diff -u -r1.12 breakpoint.h
--- breakpoint.h	10 Jun 2002 23:25:50 -0000	1.12
+++ breakpoint.h	16 Aug 2002 15:34:19 -0000
@@ -322,7 +322,7 @@
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
-extern bpstat bpstat_stop_status (CORE_ADDR *, int);
+extern bpstat bpstat_stop_status (CORE_ADDR *pc, int not_a_sw_breakpoint);
 \f
 /* This bpstat_what stuff tells wait_for_inferior what to do with a
    breakpoint (a challenging task).  */

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Daniel Jacobowitz]

On Fri, Aug 16, 2002 at 11:37:34AM -0400, Andrew Cagney wrote:
> Hello,
> 
> This patch renames the ``not_a_breakpoint'' parameter of 
> bpstat_stop_status() to the more correct ``not_a_sw_breakpoint'' so that 
> it is clear that it is indicating nothing about hardware breakpoints.
> 
> committed,
> Andrew

Great.  I'm going to have to think about this a little more though; if
you look in infrun.c you'll see that this parameter sometimes comes
from catchpoints, which is unfortunate since we have nowhere that
indicates whether a catchpoint is affected by DECR_PC_AFTER_BREAK or
not.

(For i386/Linux, when I'm done with it, I believe that throw and catch
catchpoints WILL be affected by decr_pc_after_break.... and that
fork/exec/vfork catchpoints WON'T be.  I had to hack around this in my
work tree.)

Do I remember someone commenting that DECR_PC_AFTER_BREAK was a mess,
perhaps?  I think I do... :)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Kevin Buettner]

On Aug 16, 11:37am, Andrew Cagney wrote:

> This patch renames the ``not_a_breakpoint'' parameter of 
> bpstat_stop_status() to the more correct ``not_a_sw_breakpoint'' so that 
> it is clear that it is indicating nothing about hardware breakpoints.

This may be a good change, but I don't think it's obvious.  Could
you at least explain the reasoning that lead you to conclude that
the parameter in question indicates nothing about hardware breakpoints?

Kevin

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

> On Aug 16, 11:37am, Andrew Cagney wrote:
> 
> 
>> This patch renames the ``not_a_breakpoint'' parameter of 
>> bpstat_stop_status() to the more correct ``not_a_sw_breakpoint'' so that 
>> it is clear that it is indicating nothing about hardware breakpoints.
> 
> 
> This may be a good change, but I don't think it's obvious.  Could
> you at least explain the reasoning that lead you to conclude that
> the parameter in question indicates nothing about hardware breakpoints?

The variable is used vis:

bpstat_stop_status (CORE_ADDR *pc, int not_a_sw_breakpoint)
{
[...]
   /* Get the address where the breakpoint would have been.  The
      "not_a_sw_breakpoint" argument is meant to distinguish between a
      breakpoint trap event and a trace/singlestep trap event.  For a
      trace/singlestep trap event, we would not want to subtract
      DECR_PC_AFTER_BREAK from the PC. */

   bp_addr = *pc - (not_a_sw_breakpoint && !SOFTWARE_SINGLE_STEP_P () ?
                    0 : DECR_PC_AFTER_BREAK);


Later in the code appears:

           if (DECR_PC_AFTER_HW_BREAK != 0)
             {
               *pc = *pc - DECR_PC_AFTER_HW_BREAK;
               write_pc (*pc);
             }

if not_a_sw_breakpoint applied to hardware breakpoints then the above 
decrement would be guarded by not_a_sw_breakpoint.

BTW, an even more correct name is ``not_a_sw_breakpoint_trap''. 
However, Joel might end up adding something better than that.

I guess nothing is obvious :-(

enjoy,
Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

 > Great.  I'm going to have to think about this a little more though; if
 > you look in infrun.c you'll see that this parameter sometimes comes
 > from catchpoints, which is unfortunate since we have nowhere that
 > indicates whether a catchpoint is affected by DECR_PC_AFTER_BREAK or
 > not.

See my e-mail to Kevin.  it decides if 
DECR_PC_AFTER_[SOFTWARE_]BREAK[POINT_TRAP] should be applied.

 > (For i386/Linux, when I'm done with it, I believe that throw and catch
 > catchpoints WILL be affected by decr_pc_after_break.... and that
 > fork/exec/vfork catchpoints WON'T be.  I had to hack around this in my
 > work tree.)

Are throw/catch events implemented using software breakpoints that are 
entered into the breakpoint table?

One of the characteristics of the software single step breakpoints is 
that they are not entered into the breakpoint table.  This is why Joel 
needs to hide them from core GDB :-)

I think fork/exec events can be treated separatly.

enjoy,
Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Daniel Jacobowitz]

On Fri, Aug 16, 2002 at 01:33:59PM -0400, Andrew Cagney wrote:
> > Great.  I'm going to have to think about this a little more though; if
> > you look in infrun.c you'll see that this parameter sometimes comes
> > from catchpoints, which is unfortunate since we have nowhere that
> > indicates whether a catchpoint is affected by DECR_PC_AFTER_BREAK or
> > not.
> 
> See my e-mail to Kevin.  it decides if 
> DECR_PC_AFTER_[SOFTWARE_]BREAK[POINT_TRAP] should be applied.
> 
> > (For i386/Linux, when I'm done with it, I believe that throw and catch
> > catchpoints WILL be affected by decr_pc_after_break.... and that
> > fork/exec/vfork catchpoints WON'T be.  I had to hack around this in my
> > work tree.)
> 
> Are throw/catch events implemented using software breakpoints that are 
> entered into the breakpoint table?
> 
> One of the characteristics of the software single step breakpoints is 
> that they are not entered into the breakpoint table.  This is why Joel 
> needs to hide them from core GDB :-)
> 
> I think fork/exec events can be treated separatly.

Well, throw/catch events will be (haven't done it yet) implemented
using (some kind of) breakpoints.  Whether they will be in the table or
not is a different question.  I personally think that the way
catchpoints are handled at the moment is all wrong, since it relies on
the to_wait method to determine what event occured; which is perfect
for event reporting mechanisms and awful for events synthesized by
breakpoints.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

> Well, throw/catch events will be (haven't done it yet) implemented
> using (some kind of) breakpoints.  Whether they will be in the table or
> not is a different question.  I personally think that the way
> catchpoints are handled at the moment is all wrong, since it relies on
> the to_wait method to determine what event occured; which is perfect
> for event reporting mechanisms and awful for events synthesized by
> breakpoints.

The software single step breakpoint, has a similar problem.  One theory 
is to use the breakpoint table for them as well.  The current interfaces 
definitly do not lend themselves to such a model.

enjoy,
Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Daniel Jacobowitz]

On Fri, Aug 16, 2002 at 01:43:35PM -0400, Andrew Cagney wrote:
> 
> >Well, throw/catch events will be (haven't done it yet) implemented
> >using (some kind of) breakpoints.  Whether they will be in the table or
> >not is a different question.  I personally think that the way
> >catchpoints are handled at the moment is all wrong, since it relies on
> >the to_wait method to determine what event occured; which is perfect
> >for event reporting mechanisms and awful for events synthesized by
> >breakpoints.
> 
> The software single step breakpoint, has a similar problem.  One theory 
> is to use the breakpoint table for them as well.  The current interfaces 
> definitly do not lend themselves to such a model.

Hmmmmmmmm.  I have some ideas how this would be done.  I'll stew on it
and bring it up after 5.3 branches.  It would involve doing great
violence to handle_inferior_event, unfortunately; but sometimes we've
got to take risks...

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

> On Fri, Aug 16, 2002 at 01:43:35PM -0400, Andrew Cagney wrote:
> 
>> 
> 
>> >Well, throw/catch events will be (haven't done it yet) implemented
>> >using (some kind of) breakpoints.  Whether they will be in the table or
>> >not is a different question.  I personally think that the way
>> >catchpoints are handled at the moment is all wrong, since it relies on
>> >the to_wait method to determine what event occured; which is perfect
>> >for event reporting mechanisms and awful for events synthesized by
>> >breakpoints.
> 
>> 
>> The software single step breakpoint, has a similar problem.  One theory 
>> is to use the breakpoint table for them as well.  The current interfaces 
>> definitly do not lend themselves to such a model.
> 
> 
> Hmmmmmmmm.  I have some ideas how this would be done.  I'll stew on it
> and bring it up after 5.3 branches.  It would involve doing great
> violence to handle_inferior_event, unfortunately; but sometimes we've
> got to take risks...

It can't be less violent than my patch to separate bpstop_stop_status() 
from the code that prints the stop status.

Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Daniel Jacobowitz]

On Fri, Aug 16, 2002 at 01:53:09PM -0400, Andrew Cagney wrote:
> >On Fri, Aug 16, 2002 at 01:43:35PM -0400, Andrew Cagney wrote:
> >
> >>
> >
> >>>Well, throw/catch events will be (haven't done it yet) implemented
> >>>using (some kind of) breakpoints.  Whether they will be in the table or
> >>>not is a different question.  I personally think that the way
> >>>catchpoints are handled at the moment is all wrong, since it relies on
> >>>the to_wait method to determine what event occured; which is perfect
> >>>for event reporting mechanisms and awful for events synthesized by
> >>>breakpoints.
> >
> >>
> >>The software single step breakpoint, has a similar problem.  One theory 
> >>is to use the breakpoint table for them as well.  The current interfaces 
> >>definitly do not lend themselves to such a model.
> >
> >
> >Hmmmmmmmm.  I have some ideas how this would be done.  I'll stew on it
> >and bring it up after 5.3 branches.  It would involve doing great
> >violence to handle_inferior_event, unfortunately; but sometimes we've
> >got to take risks...
> 
> It can't be less violent than my patch to separate bpstop_stop_status() 
> from the code that prints the stop status.

No, probably rather worse, I'd guess - but that's pretty impressive. 
Also queued for post-5.3?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

> Also queued for post-5.3?

Yep, stalled.  Try a directory I've had sitting around since before the 
5.2 branch! I should probably just cut a branch for it and start again :-)

Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Kevin Buettner]

On Aug 16,  1:34pm, Andrew Cagney wrote:

> > On Aug 16, 11:37am, Andrew Cagney wrote:
> > 
> > 
> >> This patch renames the ``not_a_breakpoint'' parameter of 
> >> bpstat_stop_status() to the more correct ``not_a_sw_breakpoint'' so that 
> >> it is clear that it is indicating nothing about hardware breakpoints.
> > 
> > 
> > This may be a good change, but I don't think it's obvious.  Could
> > you at least explain the reasoning that lead you to conclude that
> > the parameter in question indicates nothing about hardware breakpoints?
> 
> The variable is used vis:
> 
> bpstat_stop_status (CORE_ADDR *pc, int not_a_sw_breakpoint)
> {
> [...]
>    /* Get the address where the breakpoint would have been.  The
>       "not_a_sw_breakpoint" argument is meant to distinguish between a
>       breakpoint trap event and a trace/singlestep trap event.  For a
>       trace/singlestep trap event, we would not want to subtract
>       DECR_PC_AFTER_BREAK from the PC. */
> 
>    bp_addr = *pc - (not_a_sw_breakpoint && !SOFTWARE_SINGLE_STEP_P () ?
>                     0 : DECR_PC_AFTER_BREAK);
> 
> 
> Later in the code appears:
> 
>            if (DECR_PC_AFTER_HW_BREAK != 0)
>              {
>                *pc = *pc - DECR_PC_AFTER_HW_BREAK;
>                write_pc (*pc);
>              }
> 
> if not_a_sw_breakpoint applied to hardware breakpoints then the above 
> decrement would be guarded by not_a_sw_breakpoint.
> 
> BTW, an even more correct name is ``not_a_sw_breakpoint_trap''. 
> However, Joel might end up adding something better than that.

Your reasoning is sound so long as we assume that the code
that you're basing your reasoning on isn't bitrotted.  (I only see
one target with a non-zero DECR_PC_AFTER_HW_BREAK, and I'll bet that
hasn't been tested in a while.)

What I'd like to be convinced of is that the conditions which are
used to instantiate ``not_a_sw_breakpoint'' really imply that that
we could be stopped due to a hardware watchpoint, but not a software
breakpoint trap.

The conditions in question are:

    currently_stepping (ecs) && prev_pc != stop_pc - DECR_PC_AFTER_BREAK

and (worse still):

    (currently_stepping (ecs)
     && prev_pc !=
     stop_pc - DECR_PC_AFTER_BREAK
     && !(step_range_end
	  && INNER_THAN (read_sp (),
			 (step_sp -
			  16))))

Actually, I'm not convinced that the name "not a breakpoint" (whether
they be software or otherwise) is a good characterization of these
conditions.  That said, I don't have a better suggestion.

(My head hurts whenever I look at this code.)

Kevin

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

>> The variable is used vis:
>> 
>> bpstat_stop_status (CORE_ADDR *pc, int not_a_sw_breakpoint)
>> {
>> [...]
>>    /* Get the address where the breakpoint would have been.  The
>>       "not_a_sw_breakpoint" argument is meant to distinguish between a
>>       breakpoint trap event and a trace/singlestep trap event.  For a
>>       trace/singlestep trap event, we would not want to subtract
>>       DECR_PC_AFTER_BREAK from the PC. */
>> 
>>    bp_addr = *pc - (not_a_sw_breakpoint && !SOFTWARE_SINGLE_STEP_P () ?
>>                     0 : DECR_PC_AFTER_BREAK);
>> 
>> 
>> Later in the code appears:
>> 
>>            if (DECR_PC_AFTER_HW_BREAK != 0)
>>              {
>>                *pc = *pc - DECR_PC_AFTER_HW_BREAK;
>>                write_pc (*pc);
>>              }
>> 
>> if not_a_sw_breakpoint applied to hardware breakpoints then the above 
>> decrement would be guarded by not_a_sw_breakpoint.
>> 
>> BTW, an even more correct name is ``not_a_sw_breakpoint_trap''. 
>> However, Joel might end up adding something better than that.
> 
> 
> Your reasoning is sound so long as we assume that the code
> that you're basing your reasoning on isn't bitrotted.  (I only see
> one target with a non-zero DECR_PC_AFTER_HW_BREAK, and I'll bet that
> hasn't been tested in a while.)

I think that is separate.  The ``intent'' of the variable is to indicate 
that what is being looked at isn't a trap due to a software breakpoint 
event.

> What I'd like to be convinced of is that the conditions which are
> used to instantiate ``not_a_sw_breakpoint'' really imply that that
> we could be stopped due to a hardware watchpoint, but not a software
> breakpoint trap.
> 
> The conditions in question are:
> 
>     currently_stepping (ecs) && prev_pc != stop_pc - DECR_PC_AFTER_BREAK

Yes, see my e-mail to Joel.  I'm asking that a separate explict 
``trap_from_software_singlestep'' be ||ed into those equations. While 
the rest might be pretty bogus (the above is just an heuristic really) 
at least that flag will be correct :-)

Andrew

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Kevin Buettner]

On Aug 16,  3:34pm, Andrew Cagney wrote:

> I think that is separate.  The ``intent'' of the variable is to indicate 
> that what is being looked at isn't a trap due to a software breakpoint 
> event.

Okay, I've studied the code some more and I finally agree.

(I haven't convinced myself though that the conditions used to
instantiate this variable will guarantee this ``intent''.)

Kevin

Re: [patch/ob] not_a_breakpoint -> not_a_sw_breakpoint

[Andrew Cagney]

> On Aug 16,  3:34pm, Andrew Cagney wrote:
> 
> 
>> I think that is separate.  The ``intent'' of the variable is to indicate 
>> that what is being looked at isn't a trap due to a software breakpoint 
>> event.
> 
> 
> Okay, I've studied the code some more and I finally agree.
> 
> (I haven't convinced myself though that the conditions used to
> instantiate this variable will guarantee this ``intent''.)

That's probably because I've convinced myself that the conditions used 
most likely don't guarentee the intent! :-)  However, we've got to start 
somewhere :-)

enjoy,
Andrew