[rfc] Do not call read_pc in startup_inferior

[rfc] Do not call read_pc in startup_inferior

[Ulrich Weigand]

Hello,

a while ago, I committed a patch to avoid calling wait_for_inferior
in startup_inferior, so as to avoid accessing inferior register state
at a time where the target's actual register layout has not yet been
determined (via target_find_description).

However, startup_inferior still contains a read_pc call to retrieve
the initial value of stop_pc -- this of course runs into the same
problem.

The patch below removes the read_pc call from startup_inferior, and
instead determines the initial stop_pc value in post_create_inferior,
after the register layout has been finalized.

Tested on powerpc64-linux.

Any comments or objections to this?   Otherwise, I'm planning on 
committing this patch within the next couple of days.

Bye,
Ulrich


ChangeLog:

	* fork-child.c (startup_inferior): Move setting stop_pc ...
	* infcmd.c (post_create_inferior): ... to here.


Index: gdb-head/gdb/fork-child.c
===================================================================
--- gdb-head.orig/gdb/fork-child.c
+++ gdb-head/gdb/fork-child.c
@@ -530,8 +530,6 @@ startup_inferior (int ntraps)
 
   /* Mark all threads non-executing.  */
   set_executing (pid_to_ptid (-1), 0);
-
-  stop_pc = read_pc ();
 }
 
 /* Implement the "unset exec-wrapper" command.  */
Index: gdb-head/gdb/infcmd.c
===================================================================
--- gdb-head.orig/gdb/infcmd.c
+++ gdb-head/gdb/infcmd.c
@@ -391,6 +391,9 @@ post_create_inferior (struct target_ops 
      don't need to.  */
   target_find_description ();
 
+  /* Now that we know the register layout, retrieve current PC.  */
+  stop_pc = read_pc ();
+
   /* If the solist is global across processes, there's no need to
      refetch it here.  */
   if (exec_bfd && !gdbarch_has_global_solist (target_gdbarch))
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Do not call read_pc in startup_inferior

[Daniel Jacobowitz]

On Tue, Apr 28, 2009 at 06:37:12PM +0200, Ulrich Weigand wrote:
> Hello,
> 
> a while ago, I committed a patch to avoid calling wait_for_inferior
> in startup_inferior, so as to avoid accessing inferior register state
> at a time where the target's actual register layout has not yet been
> determined (via target_find_description).
> 
> However, startup_inferior still contains a read_pc call to retrieve
> the initial value of stop_pc -- this of course runs into the same
> problem.
> 
> The patch below removes the read_pc call from startup_inferior, and
> instead determines the initial stop_pc value in post_create_inferior,
> after the register layout has been finalized.

You're moving the call from a native "run" only routine, to an
all-targets routine.  That made me curious so I went looking... what
relies on this setting?  Anything?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Do not call read_pc in startup_inferior

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Tue, Apr 28, 2009 at 06:37:12PM +0200, Ulrich Weigand wrote:
> > Hello,
> > 
> > a while ago, I committed a patch to avoid calling wait_for_inferior
> > in startup_inferior, so as to avoid accessing inferior register state
> > at a time where the target's actual register layout has not yet been
> > determined (via target_find_description).
> > 
> > However, startup_inferior still contains a read_pc call to retrieve
> > the initial value of stop_pc -- this of course runs into the same
> > problem.
> > 
> > The patch below removes the read_pc call from startup_inferior, and
> > instead determines the initial stop_pc value in post_create_inferior,
> > after the register layout has been finalized.
> 
> You're moving the call from a native "run" only routine, to an
> all-targets routine.  That made me curious so I went looking... what
> relies on this setting?  Anything?

It doesn't seem a lot relies on it; the solib_create_inferior_hook
might, but this is the case only for solib-sunos.c (which I guess
could be changed to use regcache_read_pc).

The only other potentially user-visible change seems to be that
"info program" will report "Program stopped at ..." giving the
proper entry point address.

In any case, most create_inferior implementations either call
startup_inferior, or otherwise set stop_pc e.g. by calling into
wait_for_inferior (in the latter case it shouldn't hurt to set
it again).

There are some targets that currently do not appear to set stop_pc:
the remote-extended mode, NTO, and some monitor targets.  These
would see a difference in "info program" output due to my patch
(but the new behaviour should be preferable, I guess) ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Do not call read_pc in startup_inferior

[Pedro Alves]

On Wednesday 29 April 2009 13:32:56, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > On Tue, Apr 28, 2009 at 06:37:12PM +0200, Ulrich Weigand wrote:
> > > Hello,
> > > 
> > > a while ago, I committed a patch to avoid calling wait_for_inferior
> > > in startup_inferior, so as to avoid accessing inferior register state
> > > at a time where the target's actual register layout has not yet been
> > > determined (via target_find_description).
> > > 
> > > However, startup_inferior still contains a read_pc call to retrieve
> > > the initial value of stop_pc -- this of course runs into the same
> > > problem.
> > > 
> > > The patch below removes the read_pc call from startup_inferior, and
> > > instead determines the initial stop_pc value in post_create_inferior,
> > > after the register layout has been finalized.
> > 
> > You're moving the call from a native "run" only routine, to an
> > all-targets routine.  That made me curious so I went looking... what
> > relies on this setting?  Anything?
> 
> It doesn't seem a lot relies on it; the solib_create_inferior_hook
> might, but this is the case only for solib-sunos.c (which I guess
> could be changed to use regcache_read_pc).
> 
> The only other potentially user-visible change seems to be that
> "info program" will report "Program stopped at ..." giving the
> proper entry point address.
> In any case, most create_inferior implementations either call
> startup_inferior, or otherwise set stop_pc e.g. by calling into
> wait_for_inferior (in the latter case it shouldn't hurt to set
> it again).
> 
> There are some targets that currently do not appear to set stop_pc:
> the remote-extended mode, NTO, and some monitor targets.  These
> would see a difference in "info program" output due to my patch
> (but the new behaviour should be preferable, I guess) ...
> 

Actually, currently it is hard to stop at the entry point
in most targets.  `proceed' will step over a breakpoint set right
at the entry point exactly due to this stop_pc == current pc, even
if it was never hit.  This leads to people using the "set breakpoint
at `entry point' + 1, instead of `entry point'" trick.

Maybe we should make run_command_1
call `proceed (current_pc, TARGET_SIGNAL_0, 0)' instead of
`proceed (-1, TAR...)'.  This would (partially) fix the bpkt at
entry issue, while also making sure that `proceed' isn't faced with a
completely random random stop_pc if we pass it -1.

(I believe there are other problems that make GDB ignore
breakpoints set at the entry point, e.g., if it happens to
be the same place we have a BPSTAT_WHAT_CHECK_SHLIBS breakpoint.)

-- 
Pedro Alves

Re: [rfc] Do not call read_pc in startup_inferior

[Ulrich Weigand]

Pedro Alves wrote:

> Actually, currently it is hard to stop at the entry point
> in most targets.  `proceed' will step over a breakpoint set right
> at the entry point exactly due to this stop_pc == current pc, even
> if it was never hit.  This leads to people using the "set breakpoint
> at `entry point' + 1, instead of `entry point'" trick.
> 
> Maybe we should make run_command_1
> call `proceed (current_pc, TARGET_SIGNAL_0, 0)' instead of
> `proceed (-1, TAR...)'.  This would (partially) fix the bpkt at
> entry issue, while also making sure that `proceed' isn't faced with a
> completely random random stop_pc if we pass it -1.

I see.  It seems "stop_pc" isn't really the proper mechanism for this.
(See also the problems with skipping breakpoints in multi-threaded 
applications that Doug Evans has been working on ...)

What would you think about the following replacement for stop_pc:

- Per thread, maintain a "last stop reason" state that identifies
  *where* and *why* this thread stopped.

- We skip a breakpoint whenever we're about to restart a thread and
  its "last stop reason" is "breakpoint" with an address equal to
  the restart address (and there's still a breakpoint at this location).

- In "info program", display the last stop reason of the current
  thread (or maybe all threads?). 

- In solib-sunos.c, just use regcache_read_pc.

> (I believe there are other problems that make GDB ignore
> breakpoints set at the entry point, e.g., if it happens to
> be the same place we have a BPSTAT_WHAT_CHECK_SHLIBS breakpoint.)

Hmm, if we have two breakpoints at the same address, one that is
supposed to silently restart, and one that is supposed to be
reported to the user, shouldn't the second one always "win"?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Do not call read_pc in startup_inferior

[Pedro Alves]

On Thursday 30 April 2009 15:25:17, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > Actually, currently it is hard to stop at the entry point
> > in most targets.  `proceed' will step over a breakpoint set right
> > at the entry point exactly due to this stop_pc == current pc, even
> > if it was never hit.  This leads to people using the "set breakpoint
> > at `entry point' + 1, instead of `entry point'" trick.
> > 
> > Maybe we should make run_command_1
> > call `proceed (current_pc, TARGET_SIGNAL_0, 0)' instead of
> > `proceed (-1, TAR...)'.  This would (partially) fix the bpkt at
> > entry issue, while also making sure that `proceed' isn't faced with a
> > completely random random stop_pc if we pass it -1.
> 

> I see.  It seems "stop_pc" isn't really the proper mechanism for this.
> (See also the problems with skipping breakpoints in multi-threaded 
> applications that Doug Evans has been working on ...)

Yeah, I'm also interested in this for multi-process as well.

> What would you think about the following replacement for stop_pc:

Before going further, I don't think you should hold on your current
patch?

> - Per thread, maintain a "last stop reason" state that identifies
>   *where* and *why* this thread stopped.
> - We skip a breakpoint whenever we're about to restart a thread and
>   its "last stop reason" is "breakpoint" with an address equal to
>   the restart address (and there's still a breakpoint at this location).

This logic with per-thread "last stop reason is breakpoint"
changes the current behaviour, in the wrong direction, I believe.
I don't think "breakpoint" or not is the correct check.  Consider:

 1. (gdb) step
 2. (gdb) step
 3. (gdb) break
 4. (gdb) continue

At 3. the current thread was not stopped at a breakpoint, yet,
in 4., `proceed' is expected to skip over the breakpoint you've
just set.

Here's a multi-threaded example, in all-stop mode:

 (gdb) break foo
breakpoint 1
 (gdb) continue
stop at breakpoint 1, thread 1
 (gdb) thread 2
 (gdb) break
breakpoint 2
 (gdb) continue
stop at breakpoint 2, thread 2

Currently, GDB will switch to thread 1, single step (this is
the prepare_to_proceed logic), and then resume.  `proceed' is
not involved in the latter resume, so we stop at 'breakpoint 2'.

If we had a per-thread stop_pc, we'd be able to make this
behave identically to the example below.  Compare the above
to:

 (gdb) break foo
breakpoint 1
 (gdb) continue
stop at breakpoint 1, thread 1
 (gdb) step <<<<<<<< (move out of the breakpoint)
 (gdb) thread 2
 (gdb) break
breakpoint 2
 (gdb) continue
<doesn't stop at breakpoint 2, because proceed steps over
break 2>

> - In "info program", display the last stop reason of the current
>   thread (or maybe all threads?). 

I think the intent of "info program" is to show the last stop reason,
no matter what thread you have selected.  I think "info threads" would
be a good place to show last stop reason of all threads, especially 
if you think of non-stop mode.  Most IDE's I know that show last
stop reason, put it next to the description of each thread, in the
thread tree/list.

> - In solib-sunos.c, just use regcache_read_pc.

Sounds good.

> > (I believe there are other problems that make GDB ignore
> > breakpoints set at the entry point, e.g., if it happens to
> > be the same place we have a BPSTAT_WHAT_CHECK_SHLIBS breakpoint.)
> 
> Hmm, if we have two breakpoints at the same address, one that is
> supposed to silently restart, and one that is supposed to be
> reported to the user, shouldn't the second one always "win"?

Close, but not quite.  Yes, we should stop for the second one, but,
we should still handle the BPSTAT_WHAT_CHECK_SHLIBS shlibs checking.
Currently, shlib event gets picked over a user breakpoint, and infrun.c
immediatelly resumes the target after hanling BPSTAT_WHAT_CHECK_SHLIBS.
The root problem here is that the shlib event breakpoint handling
should *not* be mutually exclusive with other breakpoints handling,
but, it is currently.  Funny enough, Doug has just tripped on
this: http://sourceware.org/ml/gdb-patches/2009-04/msg00801.html
In his case, when stopping for a shlib event, and if the watchpoint's
value has changed simultaneously,
bpstat_stop_status->bpstat_check_watchpoint->watchpoint_check
updates the watchpoint value, but, GDB resumes the inferior immediately,
even though there was a watchpoint hit to report...  Exactly the same
problem.

-- 
Pedro Alves

Re: [rfc] Do not call read_pc in startup_inferior

[Ulrich Weigand]

Pedro Alves wrote:
> On Thursday 30 April 2009 15:25:17, Ulrich Weigand wrote:
> > I see.  It seems "stop_pc" isn't really the proper mechanism for this.
> > (See also the problems with skipping breakpoints in multi-threaded 
> > applications that Doug Evans has been working on ...)
> 
> Yeah, I'm also interested in this for multi-process as well.
> 
> > What would you think about the following replacement for stop_pc:
> 
> Before going further, I don't think you should hold on your current
> patch?

Yes, I don't think my current patch makes anything worse here.
I'll check it in shortly.

> > - Per thread, maintain a "last stop reason" state that identifies
> >   *where* and *why* this thread stopped.
> > - We skip a breakpoint whenever we're about to restart a thread and
> >   its "last stop reason" is "breakpoint" with an address equal to
> >   the restart address (and there's still a breakpoint at this location).
> 
> This logic with per-thread "last stop reason is breakpoint"
> changes the current behaviour, in the wrong direction, I believe.
> I don't think "breakpoint" or not is the correct check.  Consider:
> 
>  1. (gdb) step
>  2. (gdb) step
>  3. (gdb) break
>  4. (gdb) continue
> 
> At 3. the current thread was not stopped at a breakpoint, yet,
> in 4., `proceed' is expected to skip over the breakpoint you've
> just set.

Hmmm, I was looking for consistency with the case where you
change PC (either explicitly, or by calling an inferior function
or triggering an inferior signal handler) to a location that
has a breakpoint set: in this case, GDB does stop immediately
and reports the breakpoint hit.

However, from a user interface perspective, the two cases are
probably different enough to deserve different handling: you
may want to step through the signal handler or inferior function,
so you want the breakpoint to hit.  On the other hand, in the
sequence you mention, having the "continue" stop immediately
doesn't seem to serve any useful purpose.

> If we had a per-thread stop_pc, we'd be able to make this
> behave identically to the example below.

OK.  Let's just go for a per-thread stop_pc and keep the rest
of the logic unchanged ...

> > - In "info program", display the last stop reason of the current
> >   thread (or maybe all threads?). 
> 
> I think the intent of "info program" is to show the last stop reason,
> no matter what thread you have selected.  I think "info threads" would
> be a good place to show last stop reason of all threads, especially 
> if you think of non-stop mode.  Most IDE's I know that show last
> stop reason, put it next to the description of each thread, in the
> thread tree/list.

Makes sense.  This means we'd have to remember the "thread we last
stopped at" so we can show its stop_pc.  I don't think we actually
have this information right now?

> > Hmm, if we have two breakpoints at the same address, one that is
> > supposed to silently restart, and one that is supposed to be
> > reported to the user, shouldn't the second one always "win"?
> 
> Close, but not quite.  Yes, we should stop for the second one, but,
> we should still handle the BPSTAT_WHAT_CHECK_SHLIBS shlibs checking.
> Currently, shlib event gets picked over a user breakpoint, and infrun.c
> immediatelly resumes the target after hanling BPSTAT_WHAT_CHECK_SHLIBS.
> The root problem here is that the shlib event breakpoint handling
> should *not* be mutually exclusive with other breakpoints handling,
> but, it is currently.  Funny enough, Doug has just tripped on
> this: http://sourceware.org/ml/gdb-patches/2009-04/msg00801.html
> In his case, when stopping for a shlib event, and if the watchpoint's
> value has changed simultaneously,
> bpstat_stop_status->bpstat_check_watchpoint->watchpoint_check
> updates the watchpoint value, but, GDB resumes the inferior immediately,
> even though there was a watchpoint hit to report...  Exactly the same
> problem.

Indeed.  That's another odd corner that I've never quite understood:
at the point where we look at breakpoints, the "bpstat" structure is
already able to represent that multiple causes of actions have occurred
at the same time.  But this detailed status is then reduced via bpstat_what
to a single action code ...   Maybe handle_inferior_event should
really make full use of all the information present in the bpstat.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Do not call read_pc in startup_inferior

[Pedro Alves]

On Monday 04 May 2009 18:38:59, Ulrich Weigand wrote:
> Makes sense.  This means we'd have to remember the "thread we last
> stopped at" so we can show its stop_pc.  I don't think we actually
> have this information right now?

We have `get_last_target_status', which infcmd.c:program_info
already uses.

> Indeed.  That's another odd corner that I've never quite understood:
> at the point where we look at breakpoints, the "bpstat" structure is
> already able to represent that multiple causes of actions have occurred
> at the same time.  But this detailed status is then reduced via bpstat_what
> to a single action code ...   Maybe handle_inferior_event should
> really make full use of all the information present in the bpstat.

Yeah...  I think that the basic idea was that most of the stop
actions in the bpstat structure are mutually exclusive.  E.g.,
set-longjmp-resume, step-resume, longjmp-resume, single-stepping,
user breakpoints.  If you're handling one of these, all the other
actions that could be associated with one of the other
simultaneous breakpoints should be cancelled, so in a sense, the 
ordering and reducing make some sense.  This breaks if the actions
aren't really mutually exclusive, like in the shlib event case.
Probably, something like eliminating BPSTAT_WHAT_CHECK_SHLIBS and
adding a check_shlibs boolean field to `struct bpstat_what'
isn't too far from fixing this case.

-- 
Pedro Alves

Re: [rfc] Do not call read_pc in startup_inferior

[Ulrich Weigand]

> 	* fork-child.c (startup_inferior): Move setting stop_pc ...
> 	* infcmd.c (post_create_inferior): ... to here.

I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com