GDB HEAD (partly) broken for GNU/Hurd

GDB HEAD (partly) broken for GNU/Hurd

[Thomas Schwinge]

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

Hello!

Some of the changes that have been installed between gdb_6_8-branch and
HEAD cause GDB to no longer function properly on GNU/Hurd under certain
circumstances.

    tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
    GNU gdb 6.8.0.20081008-cvs
    [...]
    This GDB was configured as "i386-unknown-gnu0.3"...
    (gdb) r
    Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
    [New thread 8112.1]
    [New thread 8112.2]
    [New thread 8112.3]
    [New thread 8112.4]
    [New thread 8112.5]
    
    Program received signal SIGSEGV, Segmentation fault.
    convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
    579     argp.h: No such file or directory.
            in argp.h

    tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
    GNU gdb (GDB) 6.8.50.20081009-cvs
    [...]
    (gdb) r
    Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
    Can't fetch registers from thread bogus thread id 1: No such thread

Both have been built (natively) on the same up-to-date Debian GNU/Hurd
system.

For easy reproduction, I can publish the faulting binary.


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: GDB HEAD (partly) broken for GNU/Hurd

[Alfred M. Szmidt]

   Some of the changes that have been installed between gdb_6_8-branch
   and HEAD cause GDB to no longer function properly on GNU/Hurd under
   certain circumstances.

Could you try to track them down?

I am currently out of town without access to a machine running the GNU
system.  Have you asked for cvs access?

Commit access for me? (was: GDB HEAD (partly) broken for GNU/Hurd)

[Thomas Schwinge]

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

Hello!

The current maintainer for the Hurd bits of GDB, Alfred M. Szmidt, would
like me to also get direct commit access for GDB, so that I can, e.g.,
install the two pending patches I sent to the gdb-patches list.  What do
the GDB people think?

On Thu, Oct 09, 2008 at 07:44:21AM -0400, Alfred M. Szmidt wrote:
> I am currently out of town without access to a machine running the GNU
> system.  Have you asked for cvs access?


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: Commit access for me? (was: GDB HEAD (partly) broken for GNU/Hurd)

[Joel Brobecker]

> The current maintainer for the Hurd bits of GDB, Alfred M. Szmidt, would
> like me to also get direct commit access for GDB, so that I can, e.g.,
> install the two pending patches I sent to the gdb-patches list.  What do
> the GDB people think?

Sure! The criteria for having Write After Approval access is to send
one good patch. I verified that your copyright assignement papers are
already taken care of, so do you have an account on sourceware.org?
If not, then please fill out the following form:

    http://www.sourceware.org/cgi-bin/pdw/ps_form.cgi

You can list me as the person who approved your request. If you already
have an account, then please send a request to overseers@.

Once your account is setup or your access has been setup, follow the
instructions to change your read-only sandbox into a write one, or
check out a write version of the GDB sources, and then commit a change
in MAINTAINERS adding you to the Write After Approval section.  Please
remember to post the associated patch.

-- 
Joel

Re: GDB HEAD (partly) broken for GNU/Hurd

[Pedro Alves]

A Thursday 09 October 2008 10:34:24, Thomas Schwinge escreveu:
> Hello!
> 
> Some of the changes that have been installed between gdb_6_8-branch and
> HEAD cause GDB to no longer function properly on GNU/Hurd under certain
> circumstances.
> 
>     tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
>     GNU gdb 6.8.0.20081008-cvs
>     [...]
>     This GDB was configured as "i386-unknown-gnu0.3"...
>     (gdb) r
>     Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
>     [New thread 8112.1]
>     [New thread 8112.2]
>     [New thread 8112.3]
>     [New thread 8112.4]
>     [New thread 8112.5]
>     
>     Program received signal SIGSEGV, Segmentation fault.
>     convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
>     579     argp.h: No such file or directory.
>             in argp.h
> 
>     tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
>     GNU gdb (GDB) 6.8.50.20081009-cvs
>     [...]
>     (gdb) r
>     Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
>     Can't fetch registers from thread bogus thread id 1: No such thread
> 
> Both have been built (natively) on the same up-to-date Debian GNU/Hurd
> system.
> 
> For easy reproduction, I can publish the faulting binary.
> 

Could you check what caused the breakage?  It *may* have been the ptid
changes I made, or not.

2008-09-08  Pedro Alves  <pedro@codesourcery.com>

        Use ptid_t.tid to store thread ids instead of ptid_t.pid.

        * gnu-nat.c (inf_validate_procs): If this is the first time we're
        seeing a thread id, extend the main thread's ptid.  If we still
        have pending execs, don't be verbose about new threads.
        (gnu_wait, gnu_resume, gnu_attach, gnu_thread_alive)
        (gnu_pid_to_str, cur_thread, sig_thread_cmd): Adjust.
        * i386gnu-nat.c (gnu_fetch_registers, gnu_store_registers):
        Adjust.

If nothing obvious turns up, could you open up a bug report
with a testcase and instructions, please?

-- 
Pedro Alves

Re: GDB HEAD (partly) broken for GNU/Hurd

[Thomas Schwinge]

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

Hello!

On Thu, Oct 09, 2008 at 12:55:16PM +0100, Pedro Alves wrote:
> A Thursday 09 October 2008 10:34:24, Thomas Schwinge escreveu:
> > Some of the changes that have been installed between gdb_6_8-branch and
> > HEAD cause GDB to no longer function properly on GNU/Hurd under certain
> > circumstances.
> > 
> >     tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
> >     GNU gdb 6.8.0.20081008-cvs
> >     [...]
> >     This GDB was configured as "i386-unknown-gnu0.3"...
> >     (gdb) r
> >     Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
> >     [New thread 8112.1]
> >     [New thread 8112.2]
> >     [New thread 8112.3]
> >     [New thread 8112.4]
> >     [New thread 8112.5]
> >     
> >     Program received signal SIGSEGV, Segmentation fault.
> >     convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
> >     579     argp.h: No such file or directory.
> >             in argp.h
> > 
> >     tschwinge@zoobar:~/tmp/gdb/HEAD.build $ gdb/gdb ~/tmp/n1/hurd/ext2fs.static
> >     GNU gdb (GDB) 6.8.50.20081009-cvs
> >     [...]
> >     (gdb) r
> >     Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
> >     Can't fetch registers from thread bogus thread id 1: No such thread
> > 
> > Both have been built (natively) on the same up-to-date Debian GNU/Hurd
> > system.
> > 
> > For easy reproduction, I can publish the faulting binary.
> 
> Could you check what caused the breakage?  It *may* have been the ptid
> changes I made, or not.
> 
> 2008-09-08  Pedro Alves  <pedro@codesourcery.com>
> 
>         Use ptid_t.tid to store thread ids instead of ptid_t.pid.

That's not it.  But I bisected it down to this change:

2008-09-11  Ulrich Weigand  <uweigand@de.ibm.com>

	* fork-child.c (startup_inferior): Use target_wait and target_resume
	directly instead of calling wait_for_inferior / resume.

On HEAD, when undoing this change (and additionally commenting out the
two ``stop_soon = X'' lines in that file), things are fine again.

As most of GDB's internals are a big black box to me, I need help here.
:-)

Here is the ext2fs.static binary (bzip2ed) exhibiting the above problem:
<http://www.thomas.schwinge.homeip.net/tmp/ext2fs.static.bz2>.  (Please
bear with the poor upload bandwidth on my side.)


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: GDB HEAD (partly) broken for GNU/Hurd

[Pedro Alves]

On Saturday 11 October 2008 00:27:06, Thomas Schwinge wrote:
> On HEAD, when undoing this change (and additionally commenting out the
> two ``stop_soon = X'' lines in that file), things are fine again.
> 
> As most of GDB's internals are a big black box to me, I need help here.
> :-)
> 

Eh, I did point out at the time of that change that gnu-nat.c does
things a bit different.  :-)

Off-hand advice:

One thing that the hurd has a bit different, is that we have
multi-threading when going through the shell.

Could it be that target_wait is returning a specific ptid here:

fork_child.c:startup_inferior:

  while (1)
    {
      int resume_signal = TARGET_SIGNAL_0;
      ptid_t resume_ptid;

      struct target_waitstatus ws;
      memset (&ws, 0, sizeof (ws));
      resume_ptid = target_wait (pid_to_ptid (-1), &ws);
      ^^^^^^^^^^^


Hence this a bit below:

	  if (--pending_execs == 0)
	    break;

	  /* Just make it go on.  */
	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
                         ^^^^^^^^^^^
	}
    }

Doesn't resume the whole shell?

If you make this change:
-     target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
+     target_resume (minus_one_ptid, 0, TARGET_SIGNAL_0);

The other thing I suggest to look at, is to make sure the
local `pending_execs' and the `gnu-nat.c:struct inf'::pending_execs
aren't in conflict, but it doesn't look like it.

Hope this helps.

-- 
Pedro Alves

Re: GDB HEAD (partly) broken for GNU/Hurd

[Thomas Schwinge]

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

Hello!

On Sat, Oct 11, 2008 at 12:47:39AM +0100, Pedro Alves wrote:
> On Saturday 11 October 2008 00:27:06, Thomas Schwinge wrote:
> > On HEAD, when undoing this change (and additionally commenting out the
> > two ``stop_soon = X'' lines in that file), things are fine again.

At the end of this email I'll append the patch I use to get it going
again.  Obviously this can't be installed as-is.  :-)

> Eh, I did point out at the time of that change that gnu-nat.c does
> things a bit different.  :-)

Yes, I can see that:
<http://sourceware.org/ml/gdb-patches/2008-09/msg00097.html>

> Off-hand advice:
> 
> One thing that the hurd has a bit different, is that we have
> multi-threading when going through the shell.
> 
> Could it be that target_wait is returning a specific ptid here:
> 
> fork_child.c:startup_inferior:
> 
>   while (1)
>     {
>       int resume_signal = TARGET_SIGNAL_0;
>       ptid_t resume_ptid;
> 
>       struct target_waitstatus ws;
>       memset (&ws, 0, sizeof (ws));
>       resume_ptid = target_wait (pid_to_ptid (-1), &ws);
>       ^^^^^^^^^^^
> 
> 
> Hence this a bit below:
> 
> 	  if (--pending_execs == 0)
> 	    break;
> 
> 	  /* Just make it go on.  */
> 	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
>                          ^^^^^^^^^^^
> 	}
>     }
> 
> Doesn't resume the whole shell?

But as I see things we always have ``non_stop == 0'' and thus
``resume_ptid = pid_to_ptid (-1)'', so that should be fine, isn't it?

> If you make this change:
> -     target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
> +     target_resume (minus_one_ptid, 0, TARGET_SIGNAL_0);

This didn't help.

> The other thing I suggest to look at, is to make sure the
> local `pending_execs' and the `gnu-nat.c:struct inf'::pending_execs
> aren't in conflict, but it doesn't look like it.

Doesn't look like it, no.  For another issue, see the comment in the
patch below.

> Hope this helps.

Unfortunately no luck so far.  wait_for_inferior / handle_inferior_event
(which was used in the old code) is too complex as to be quickly
understandable for me.  And I guess I'm estimating correctly that it's
``simply'' some side-effect of these that the old code works, while the
new doesn't?  Perhaps having a look at the logs I appended below some of
you GDB gurus is able to spot the obvious?


Index: fork-child.c
===================================================================
RCS file: /cvs/src/src/gdb/fork-child.c,v
retrieving revision 1.45
diff -u -p -r1.45 fork-child.c
--- fork-child.c	22 Sep 2008 15:16:51 -0000	1.45
+++ fork-child.c	11 Oct 2008 16:59:47 -0000
@@ -128,7 +128,7 @@ fork_inferior (char *exec_file_arg, char
   static char default_shell_file[] = SHELL_FILE;
   int len;
   /* Set debug_fork then attach to the child while it sleeps, to debug. */
-  static int debug_fork = 0;
+  static int debug_fork = 1;  /* Not functionally needed, but helpful for readable logging messages.  */
   /* This is set to the result of setpgrp, which if vforked, will be visible
      to you in the parent process.  It's only used by humans for debugging.  */
   static int debug_setpgrp = 657473;
@@ -427,14 +427,23 @@ startup_inferior (int ntraps)
      have stopped one instruction after execing the shell.  Here we
      must get it up to actual execution of the real program.  */
 
+  /* TODO.  How to keep this synchronized with gnu-nat.c's own counting?  */
   if (exec_wrapper)
     pending_execs++;
 
+#define NEW_CODE 0
+
+#if NEW_CODE
+#else
+  init_wait_for_inferior ();
+#endif
+
   while (1)
     {
       int resume_signal = TARGET_SIGNAL_0;
       ptid_t resume_ptid;
 
+#if NEW_CODE
       struct target_waitstatus ws;
       memset (&ws, 0, sizeof (ws));
       resume_ptid = target_wait (pid_to_ptid (-1), &ws);
@@ -486,6 +495,17 @@ startup_inferior (int ntraps)
 	    resume_signal = ws.value.sig;
 	    break;
 	}
+#else
+      struct thread_info *tp;
+
+      /* Make wait_for_inferior be quiet. */
+      current_inferior ()->stop_soon = STOP_QUIETLY;
+      wait_for_inferior (1);
+      tp = inferior_thread ();
+
+resume_signal = tp->stop_signal;
+resume_ptid = pid_to_ptid (-1);
+#endif
 
       if (resume_signal != TARGET_SIGNAL_TRAP)
 	{
@@ -519,6 +539,10 @@ startup_inferior (int ntraps)
 	  target_resume (resume_ptid, 0, TARGET_SIGNAL_0);
 	}
     }
+#if NEW_CODE
+#else
+  current_inferior ()->stop_soon = NO_STOP_QUIETLY;
+#endif
 }
 
 /* Implement the "unset exec-wrapper" command.  */


Here is a debugging-enabled run of a thusly patched GDB HEAD:

    GNU gdb (GDB) 6.8.50.20081011-cvs
    [...]
    (gdb) set debug infrun 1
    (gdb) set debug target 1
    (gdb) r
    Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static
    infrun: wait_for_inferior (treat_exec_as_sigtrap=1)
    target_wait (-1, status) = 25830,   status->kind = stopped, signal = SIGTRAP
    target_fetch_registers (eip) = 701e0000 0x1e70 7792
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x1e70
    STOPPED_BY_WATCHPOINT () = 0
    infrun: context switch
    infrun: Switching context from bogus thread id 1 to Thread 25830.3
    target_fetch_registers (eip) = 701e0000 0x1e70 7792
    STOPPED_BY_WATCHPOINT () = 0
    infrun: quietly stopped
    infrun: stop_stepping
    target_terminal_init ()
    target_terminal_inferior ()
    target_resume (-1, continue, 0)
    infrun: wait_for_inferior (treat_exec_as_sigtrap=1)
    target_wait (-1, status) = 25830,   status->kind = stopped, signal = SIGTRAP
    target_fetch_registers (eip) = 30810408 0x8048130 134512944
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    infrun: stop_pc = 0x8048130
    STOPPED_BY_WATCHPOINT () = 0
    infrun: context switch
    infrun: Switching context from bogus thread id 3 to Thread 25830.4
    target_fetch_registers (eip) = 30810408 0x8048130 134512944
    STOPPED_BY_WATCHPOINT () = 0
    target_terminal_ours_for_output ()
    infrun: quietly stopped
    infrun: stop_stepping
    target_terminal_ours ()
    GNU:target_xfer_partial (9, target.xml, 0x843d140,  0x0,  0x0, 4095) = -1
    GNU:target_xfer_partial (5, (null), 0x843d140,	0x0,  0x0, 4096) = -1
    GNU:target_xfer_partial (5, (null), 0x843d140,	0x0,  0x0, 4096) = -1
    target_memory_map ()
    GNU:target_xfer_partial (2, (null), 0x15ff570,	0x0,  0x816ff14, 4) = 4, bytes =
     00 00 00 00
    GNU:target_xfer_partial (5, (null), 0x843d140,	0x0,  0x0, 4096) = -1
    GNU:target_xfer_partial (5, (null), 0x843d140,	0x0,  0x0, 4096) = -1
    GNU:target_xfer_partial (2, (null), 0x15ff510,	0x0,  0x816ff14, 4) = 4, bytes =
     00 00 00 00
    GNU:target_xfer_partial (2, (null), 0x15ff5d0,	0x0,  0x816ff14, 4) = 4, bytes =
     00 00 00 00
    GNU:target_xfer_partial (5, (null), 0x843d140,	0x0,  0x0, 4096) = -1
    infrun: proceed (addr=0xffffffff, signal=0, step=0)
    GNU:target_xfer_partial (2, (null), 0x836af1c,	0x0,  0x80deef0, 1) = 1, bytes = 55
    GNU:target_xfer_partial (2, (null), 0x0,  0x82d612b,  0x80deef0, 1) = 1, bytes = cc
    target_insert_breakpoint (0x80deef0, xxx) = 0
    infrun: resume (step=0, signal=0), trap_expected=0
    target_terminal_inferior ()
    target_resume (-1, continue, 0)
    infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
    [New Thread 25830.5]
    target_wait (-1, status) = 25830,   status->kind = stopped, signal = SIGSEGV
    infrun: infwait_normal_state
    infrun: TARGET_WAITKIND_STOPPED
    target_fetch_registers (eip) = 79440b08 0x80b4479 134956153
    infrun: stop_pc = 0x80b4479
    STOPPED_BY_WATCHPOINT () = 0
    STOPPED_BY_WATCHPOINT () = 0
    target_terminal_ours_for_output ()
    infrun: random signal 11
    target_terminal_ours_for_output ()
    
    Program received signal SIGSEGV, Segmentation fault.
    infrun: stop_stepping
    GNU:target_xfer_partial (2, (null), 0x0,  0x836af1c,  0x80deef0, 1) = 1, bytes = 55
    target_remove_breakpoint (0x80deef0, xxx) = 0
    target_terminal_ours ()
    target_fetch_registers (ebp) = 18f90101 0x101f918 16906520
    GNU:target_xfer_partial (2, (null), 0x83a567c,	0x0,  0x101f8ec, 4) = 4, bytes = bc f0 13 08
    GNU:target_xfer_partial (2, (null), 0x837b994,	0x0,  0x101f8e8, 4) = 4, bytes = 00 00 00 00
    GNU:target_xfer_partial (2, (null), 0x836e19c,	0x0,  0x101f8e4, 4) = 4, bytes = 00 00 00 00
    GNU:target_xfer_partial (2, (null), 0x836e25c,	0x0,  0x101f920, 4) = 4, bytes = e8 12 17 08
    GNU:target_xfer_partial (2, (null), 0x837b814,	0x0,  0x101f924, 4) = 4, bytes = d0 fa 01 01
    convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
    579	argp.h: No such file or directory.
    	in argp.h
    (gdb) info threads
    target_thread_alive (25830) = 1
    target_thread_alive (25830) = 1
    target_thread_alive (25830) = 0
    target_thread_alive (25830) = 0
    target_thread_alive (25830) = 0
    target_find_new_threads ()
    target_fetch_registers (eip) = 8c5a0b08 0x80b5a8c 134961804
      5 Thread 25830.5  0x080b5a8c in mach_msg_trap ()
    target_fetch_registers (eip) = 79440b08 0x80b4479 134956153
    target_fetch_registers (ebp) = 18f90101 0x101f918 16906520
    GNU:target_xfer_partial (2, (null), 0x838142c,	0x0,  0x101f8ec, 4) = 4, bytes = bc f0 13 08
    GNU:target_xfer_partial (2, (null), 0x837db74,	0x0,  0x101f8e8, 4) = 4, bytes = 00 00 00 00
    GNU:target_xfer_partial (2, (null), 0x83815fc,	0x0,  0x101f8e4, 4) = 4, bytes = 00 00 00 00
    GNU:target_xfer_partial (2, (null), 0x83816bc,	0x0,  0x101f920, 4) = 4, bytes = e8 12 17 08
    GNU:target_xfer_partial (2, (null), 0x83a2074,	0x0,  0x101f924, 4) = 4, bytes = d0 fa 01 01
    * 4 Thread 25830.4  convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579

... as compared to plain GDB HEAD:

    (gdb) set debug infrun 1
    (gdb) set debug target 1
    (gdb) r
    Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static 
    target_wait (-1, status) = 25945,   status->kind = stopped, signal = SIGTRAP
    target_terminal_init ()
    target_terminal_inferior ()
    target_resume (-1, continue, 0)
    target_wait (-1, status) = 25945,   status->kind = stopped, signal = SIGTRAP
    target_terminal_ours ()
    GNU:target_xfer_partial (9, target.xml, 0x843b360,  0x0,  0x0, 4095) = -1
    GNU:target_xfer_partial (5, (null), 0x843b360,  0x0,  0x0, 4096) = -1
    GNU:target_xfer_partial (5, (null), 0x843b360,  0x0,  0x0, 4096) = -1
    target_memory_map ()
    GNU:target_xfer_partial (2, (null), 0x15ff570,  0x0,  0x816ff14, 4) = 4, bytes =
     00 00 00 00
    target_terminal_ours ()
    Can't fetch registers from thread bogus thread id 1: No such thread


Thanks for your help!


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

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

[Thomas Schwinge]

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

Hello again!

On Sat, Oct 11, 2008 at 07:26:11PM +0200, I wrote:
> Unfortunately no luck so far.  wait_for_inferior / handle_inferior_event
> (which was used in the old code) is too complex as to be quickly
> understandable for me.  And I guess I'm estimating correctly that it's
> ``simply'' some side-effect of these that the old code works, while the
> new doesn't?  Perhaps having a look at the logs I appended below some of
> you GDB gurus is able to spot the obvious?

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?


Index: fork-child.c
===================================================================
RCS file: /cvs/src/src/gdb/fork-child.c,v
retrieving revision 1.45
diff -u -p -r1.45 fork-child.c
--- fork-child.c	22 Sep 2008 15:16:51 -0000	1.45
+++ fork-child.c	11 Oct 2008 17:34:33 -0000
@@ -427,6 +427,7 @@ startup_inferior (int ntraps)
      have stopped one instruction after execing the shell.  Here we
      must get it up to actual execution of the real program.  */
 
+  /* TODO.  How to keep this synchronized with gnu-nat.c's own counting?  */
   if (exec_wrapper)
     pending_execs++;
 
@@ -439,6 +440,8 @@ startup_inferior (int ntraps)
       memset (&ws, 0, sizeof (ws));
       resume_ptid = target_wait (pid_to_ptid (-1), &ws);
 
+      switch_to_thread (resume_ptid);
+
       /* Mark all threads non-executing.  */
       set_executing (pid_to_ptid (-1), 0);
 


    (gdb) r
    Starting program: /media/data/home/tschwinge/tmp/n1/hurd/ext2fs.static 
    [New Thread 27194.5]
    
    Program received signal SIGSEGV, Segmentation fault.
    convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
    579     argp.h: No such file or directory.
            in argp.h
    (gdb) bt
    #0  convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579
    #1  0x080b4675 in convert_options (argp=0x101f980, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp-parse.c:407
    #2  0x080b4834 in __argp_parse (argp=0x101f980, argc=1, argv=0x101fc14, flags=<value optimized out>, end_index=0x0, input=0x101fb3c) at argp-parse.c:435
    #3  0x08056dad in diskfs_init_main (startup_argp=0x0, argc=1, argv=0x101fc14, store_parsed=0x8153284, bootstrap=0x101fb8c) at ../../hurd.work/libdiskfs/init-main.c:37
    #4  0x0804bd9b in main (argc=1, argv=0x101fc14) at ../../hurd.work/ext2fs/ext2fs.c:172
    (gdb) info threads 
      5 Thread 27194.5  0x080b5a8c in mach_msg_trap ()
    * 4 Thread 27194.4  convert_options (argp=0x813f0bc, parent=0x0, parent_index=0, group=0x81712e8, cvt=0x101fad0) at argp.h:579

As it should be!  :-)


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

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

[Ulrich Weigand]

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?

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

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?

Thanks,
Ulrich

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

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

[Pedro Alves]

[-- 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.  */

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

[Thomas Schwinge]

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

Hello!

On Mon, Oct 13, 2008 at 07:35:34PM +0100, Pedro Alves wrote:
> 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.

I can't tell without having a detailed look at that -- for which I
absolutely don't have the time at the moment, unfortunately.  I made this
a TODO item:
<http://www.bddebian.com/~wiki/hurd/open_issues/gdb_thread_ids/>


> On Monday 13 October 2008 17:40:25, Ulrich Weigand wrote:
> > 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'.

Another TODO item:
<http://www.bddebian.com/~wiki/hurd/open_issues/gdb_pending_execs/>


> (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.)

And non-stop mode isn't even supported with gnu-nat.c -- yet another TODO
item: <http://www.bddebian.com/~wiki/hurd/open_issues/gdb_non-stop_mode/>


> 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.

> 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.

Fine on Hurd as well, thanks!


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: GDB HEAD (partly) broken for GNU/Hurd

[Pedro Alves]

On Saturday 11 October 2008 18:26:11, Thomas Schwinge wrote:

> > Doesn't resume the whole shell?
> 
> But as I see things we always have ``non_stop == 0'' and thus
> ``resume_ptid = pid_to_ptid (-1)'', so that should be fine, isn't it?
> 

Duh!  Yes, of course.

> Here is a debugging-enabled run of a thusly patched GDB HEAD:

Thanks.

> 
>     STOPPED_BY_WATCHPOINT () = 0
>     infrun: context switch
>     infrun: Switching context from bogus thread id 1 to Thread 25830.3

              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is missing in the new implementation.  In Hurd when going through
the shell doing fork/execs, we see new threads at every exec, and the
old threads disappear.  Could you try adding a `switch_to_thread (resume_thread)'
somewhere after target_wait returns and before any other target method?

-- 
Pedro Alves

Re: GDB HEAD (partly) broken for GNU/Hurd

[Thomas Schwinge]

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

Hello!

On Sat, Oct 11, 2008 at 06:46:31PM +0100, Pedro Alves wrote:
> >     STOPPED_BY_WATCHPOINT () = 0
> >     infrun: context switch
> >     infrun: Switching context from bogus thread id 1 to Thread 25830.3
> 
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This is missing in the new implementation.  In Hurd when going through
> the shell doing fork/execs, we see new threads at every exec, and the
> old threads disappear.  Could you try adding a `switch_to_thread (resume_thread)'
> somewhere after target_wait returns and before any other target method?

Haha!, see my other email.  :-D


Regards,
 Thomas

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]