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

[Michael Snyder <msnyder@cygnus.com>]

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