Re: [PATCH?] GDB HEAD (partly) broken for GNU/Hurd

[Pedro Alves <pedro@codesourcery.com>]

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

Hi, and sorry I didn't reply back sooner.

On Monday 13 October 2008 17:40:25, Ulrich Weigand wrote:
> Thomas Schwinge wrote:
> 
> > Ha, I, myself, am the GDB guru here ;-)!  I had a look at the log again,
> > experimented some more, and finally got it going with the following
> > patch.  However, I have absolutely no idea whether that is correct in all
> > cases, etc.  Should perhaps target_wait (a.k.a. gnu-nat.c's gnu_wait) be
> > doing that?

Thanks Thomas :-)  One thing I asked myself was, if gnu-nat.c couldn't be using
the port's id as thread ids instead of a locally auto-generated number.  Maybe
the thread id of the main thread would be preserved across execs this way, but,
this is off-scope for now.

> 
> Adding switch_to_thread at this location seems correct to me (even though
> it should be a no-op on most targets).

You shouldn't call it on a few cases, namely:

- TARGET_WAITKIND_IGNORE, TARGET_WAITKIND_SIGNALLED and
TARGET_WAITKIND_EXITED.  The ptid returned isn't a thread, so
you could hit an internal error inside e.g.,
switch_to_thread->is_exited,is_running.

The _IGNORE case is actually broken today, as we shouldn't issue either
a target_resume or set_executing (..., 0) in that case.  Sorry I missed it
before.  Here's the similar handling in handle_inferior_event for reference:

  if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
    {
      /* Mark the non-executing threads accordingly.  */
      if (!non_stop
 	  || ecs->ws.kind == TARGET_WAITKIND_EXITED
 	  || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED)
 	set_executing (pid_to_ptid (-1), 0);
      else
 	set_executing (ecs->ptid, 0);
    }

- TARGET_WAITKIND_SPURIOUS.  This one's a bit tricky, as some targets
  may return ptid(-1,0,0) with it.  It looked like procfs.c can
  in some cases last time I tried looking at cleaning that up.
  handle_inferior_event doesn't switch context on it, so we could
  do the same.  But, we'll most probably not see this event
  here, I hope.

- If target_wait returns ptid(-1).  It shouldn't be returning that
  in any case where we should call switch_to_thread, though.  We
  can see this happening in the the TARGET_WAITKIND_IGNORE case --- don't
  switch threads in this case anyway.

- calling switch_to_thread before calling set_executing (..., 0),
  has the effect of not reading stop_pc.  I guess that's what we really
  want here?  Thus we avoid touching the shell's registers until we
  return from startup_inferior, which I undertand was one of the
  goals of this new loop.

Notice that we're assuming that handle_inferior_event()'s
new_thread_event handling isn't needed here in any platform.

> 
> Could you test your patch on a non-Hurd target to make sure, anyway?
> 
> > +  /* TODO.  How to keep this synchronized with gnu-nat.c's own counting?  */
> 
> Hmm.  It would appear that "set exec-wrapper" is currently broken with
> the gnu-nat.c target, right?

Yeah, it appears so.  Don't know if it's possible to get rid of the local
pending execs handling in gnu-nat.c.  An alternative would be to make
pending_execs a property of inferior.h:`struct inferior' instead of of
gnu-nat.c:`struct inf'.

(I also notice that when going through the shell in non-stop
mode, it would be more correct to resume all threads --- we
don't want non-stop and its scheduler-locking to apply to the shell.
Basically, non-stop should be off if there are pending execs.
This was an existing issue, and doesn't affect linux today, so I'll
just ignore that for now, as it needs more tweaking to fix.)

( hope not many issues like this appear, as we're doing more
duplication of handle_inferior_event logic :-( )

What do you guys think?  Thomas, could you try the attached patch
on the Hurd, please?  I just gave it a spin on x86-64-unknown-linux-gnu
without regressions.

-- 
Pedro Alves

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

2008-10-13  Pedro Alves  <pedro@codesourcery.com>

	* fork-child.c (startup_inferior): Only set threads not-executing
	after getting all the pending execs.  On TARGET_WAITKIND_IGNORE,
	keep waiting, don't resume.  On all other cases but
	TARGET_WAITKIND_SIGNALLED and TARGET_WAITKIND_EXITED, switch to
	the event ptid.

---
 gdb/fork-child.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

Index: src/gdb/fork-child.c
===================================================================
--- src.orig/gdb/fork-child.c	2008-10-13 18:34:10.000000000 +0100
+++ src/gdb/fork-child.c	2008-10-13 19:12:03.000000000 +0100
@@ -434,21 +434,18 @@ startup_inferior (int ntraps)
     {
       int resume_signal = TARGET_SIGNAL_0;
       ptid_t resume_ptid;
+      ptid_t event_ptid;
 
       struct target_waitstatus ws;
       memset (&ws, 0, sizeof (ws));
-      resume_ptid = target_wait (pid_to_ptid (-1), &ws);
+      event_ptid = target_wait (pid_to_ptid (-1), &ws);
 
-      /* Mark all threads non-executing.  */
-      set_executing (pid_to_ptid (-1), 0);
-
-      /* In all-stop mode, resume all threads.  */
-      if (!non_stop)
-	resume_ptid = pid_to_ptid (-1);
+      if (ws.kind == TARGET_WAITKIND_IGNORE)
+	/* The inferior didn't really stop, keep waiting.  */
+	continue;
 
       switch (ws.kind)
 	{
-	  case TARGET_WAITKIND_IGNORE:
 	  case TARGET_WAITKIND_SPURIOUS:
 	  case TARGET_WAITKIND_LOADED:
 	  case TARGET_WAITKIND_FORKED:
@@ -456,6 +453,7 @@ startup_inferior (int ntraps)
 	  case TARGET_WAITKIND_SYSCALL_ENTRY:
 	  case TARGET_WAITKIND_SYSCALL_RETURN:
 	    /* Ignore gracefully during startup of the inferior.  */
+	    switch_to_thread (event_ptid);
 	    break;
 
 	  case TARGET_WAITKIND_SIGNALLED:
@@ -480,13 +478,21 @@ startup_inferior (int ntraps)
 	    /* Handle EXEC signals as if they were SIGTRAP signals.  */
 	    xfree (ws.value.execd_pathname);
 	    resume_signal = TARGET_SIGNAL_TRAP;
+	    switch_to_thread (event_ptid);
 	    break;
 
 	  case TARGET_WAITKIND_STOPPED:
 	    resume_signal = ws.value.sig;
+	    switch_to_thread (event_ptid);
 	    break;
 	}
 
+      /* In all-stop mode, resume all threads.  */
+      if (!non_stop)
+	resume_ptid = pid_to_ptid (-1);
+      else
+	resume_ptid = event_ptid;
+
       if (resume_signal != TARGET_SIGNAL_TRAP)
 	{
 	  /* Let shell child handle its own signals in its own way.  */
@@ -519,6 +525,14 @@ startup_inferior (int ntraps)
 	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
 	}
     }
+
+  /* Mark all threads non-executing.  */
+  set_executing (pid_to_ptid (-1), 0);
+
+  /* we called switch_to_thread above when threads were still tagged
+     as `executing', which had the effect of avoiding to fetch the
+     shell's registers, hence stop_pc as well --- read it now.  */
+  stop_pc = read_pc ();
 }
 
 /* Implement the "unset exec-wrapper" command.  */