Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves <pedro at codesourcery dot com>]

On Friday 05 December 2008 01:15:30, Pedro Alves wrote:
> On Friday 05 December 2008 00:36:56, Pedro Alves wrote:
> > On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
> > > Pedro Alves wrote:
> > > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > > > > In the original code, is there a case when stop_pc != registers.pc?
> > > > 
> > > > Here,
> > > > 
> > > > <stopped at 0x1234, thread 1>
> > > >  (gdb) set $pc = 0xf00
> > > >  (gdb) call func()
> > > 
> > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > incorrectly: for example, the check whether we are about to continue
> > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > 
> > This one I believe was the original intention.  The rationale being
> > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > because there's where you stopped; but, you'd want to hit a a breakpoint
> > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > 
> > Note, I'm not saying I agree with this.  I did say that probably nobody
> > would notice if we got rid of stop_pc.
> > 
> > > It seems to me just about every current user of stop_pc *really* wants
> > > to look at regcache_read_pc (get_current_regcache ()) ...
> 
> Is using read_pc instead OK with you?  It's what I had written already.
> 
> > I've been sneaking the idea of getting rid of stop_pc for a while now:
> >  http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html
> > 
> > In fact, I have a months old patch here that completelly removes stop_pc.
> > IIRC, there were no visible changes in the testsuite.  Say the word,
> > and I'll brush it up, regtest, submit it.
> 
> Here it is, it still applied cleanly.  It's smallish.  Regtested on
> x86-64-unknown-linux-gnu.
> 
> My original motivation was to get rid of the ugly checks
> in switch_to_thread, and to try to minimize the extra thread
> switching and register reads in non-stop mode.
> 
> I had held posting this when I wrote it, since I was not sure we'd not
> miss stop_pc in some case.
> 

I should say that I also considered going the other direction and
adding a stop_pc per thread for use in `proceed', while still
replacing most other references to stop_pc by read_pc.

Say something like, in all-stop mode, the thread that hit the breakpoint
would have stop_pc set to read_pc, and all the other threads would have it
set to say (CORE_ADDR) -1.  This was to consider the following case:

Say where you're debugging an inferior with 2 threads, and two distinct breakpoints
installed.  thread 1 has just reported a hit on breakpoint 1; and thread 2
happens to have hit breakpoint 2 simultaneously, but, since events are serialized,
linux-nat.c:cancel_breakpoint took action, so thread 2 got the PC rolled back.

The user does:

(gdb) b foo
<bpkt 1>
(gdb) b bar
<bpkt 2>
(gdb) continue
<thread 1, bpk1 1 hit>
(gdb) thread 2
<GDB reads stop_pc of thread 2, which is now pointing at a breakpoint 2>
(gdb) delete 1

 proceed'ing now doesn't need to step over
 breakpoint 1 (prepare_to_proceed will do nothing), since breakpoint 1
 is gone.

(gdb) continue

At this point, current GDB will step over breakpoint 2, although it
was never reported as hit.

proceed:
  if (addr == (CORE_ADDR) -1)
    {
      if (pc == stop_pc && breakpoint_here_p (pc) 
          ^^^^^^^^^^^^^

If this was instead tp->stop_pc, and it was -1 at this point, because
thread 2 was not the thread that reported the last event, breakpoint 2
would not be missed.

-- 
Pedro Alves