Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Long story short:

After following a child, detaching from the parent,

('set follow-fork-mode child' + 'set detach-on-fork on')

here in this bit,

 infrun.c:resume():
  {
    ....
    follow_fork ();
    ...
    tp->stop_signal = TARGET_SIGNAL_0;
  }

... `tp' is no longer in the thread list (it was pointing at a thread
of the parent process, which we've detached from, hence no longer
in the thread list), so if the assignment above doesn't crash, it ends
up writing to who-knows-where.

With some local changes I was making, sometimes, `tp' happened to be left pointing
at linux_nat.c:lwp_list, and so that assignment above ended up clearing
lp->waitstatus.kind (of the first lwp in the list), which resulted in
GDB considering that the child process had
exited (because TARGET_SIGNAL_0 == TARGET_WAITKIND_EXITED).

This should fix intermittent foll-fork.exp foll-vfork.exp
fork-child-threads.exp failures.

Checked in.

-- 
Pedro Alves

[-- Attachment #2: foll_fork_dang.diff --]
[-- Type: text/x-diff, Size: 1216 bytes --]

2008-11-20  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (resume): If following a fork, reread the current
	thread.  Avoid dereferencing a possibly dangling pointer.

---
 gdb/infrun.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-11-20 05:37:35.000000000 +0000
+++ src/gdb/infrun.c	2008-11-20 12:30:26.000000000 +0000
@@ -1053,6 +1053,9 @@ a command like `return' or `jump' to con
       pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
       if (follow_fork ())
 	should_resume = 0;
+
+      /* Following a fork may change inferior_ptid.  */
+      tp = inferior_thread ();
       break;
 
     case TARGET_WAITKIND_EXECD:
@@ -1148,11 +1151,11 @@ a command like `return' or `jump' to con
           displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf));
         }
 
-      target_resume (resume_ptid, step, sig);
-
       /* Avoid confusing the next resume, if the next stop/resume
 	 happens to apply to another thread.  */
       tp->stop_signal = TARGET_SIGNAL_0;
+
+      target_resume (resume_ptid, step, sig);
     }
 
   discard_cleanups (old_cleanups);

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Ulrich Weigand]

Pedro Alves wrote:

>  infrun.c:resume():
>   {
>     ....
>     follow_fork ();
>     ...
>     tp->stop_signal = TARGET_SIGNAL_0;
>   }
> 
> ... `tp' is no longer in the thread list (it was pointing at a thread
> of the parent process, which we've detached from, hence no longer
> in the thread list), so if the assignment above doesn't crash, it ends
> up writing to who-knows-where.

Funny, I was debugging the very same problem just yesterday ...
In my case, the wild store clobbered a struct breakpoint for the
thread event breakpoint.

>        pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
>        if (follow_fork ())
>  	should_resume = 0;
> +
> +      /* Following a fork may change inferior_ptid.  */
> +      tp = inferior_thread ();
>        break;

I guess the "regcache" and "gdbarch" variables now also contain stale
information.  That's not an actual problem right now, as regcache is
(currently) not used after this point, and gdbarch (today) will never
actually change -- but in order to reduce potential future problems,
I think those should also be reset.

Bye,
Ulrich

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

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Thursday 20 November 2008 14:28:24, Ulrich Weigand wrote:

> I guess the "regcache" and "gdbarch" variables now also contain stale
> information.  That's not an actual problem right now, as regcache is
> (currently) not used after this point, and gdbarch (today) will never
> actually change -- but in order to reduce potential future problems,
> I think those should also be reset.

Right, how about the attached?

-- 
Pedro Alves

[-- Attachment #2: foll_fork_dang2.diff --]
[-- Type: text/x-diff, Size: 1241 bytes --]

2008-11-20  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (resume): If following a fork, also reset regcache,
	gdbarch and pc.

---
 gdb/infrun.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-11-20 14:59:05.000000000 +0000
+++ src/gdb/infrun.c	2008-11-20 15:03:21.000000000 +0000
@@ -965,10 +965,13 @@ resume (int step, enum target_signal sig
 {
   int should_resume = 1;
   struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
+
+  /* Note that these must be reset if we follow a fork below.  */
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct thread_info *tp = inferior_thread ();
   CORE_ADDR pc = regcache_read_pc (regcache);
+
   QUIT;
 
   if (debug_infrun)
@@ -1057,6 +1060,9 @@ a command like `return' or `jump' to con
       /* Following a child fork will change our notion of current
 	 thread.  */
       tp = inferior_thread ();
+      regcache = get_current_regcache ();
+      gdbarch = get_regcache_arch (regcache);
+      pc = regcache_read_pc (regcache);
       break;
 
     case TARGET_WAITKIND_EXECD:

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Ulrich Weigand]

Pedro Alves wrote:

> On Thursday 20 November 2008 14:28:24, Ulrich Weigand wrote:
> 
> > I guess the "regcache" and "gdbarch" variables now also contain stale
> > information.  That's not an actual problem right now, as regcache is
> > (currently) not used after this point, and gdbarch (today) will never
> > actually change -- but in order to reduce potential future problems,
> > I think those should also be reset.
> 
> Right, how about the attached?

> 2008-11-20  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (resume): If following a fork, also reset regcache,
> 	gdbarch and pc.

Looks good to me, thanks.

As a separate question, I'm wondering why this is the right place to
put the follow-fork logic anyway (and not in handle_inferior_event
like follow-exec ...).  Do you know the history of this?

Bye,
Ulrich

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

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Pedro Alves]

On Thursday 20 November 2008 16:36:58, Ulrich Weigand wrote:
> Looks good to me, thanks.

Thanks.  I'll check it in.

> As a separate question, I'm wondering why this is the right place to
> put the follow-fork logic anyway (and not in handle_inferior_event
> like follow-exec ...).  Do you know the history of this?

To be able to decide if you want to follow a child or a parent
when you catch a fork with "catch fork".

There's nothing to decide to follow or not on a "catch exec", the
image has already been replaced when you get the event, so gdb must
load the new symbols before dropping to the user/cli.

-- 
Pedro Alves

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Ulrich Weigand]

Pedro Alves wrote:
> On Thursday 20 November 2008 16:36:58, Ulrich Weigand wrote:
> > As a separate question, I'm wondering why this is the right place to
> > put the follow-fork logic anyway (and not in handle_inferior_event
> > like follow-exec ...).  Do you know the history of this?
> 
> To be able to decide if you want to follow a child or a parent
> when you catch a fork with "catch fork".

Ah, I see.  Makes sense.

However, even given that we need to do it in "resume" -- why so late
in resume, after e.g. displaced stepping or software single-step
was already set up?   For example, isn't singlestep_ptid set to the
wrong value if we later decide to follow the child?

It seems to me it would make more sense to have that decision come
*first* -- and then we could use the correct thread_info and regcache
etc. pointers throughout.  I guess there may have been a good reason
to place the call where it is, but I don't see it off-hand ...

Bye,
Ulrich

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

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Pedro Alves]

On Thursday 20 November 2008 18:24:27, Ulrich Weigand wrote:

> However, even given that we need to do it in "resume" -- why so late
> in resume, after e.g. displaced stepping or software single-step
> was already set up?   For example, isn't singlestep_ptid set to the
> wrong value if we later decide to follow the child?
> 
> It seems to me it would make more sense to have that decision come
> *first* -- and then we could use the correct thread_info and regcache
> etc. pointers throughout.  I guess there may have been a good reason
> to place the call where it is, but I don't see it off-hand ...

Hmmm, right, good point.  I agree.

-- 
Pedro Alves

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Michael Snyder]

Pedro Alves wrote:
> Long story short:
> 
> After following a child, detaching from the parent,
> 
> ('set follow-fork-mode child' + 'set detach-on-fork on')
> 
> here in this bit,
> 
>  infrun.c:resume():
>   {
>     ....
>     follow_fork ();
>     ...
>     tp->stop_signal = TARGET_SIGNAL_0;
>   }
> 
> ... `tp' is no longer in the thread list (it was pointing at a thread
> of the parent process, which we've detached from, hence no longer
> in the thread list), so if the assignment above doesn't crash, it ends
> up writing to who-knows-where.
> 
> With some local changes I was making, sometimes, `tp' happened to be left pointing
> at linux_nat.c:lwp_list, and so that assignment above ended up clearing
> lp->waitstatus.kind (of the first lwp in the list), which resulted in
> GDB considering that the child process had
> exited (because TARGET_SIGNAL_0 == TARGET_WAITKIND_EXITED).
> 
> This should fix intermittent foll-fork.exp foll-vfork.exp
> fork-child-threads.exp failures.
> 
> Checked in.

Pedro,

I'm not sure if this change goes far enough.
If a multi-threaded program forks, only the currently
executing thread survives in the child.  All others are
left behind (and its not unlikely that the thread library
is left in an inconsistant state, possibly leading to
deadlocks).

We can't do anything about that, but we could, eg.,
invalidate all known debugger state having to do with
other threads.  Clear the gdb thread list and preserve
only the current thread.

What do you think?




> ------------------------------------------------------------------------
> 
> 2008-11-20  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (resume): If following a fork, reread the current
> 	thread.  Avoid dereferencing a possibly dangling pointer.
> 
> ---
>  gdb/infrun.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c	2008-11-20 05:37:35.000000000 +0000
> +++ src/gdb/infrun.c	2008-11-20 12:30:26.000000000 +0000
> @@ -1053,6 +1053,9 @@ a command like `return' or `jump' to con
>        pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
>        if (follow_fork ())
>  	should_resume = 0;
> +
> +      /* Following a fork may change inferior_ptid.  */
> +      tp = inferior_thread ();
>        break;
>  
>      case TARGET_WAITKIND_EXECD:
> @@ -1148,11 +1151,11 @@ a command like `return' or `jump' to con
>            displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf));
>          }
>  
> -      target_resume (resume_ptid, step, sig);
> -
>        /* Avoid confusing the next resume, if the next stop/resume
>  	 happens to apply to another thread.  */
>        tp->stop_signal = TARGET_SIGNAL_0;
> +
> +      target_resume (resume_ptid, step, sig);
>      }
>  
>    discard_cleanups (old_cleanups);

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Daniel Jacobowitz]

On Mon, Dec 01, 2008 at 10:58:31AM -0800, Michael Snyder wrote:
> I'm not sure if this change goes far enough.
> If a multi-threaded program forks, only the currently
> executing thread survives in the child.  All others are
> left behind (and its not unlikely that the thread library
> is left in an inconsistant state, possibly leading to
> deadlocks).

If you use fork () rather than syscall (SYS_fork), the thread library
ought to clean up first; this is a POSIX supported functionality, I
believe.  I know it works with glibc and on Solaris.  Of course,
Solaris has rfork too which copies threads...

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Dec 01, 2008 at 10:58:31AM -0800, Michael Snyder wrote:
>> I'm not sure if this change goes far enough.
>> If a multi-threaded program forks, only the currently
>> executing thread survives in the child.  All others are
>> left behind (and its not unlikely that the thread library
>> is left in an inconsistant state, possibly leading to
>> deadlocks).
> 
> If you use fork () rather than syscall (SYS_fork), the thread library
> ought to clean up first; this is a POSIX supported functionality, I
> believe.  I know it works with glibc and on Solaris.  Of course,
> Solaris has rfork too which copies threads...
> 

All right, but separate from that, shouldn't gdb clean up too?
At least do a prune-threads?

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Pedro Alves]

On Monday 01 December 2008 18:58:31, Michael Snyder wrote:

> I'm not sure if this change goes far enough.

This patch restored the same behaviour to what happened
when GDB still used context-switching.  No more, no less.
When you had context-switching, no matter which side of
the fork you followed, the current infrun context would have
been left to the thread that ended up being set as current.
Without it, you have to refetch a new pointer to the current
thread, as it may be a different thread that it was on entry.

> If a multi-threaded program forks, only the currently
> executing thread survives in the child.  
> All others are 
> left behind (and its not unlikely that the thread library
> is left in an inconsistant state, possibly leading to
> deadlocks).

You mean 'not unlikely' in thread library implementations
in several targets, or 'not unlikely' in current glibc/ntpl
implementations?  glibc is supposed to handle that gracefully.

> 
> We can't do anything about that, but we could, eg.,
> invalidate all known debugger state having to do with
> other threads.  Clear the gdb thread list and preserve
> only the current thread.

The new fork *is* considered a new inferior, and starts
with only one thread listed.  In mainline,
linux_nat_switch_fork always clears the thread list
and adds a new thread.  In a true multi-process linux 
native implementation (which can leave GDB attached to
both parent and child, and have them running
simultaneously (*)), we'd still want to consider the child
fork a new single-threaded inferior, and defer to
thread_db to find the thread's thread_db id (and eventually, any
other thread, in the child in case that changes in the future).
Other targets may behave differently, so this is target
side concern.

(*) - I'm working some on that.

-- 
Pedro Alves

Re: Fix foll-fork.exp foll-vfork.exp fork-child-threads.exp

[Michael Snyder]

Pedro Alves wrote:
> On Monday 01 December 2008 18:58:31, Michael Snyder wrote:
> 
>> I'm not sure if this change goes far enough.
> 
> This patch restored the same behaviour to what happened
> when GDB still used context-switching.  No more, no less.
> When you had context-switching, no matter which side of
> the fork you followed, the current infrun context would have
> been left to the thread that ended up being set as current.
> Without it, you have to refetch a new pointer to the current
> thread, as it may be a different thread that it was on entry.
> 
>> If a multi-threaded program forks, only the currently
>> executing thread survives in the child.  
>> All others are 
>> left behind (and its not unlikely that the thread library
>> is left in an inconsistant state, possibly leading to
>> deadlocks).
> 
> You mean 'not unlikely' in thread library implementations
> in several targets, or 'not unlikely' in current glibc/ntpl
> implementations?  glibc is supposed to handle that gracefully.
> 
>> We can't do anything about that, but we could, eg.,
>> invalidate all known debugger state having to do with
>> other threads.  Clear the gdb thread list and preserve
>> only the current thread.
> 
> The new fork *is* considered a new inferior, and starts
> with only one thread listed.  In mainline,
> linux_nat_switch_fork always clears the thread list
> and adds a new thread.  In a true multi-process linux 
> native implementation (which can leave GDB attached to
> both parent and child, and have them running
> simultaneously (*)), we'd still want to consider the child
> fork a new single-threaded inferior, and defer to
> thread_db to find the thread's thread_db id (and eventually, any
> other thread, in the child in case that changes in the future).
> Other targets may behave differently, so this is target
> side concern.
> 
> (*) - I'm working some on that.

OK, sounds like you've already thought it through then.