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

Michael Snyder
2001-06-13T15:21:00Z

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