Questionable breakpoint stepping code

Questionable breakpoint stepping code

[Vladimir Prus]

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

Re: Questionable breakpoint stepping code

[Michael Snyder]

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

Re: Questionable breakpoint stepping code

[Vladimir Prus]

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

Re: Questionable breakpoint stepping code

[Daniel Jacobowitz]

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

Re: Questionable breakpoint stepping code

[Michael Snyder]

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.

Re: Questionable breakpoint stepping code

[Vladimir Prus]

> 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

Re: Questionable breakpoint stepping code

[Daniel Jacobowitz]

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

Re: Questionable breakpoint stepping code

[Joel Brobecker]

> 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

Re: Questionable breakpoint stepping code

[Vladimir Prus]

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

Re: Questionable breakpoint stepping code

[Joel Brobecker]

> > 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

Re: Questionable breakpoint stepping code

[Vladimir Prus]

On Friday 30 November 2007 04:14:13 Joel Brobecker wrote:
> > > 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.

Thanks. Michael, do you have any objections to me checking that
patch in (included before for convenience).

- 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))

Re: Questionable breakpoint stepping code

[Vladimir Prus]

Vladimir Prus wrote:

Michael,
I wonder if you've missed the below email. Can you
tell what you think?

Thanks,
Volodya

> On Friday 30 November 2007 04:14:13 Joel Brobecker wrote:
>> > > 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.
> 
> Thanks. Michael, do you have any objections to me checking that
> patch in (included before for convenience).
> 
> - 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))

Re: Questionable breakpoint stepping code

[Jim Blandy]

Vladimir Prus <ghost at cs.msu.su> writes:
> I wonder if you've missed the below email. Can you
> tell what you think?

Hi, Vlad.  This change looks fine to me; please commit.

>
> Thanks,
> Volodya
>
>> On Friday 30 November 2007 04:14:13 Joel Brobecker wrote:
>>> > > 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.
>> 
>> Thanks. Michael, do you have any objections to me checking that
>> patch in (included before for convenience).
>> 
>> - 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))