Questionable breakpoint stepping code

The infrun.c:handle_inferiour_event function has
this code block:

        if (thread_hop_needed)
        {
          ........
          remove_status = remove_breakpoints ();
          /* Did we fail to remove breakpoints?  If so, try
             to set the PC past the bp.  (There's at least
             one situation in which we can fail to remove
             the bp's: On HP-UX's that use ttrace, we can't
             change the address space of a vforking child
             process until the child exits (well, okay, not
             then either  or execs.
*/
          if (remove_status != 0)
            {
              /* FIXME!  This is obviously non-portable! */
              write_pc_pid (stop_pc + 4, ecs->ptid);
              /* We need to restart all the threads now,
               * unles we're running in scheduler-locked
mode. 
               * Use currently_stepping to determine whether
to 
               * step or continue.
               */
              /* FIXME MVS: is there any reason not to call
resume()? */
              if (scheduler_mode == schedlock_on)
                target_resume (ecs->ptid,
                               currently_stepping (ecs),
TARGET_SIGNAL_0);
              else
                target_resume (RESUME_ALL,
                               currently_stepping (ecs),
TARGET_SIGNAL_0);
              prepare_to_wait (ecs);
              return;
            }

The code is a bit scary -- specifically I sure don't want
GDB to mess
with PC values like this on x86, if removing breakpoints
fails in any way.
The essential bits of this code are present as of revision
1.1 of infrun.c
(added in 1999). 

So:
1. Anybody knows if this code is still needed for modern
HPUX?
2. Can we have it wrapped in #ifdef, and if so, which one?

- Volodya

On Fri, 2007-11-23 at 16:56 +0300, Vladimir Prus wrote:
> The infrun.c:handle_inferiour_event function has
> this code block:
> 
>         if (thread_hop_needed)
>         {
>           ........
>           remove_status = remove_breakpoints ();
>           /* Did we fail to remove breakpoints?  If so,
try
>              to set the PC past the bp.  (There's at
least
>              one situation in which we can fail to
remove
>              the bp's: On HP-UX's that use ttrace, we
can't
>              change the address space of a vforking
child
>              process until the child exits (well, okay,
not
>              then either  or execs.
*/
>           if (remove_status != 0)
>             {
>               /* FIXME!  This is obviously
non-portable! */
>               write_pc_pid (stop_pc + 4,
ecs->ptid);
>               /* We need to restart all the threads
now,
>                * unles we're running in
scheduler-locked mode. 
>                * Use currently_stepping to determine
whether to 
>                * step or continue.
>                */
>               /* FIXME MVS: is there any reason not to
call resume()? */
>               if (scheduler_mode == schedlock_on)
>                 target_resume (ecs->ptid,
>                                currently_stepping
(ecs), TARGET_SIGNAL_0);
>               else
>                 target_resume (RESUME_ALL,
>                                currently_stepping
(ecs), TARGET_SIGNAL_0);
>               prepare_to_wait (ecs);
>               return;
>             }
> 
> The code is a bit scary -- specifically I sure don't
want GDB to mess
> with PC values like this on x86, if removing
breakpoints fails in any way.
> The essential bits of this code are present as of
revision 1.1 of infrun.c
> (added in 1999). 
> 
> So:
> 1. Anybody knows if this code is still needed for
modern HPUX?
> 2. Can we have it wrapped in #ifdef, and if so, which
one?
> 
> - Volodya

Hi Volodya, 

I think it's my code.  It's not really related specifically
to HPUX, that comment was there in the previous iteration
and
I just kept it.

The several state variables with "thread_hop" as
part of their
names are related to single-stepping in the presence of
thread-
specific breakpoints.  They are meant to solve the problem
of
what to do if you are doing a step, and you hit a
thread-specific
breakpoint, but with the wrong thread.

You need to do a kind of special single-step to get past
that
particular breakpoint, then return to the single-stepping
infrun state.

As for the scheduler-locking code, that pertains to a 
different but not wholly unrelated functionality (set 
scheduler-locking), which affects which threads can run
at which times.

As for your last question, no, I don't believe we approve
of ifdefs...

Cheers,
Michael

On Monday 26 November 2007 21:49:27 Michael Snyder wrote:
> On Fri, 2007-11-23 at 16:56 +0300, Vladimir Prus
wrote:
> > The infrun.c:handle_inferiour_event function has
> > this code block:
> > 
> >         if (thread_hop_needed)
> >         {
> >           ........
> >           remove_status = remove_breakpoints ();
> >           /* Did we fail to remove breakpoints? 
If so, try
> >              to set the PC past the bp.  (There's
at least
> >              one situation in which we can fail to
remove
> >              the bp's: On HP-UX's that use ttrace,
we can't
> >              change the address space of a
vforking child
> >              process until the child exits (well,
okay, not
> >              then either  or execs.
*/
> >           if (remove_status != 0)
> >             {
> >               /* FIXME!  This is obviously
non-portable! */
> >               write_pc_pid (stop_pc + 4,
ecs->ptid);
> >               /* We need to restart all the
threads now,
> >                * unles we're running in
scheduler-locked mode. 
> >                * Use currently_stepping to
determine whether to 
> >                * step or continue.
> >                */
> >               /* FIXME MVS: is there any reason
not to call resume()? */
> >               if (scheduler_mode == schedlock_on)
> >                 target_resume (ecs->ptid,
> >                                currently_stepping
(ecs), TARGET_SIGNAL_0);
> >               else
> >                 target_resume (RESUME_ALL,
> >                                currently_stepping
(ecs), TARGET_SIGNAL_0);
> >               prepare_to_wait (ecs);
> >               return;
> >             }
> > 
> > The code is a bit scary -- specifically I sure
don't want GDB to mess
> > with PC values like this on x86, if removing
breakpoints fails in any way.
> > The essential bits of this code are present as of
revision 1.1 of infrun.c
> > (added in 1999). 
> > 
> > So:
> > 1. Anybody knows if this code is still needed for
modern HPUX?
> > 2. Can we have it wrapped in #ifdef, and if so,
which one?
> > 
> > - Volodya
> 
> Hi Volodya, 
> 
> I think it's my code.  It's not really related
specifically
> to HPUX, that comment was there in the previous
iteration and
> I just kept it.
> 
> The several state variables with "thread_hop"
as part of their
> names are related to single-stepping in the presence of
thread-
> specific breakpoints.  They are meant to solve the
problem of
> what to do if you are doing a step, and you hit a
thread-specific
> breakpoint, but with the wrong thread.

Yes, I know that.

> 
> You need to do a kind of special single-step to get
past that
> particular breakpoint, then return to the
single-stepping
> infrun state.

Right.

> As for the scheduler-locking code, that pertains to a 
> different but not wholly unrelated functionality (set 
> scheduler-locking), which affects which threads can
run
> at which times.

I know that too.

> 
> As for your last question, no, I don't believe we
approve
> of ifdefs...

Well, the question, then is -- how do we make this code
work
correctly even if the instruction at PC is not 4 bytes in
size?
Calling disassembler seems a plausible approach.

- Volodya

On Mon, Nov 26, 2007 at 10:16:27PM +0300, Vladimir Prus
wrote:
> > As for your last question, no, I don't believe we
approve
> > of ifdefs...
> 
> Well, the question, then is -- how do we make this code
work
> correctly even if the instruction at PC is not 4 bytes
in size?
> Calling disassembler seems a plausible approach.

Don't Do It.  Just call error, I suggest.  Skipping an
instruction
can lead to bizarre failures later.

-- 
Daniel Jacobowitz
CodeSourcery

On Mon, 2007-11-26 at 22:16 +0300, Vladimir Prus wrote:

> > As for your last question, no, I don't believe we
approve
> > of ifdefs...
> 
> Well, the question, then is -- how do we make this code
work
> correctly even if the instruction at PC is not 4 bytes
in size?
> Calling disassembler seems a plausible approach.

Good suggestion.

> On Mon, 2007-11-26 at 22:16 +0300, Vladimir Prus
wrote:
> 
> > > As for your last question, no, I don't
believe we approve
> > > of ifdefs...
> > 
> > Well, the question, then is -- how do we make this
code work
> > correctly even if the instruction at PC is not 4
bytes in size?
> > Calling disassembler seems a plausible approach.

On Monday 26 November 2007 22:16:16 Michael Snyder wrote:

> Good suggestion.

On Monday 26 November 2007 22:24:07 Daniel Jacobowitz
wrote:

> Don't Do It.  Just call error, I suggest.  Skipping an
instruction
> can lead to bizarre failures later.

Ok, I'm now not quite sure what to do. I personally prefer
calling 'error' --
is this going to be acceptable?

- Volodya

On Tue, Nov 27, 2007 at 04:50:16PM +0300, Vladimir Prus
wrote:
> Ok, I'm now not quite sure what to do. I personally
prefer calling 'error' --
> is this going to be acceptable?

I would recommend asking someone with access to HP-UX (I
think Mark
Kettenis and Joel both do) to test the patch.  I don't think
the error
will trigger...

-- 
Daniel Jacobowitz
CodeSourcery

> I would recommend asking someone with access to HP-UX
(I think Mark
> Kettenis and Joel both do) to test the patch.  I don't
think the error
> will trigger...

I have been away for a week and I still a bit overwhelmed by
my email
backlog that caused me to miss the patch. But if Volodya
sends me the
patch again, I'll give it a whirl.

-- 
Joel

On Tuesday 27 November 2007 22:32:43 Joel Brobecker wrote:
> > I would recommend asking someone with access to
HP-UX (I think Mark
> > Kettenis and Joel both do) to test the patch.  I
don't think the error
> > will trigger...
> 
> I have been away for a week and I still a bit
overwhelmed by my email
> backlog that caused me to miss the patch. But if
Volodya sends me the
> patch again, I'll give it a whirl.

Thanks Joel!
Here's the patch.

- Volodya
   
        * infrun.c (handle_inferior_event): If
        we failed to remove breakpoints, error,
        don't try to increment PC by hand.
---
 gdb/infrun.c |   19 +------------------
 1 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index ad1de6b..b92d0de 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
 -1745,24
+1745,7  handle_inferior_event (struct
execution_control_state *ecs)
             process until the child exits (well, okay, not
             then either  or execs.
*/
          if (remove_status != 0)
-           {
-             /* FIXME!  This is obviously non-portable! */
-             write_pc_pid (stop_pc + 4, ecs->ptid);
-             /* We need to restart all the threads now,
-              * unles we're running in scheduler-locked
mode. 
-              * Use currently_stepping to determine whether
to 
-              * step or continue.
-              */
-             /* FIXME MVS: is there any reason not to call
resume()? */
-             if (scheduler_mode == schedlock_on)
-               target_resume (ecs->ptid,
-                              currently_stepping (ecs),
TARGET_SIGNAL_0);
-             else
-               target_resume (RESUME_ALL,
-                              currently_stepping (ecs),
TARGET_SIGNAL_0);
-             prepare_to_wait (ecs);
-             return;
-           }
+           error (_("Cannot step over breakpoint hit
in wrong thread"));
          else
            {                   /* Single step */
              if (!ptid_equal (inferior_ptid,
ecs->ptid))
-- 
1.5.3.5

> > I have been away for a week and I still a bit
overwhelmed by my email
> > backlog that caused me to miss the patch. But if
Volodya sends me the
> > patch again, I'll give it a whirl.
> 
> Thanks Joel!
> Here's the patch.

Thanks. I don't know why, but somehow, the patch didn't
apply cleanly
to HEAD, so I applied it manually. The results were
identical after
applying and I double-checked that the error never
triggered.

-- 
Joel