PATCH: resume + threads + software stepping == boom

PATCH: resume + threads + software stepping == boom

[Daniel Jacobowitz]

resume () in infrun.c has this block:

  if (SOFTWARE_SINGLE_STEP_P () && step)
    {
      /* Do it the hard way, w/temp breakpoints */
      SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
      /* ...and don't ask hardware to do it.  */
      step = 0;

Then, further down, if (use_thread_step_needed && thread_step_needed)
and there's already a breakpoint at the PC, is this:

              if (!step)
                {
                  warning ("Internal error, changing continue to step.");

That blows up, because step will always be zero here if
SOFTWARE_SINGLE_STEP_P ().  Is this patch OK?  It seems to work in my tests
here.


-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team
From ac131313@cygnus.com Fri Jun 08 12:48:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: Nick Duffek <nsd@redhat.com>
Cc: brobecker@act-europe.fr, gdb-patches@sources.redhat.com
Subject: Re: patch to gdb on Tru64 5.1
Date: Fri, 08 Jun 2001 12:48:00 -0000
Message-id: <3B212C20.6090005@cygnus.com>
References: <3AFC67C4.9000102@cygnus.com> <200105130240.f4D2eiU07396@rtl.cygnus.com>
X-SW-Source: 2001-06/msg00172.html
Content-length: 592

> On 11-May-2001, Andrew Cagney wrote:
> 
> 
>> o	is it possible to compile in both tables
>> so that the correct table can be selected
>> at run time.  In theory allowing GDB to
>> handle both 4.x and 5.x core files.
> 
> 
> There's only one table.  In the transition from 4.x to 5.x, EF_* #defines
> in system header files were renamed to CF_*, but their values didn't
> change.  Therefore, a single table allows GDB to handle both 4.x and 5.x
> core files.
> 
> Maybe a comment to that effect would be enough to prevent future
> confusion?


Yes, ok, that explains it.  Thanks,

	Andrew

Re: PATCH: resume + threads + software stepping == boom

[Daniel Jacobowitz]

On Fri, Jun 08, 2001 at 12:34:32PM -0700, Daniel Jacobowitz wrote:
> resume () in infrun.c has this block:
> 
>   if (SOFTWARE_SINGLE_STEP_P () && step)
>     {
>       /* Do it the hard way, w/temp breakpoints */
>       SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
>       /* ...and don't ask hardware to do it.  */
>       step = 0;
> 
> Then, further down, if (use_thread_step_needed && thread_step_needed)
> and there's already a breakpoint at the PC, is this:
> 
>               if (!step)
>                 {
>                   warning ("Internal error, changing continue to step.");
> 
> That blows up, because step will always be zero here if
> SOFTWARE_SINGLE_STEP_P ().  Is this patch OK?  It seems to work in my tests
> here.

Oops, forgot a changelog entry for this one.

2001-06-08  Daniel Jacobowitz  <drow@mvista.com>
	* infrun.c (resume):  Add ostep variable.  Test ostep
	instead of step.



-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: PATCH: resume + threads + software stepping == boom

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> resume () in infrun.c has this block:
> 
>   if (SOFTWARE_SINGLE_STEP_P () && step)
>     {
>       /* Do it the hard way, w/temp breakpoints */
>       SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
>       /* ...and don't ask hardware to do it.  */
>       step = 0;
> 
> Then, further down, if (use_thread_step_needed && thread_step_needed)
> and there's already a breakpoint at the PC, is this:
> 
>               if (!step)
>                 {
>                   warning ("Internal error, changing continue to step.");
> 
> That blows up, because step will always be zero here if
> SOFTWARE_SINGLE_STEP_P ().  Is this patch OK?  It seems to work in my tests
> here.

I like the problem analysis, but not the implementation of the solution.
If we are going to always set step to zero for SOFTWARE_SINGLE_STEP_P,
then it does not make sense to set it to one again, even if the code
will never be reached (in theory).  I would rather see it made explicit
that this code should never be reached if SOFTWARE_SINGLE_STEP_P is true.
Something like this:

<	if (!step)
---
>	if (!(step && SOFTWARE_SINGLE_STEP_P()))

Re: PATCH: resume + threads + software stepping == boom

[Michael Snyder]

Michael Snyder wrote:
> 
> Daniel Jacobowitz wrote:
> >
> > resume () in infrun.c has this block:
> >
> >   if (SOFTWARE_SINGLE_STEP_P () && step)
> >     {
> >       /* Do it the hard way, w/temp breakpoints */
> >       SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
> >       /* ...and don't ask hardware to do it.  */
> >       step = 0;
> >
> > Then, further down, if (use_thread_step_needed && thread_step_needed)
> > and there's already a breakpoint at the PC, is this:
> >
> >               if (!step)
> >                 {
> >                   warning ("Internal error, changing continue to step.");
> >
> > That blows up, because step will always be zero here if
> > SOFTWARE_SINGLE_STEP_P ().  Is this patch OK?  It seems to work in my tests
> > here.
> 
> I like the problem analysis, but not the implementation of the solution.
> If we are going to always set step to zero for SOFTWARE_SINGLE_STEP_P,
> then it does not make sense to set it to one again, even if the code
> will never be reached (in theory).  I would rather see it made explicit
> that this code should never be reached if SOFTWARE_SINGLE_STEP_P is true.
> Something like this:
> 
> <       if (!step)
> ---
> >       if (!(step && SOFTWARE_SINGLE_STEP_P()))

Err, my logic is wrong, but you get the idea...  maybe I meant
	if (!step && !SOFTWARE_SINGLE_STEP_P())

Re: PATCH: resume + threads + software stepping == boom

[Daniel Jacobowitz]

On Fri, Jun 08, 2001 at 04:20:30PM -0700, Michael Snyder wrote:
> Michael Snyder wrote:
> > I like the problem analysis, but not the implementation of the solution.
> > If we are going to always set step to zero for SOFTWARE_SINGLE_STEP_P,
> > then it does not make sense to set it to one again, even if the code
> > will never be reached (in theory).  I would rather see it made explicit
> > that this code should never be reached if SOFTWARE_SINGLE_STEP_P is true.
> > Something like this:
> > 
> > <       if (!step)
> > ---
> > >       if (!(step && SOFTWARE_SINGLE_STEP_P()))
> 
> Err, my logic is wrong, but you get the idea...  maybe I meant
> 	if (!step && !SOFTWARE_SINGLE_STEP_P())
> 

Does SOFTWARE_SINGLE_STEP_P () contradict the error we are detecting
here?  From reading the surrounding code, I'm not entirely sure what
the case is; is it: the current thread has stopped at a breakpoint,
and we do not want to let other threads continue, so we require that we
be single stepping so that one thread does not run independently?

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: PATCH: resume + threads + software stepping == boom

[Andrew Cagney]

Daniel,

Just FYI, I don't see an assignment/diclaimer on record.  Is this the 
case?  At present your patches are just sneeking in under the bar.

enjoy,
	Andrew

Re: PATCH: resume + threads + software stepping == boom

[Daniel Jacobowitz]

On Sat, Jun 09, 2001 at 01:17:52PM -0400, Andrew Cagney wrote:
> Daniel,
> 
> Just FYI, I don't see an assignment/diclaimer on record.  Is this the 
> case?  At present your patches are just sneeking in under the bar.

Right, there isn't one yet.  I'm going to take care of that next week -
these are just the independent portions of the MIPS patches, and when I
hit the actual port... Even despite the fact that David Miller and Ralf
Baechle originally wrote it I've added enough that it certainly won't
fit under the bar.

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: PATCH: resume + threads + software stepping == boom

[Daniel Jacobowitz]

On Sat, Jun 09, 2001 at 01:15:00PM -0400, Andrew Cagney wrote:
> Could I suggest:
> 
> > On Fri, Jun 08, 2001 at 04:20:30PM -0700, Michael Snyder wrote:
> > 
> >> Michael Snyder wrote:
> > 
> >> > I like the problem analysis, but not the implementation of the solution.
> >> > If we are going to always set step to zero for SOFTWARE_SINGLE_STEP_P,
> >> > then it does not make sense to set it to one again, even if the code
> >> > will never be reached (in theory).  I would rather see it made explicit
> >> > that this code should never be reached if SOFTWARE_SINGLE_STEP_P is true.
> >> > Something like this:
> >> > 
> >> > < if (!step)
> >> > ---
> > 
> >> > > if (!(step && SOFTWARE_SINGLE_STEP_P()))
> > 
> >> 
> >> Err, my logic is wrong, but you get the idea...  maybe I meant
> >> if (!step && !SOFTWARE_SINGLE_STEP_P())
> 
> 
> Dumping the warning("internal error: ...") and replacing the entire 
> block of code with something like:
> 
> 	gdb_assert (step || SOFTWARE_SINGLE_STEP_P ())
> 
> (I know my logic is wrong).  That way the problem of ``never reached (in 
> theory)'' is eliminated.

My concern is that this is a weaker assertion than was intended.  How
about this - it changes the meaning of the assertion slightly since
it's not conditional on should_resume, but I think it'll be correct
none the less:


Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.35
diff -u -r1.35 infrun.c
--- infrun.c	2001/06/02 00:36:20	1.35
+++ infrun.c	2001/06/09 23:05:32
@@ -850,6 +850,11 @@
   if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
     SKIP_PERMANENT_BREAKPOINT ();
 
+  /* If we stopped on a breakpoint and it was not deleted, we want to continue
+     only this thread, so we should be stepping. */
+  gdb_assert (step || !use_thread_step || !thread_step_needed ||
+	      !breakpoint_here_p (read_pc ()));
+
   if (SOFTWARE_SINGLE_STEP_P () && step)
     {
       /* Do it the hard way, w/temp breakpoints */
@@ -927,14 +932,7 @@
 	    }
 	  else
 	    {
-	      if (!step)
-		{
-		  warning ("Internal error, changing continue to step.");
-		  remove_breakpoints ();
-		  breakpoints_inserted = 0;
-		  trap_expected = 1;
-		  step = 1;
-		}
+	      /* Breakpoint not deleted, so step only this thread. */
 	      resume_ptid = inferior_ptid;
 	    }
 	}


-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: PATCH: resume + threads + software stepping == boom

[Michael Snyder]

Daniel Jacobowitz wrote:
[extensive snipping]
> +  /* If we stopped on a breakpoint and it was not deleted, we want to continue
> +     only this thread, so we should be stepping. */

I agree with the premise -- whenever we resume at a breakpoint,
we should do a single-thread-step before inserting bps, so that 
other threads do not have a chance to run (and possibly go past
other breakpoints) while breakpoints are not inserted.

How do others feel about that premise?  I think we had 
reached that consensus at some earlier time, but it has not
been implemented (almost, but not quite).

Re: PATCH: resume + threads + software stepping == boom

[Jim Blandy]

Daniel Jacobowitz <dmj+@andrew.cmu.edu> writes:
> On Sat, Jun 09, 2001 at 01:17:52PM -0400, Andrew Cagney wrote:
> > Daniel,
> > 
> > Just FYI, I don't see an assignment/diclaimer on record.  Is this the 
> > case?  At present your patches are just sneeking in under the bar.
> 
> Right, there isn't one yet.  I'm going to take care of that next week -
> these are just the independent portions of the MIPS patches, and when I
> hit the actual port... Even despite the fact that David Miller and Ralf
> Baechle originally wrote it I've added enough that it certainly won't
> fit under the bar.

I've e-mailed him the form.

Re: PATCH: resume + threads + software stepping == boom

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Sat, Jun 09, 2001 at 01:15:00PM -0400, Andrew Cagney wrote:
> > Could I suggest:
> >
> > > On Fri, Jun 08, 2001 at 04:20:30PM -0700, Michael Snyder wrote:
> > >
> > >> Michael Snyder wrote:
> > >
> > >> > I like the problem analysis, but not the implementation of the solution.
> > >> > If we are going to always set step to zero for SOFTWARE_SINGLE_STEP_P,
> > >> > then it does not make sense to set it to one again, even if the code
> > >> > will never be reached (in theory).  I would rather see it made explicit
> > >> > that this code should never be reached if SOFTWARE_SINGLE_STEP_P is true.
> > >> > Something like this:
> > >> >
> > >> > < if (!step)
> > >> > ---
> > >
> > >> > > if (!(step && SOFTWARE_SINGLE_STEP_P()))
> > >
> > >>
> > >> Err, my logic is wrong, but you get the idea...  maybe I meant
> > >> if (!step && !SOFTWARE_SINGLE_STEP_P())
> >
> >
> > Dumping the warning("internal error: ...") and replacing the entire
> > block of code with something like:
> >
> >       gdb_assert (step || SOFTWARE_SINGLE_STEP_P ())
> >
> > (I know my logic is wrong).  That way the problem of ``never reached (in
> > theory)'' is eliminated.
> 
> My concern is that this is a weaker assertion than was intended.  How
> about this - it changes the meaning of the assertion slightly since
> it's not conditional on should_resume, but I think it'll be correct
> none the less:

Daniel, just to let you know, I'm still thinking about this.
As you may or may not have noticed, I'm in the process of submitting
a bunch of bug fixes in infrun.c thread handling code.  I won't 
forget about this one.   ;-)

Michael

> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.35
> diff -u -r1.35 infrun.c
> --- infrun.c    2001/06/02 00:36:20     1.35
> +++ infrun.c    2001/06/09 23:05:32
> @@ -850,6 +850,11 @@
>    if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
>      SKIP_PERMANENT_BREAKPOINT ();
> 
> +  /* If we stopped on a breakpoint and it was not deleted, we want to continue
> +     only this thread, so we should be stepping. */
> +  gdb_assert (step || !use_thread_step || !thread_step_needed ||
> +             !breakpoint_here_p (read_pc ()));
> +
>    if (SOFTWARE_SINGLE_STEP_P () && step)
>      {
>        /* Do it the hard way, w/temp breakpoints */
> @@ -927,14 +932,7 @@
>             }
>           else
>             {
> -             if (!step)
> -               {
> -                 warning ("Internal error, changing continue to step.");
> -                 remove_breakpoints ();
> -                 breakpoints_inserted = 0;
> -                 trap_expected = 1;
> -                 step = 1;
> -               }
> +             /* Breakpoint not deleted, so step only this thread. */
>               resume_ptid = inferior_ptid;
>             }
>         }
> 
> --
> Daniel Jacobowitz                           Debian GNU/Linux Developer
> Monta Vista Software                              Debian Security Team