[gdbserver] Fix attaching notices

[gdbserver] Fix attaching notices

[Pedro Alves]

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

Hi,

This patch fixes several issues I noticed with attaching
to processes with gdbserver's --multi support, on several
host/target combinations:

Linux host -> Linux gdbserver

 There's code in gdbserver to pretend the initial SIGSTOP
 was a SIGTRAP, that's been put there so
 "gdbserver --attach PID"; "target remote", doesn't see
 the SIGSTOP.  This smudging should not be done when
 handling a vAttach, otherwise, the user is presented
 with:

 (gdb) tar extended-remote :9999
 Remote debugging using :9999
 (gdb) attach 32762
 Attached to Thread 32762
 [New Thread 32762]

 Program received signal SIGTRAP, Trace/breakpoint trap.
 0x00007fc30f5d0b30 in ?? ()

 ... a bogus SIGTRAP.

Linux host -> Windows gdbserver

 - On Windows, the signal we get while attaching when the inferior gets
   stopped is really a SIGTRAP.  In this case, we do want do convert it
   to a SIGSTOP, so GDB suppresses it.

 - Due to the fact that ATTACH_NO_WAIT is a host macro (it shouldn't, and
   I'm posting a patch next to fix that), and it being defined in
   Windows hosts, the T stop reply packet gdbserver sends after attaching
   is just ignored on the GDB side.  On most other hosts (except hurd),
   ATTACH_NO_WAIT is not defined, which reveals a buglet that had
   gotten unnoticed.

 (gdb) tar extended-remote sandra:9999
 Remote debugging using sandra:9999
 (gdb) attach 988
 Attached to Thread 988
 [New Thread 2232]

 Program received signal 0, Signal 0.
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 Error while mapping shared library sections:
 /cygdrive/c/WINDOWS/system32/ntdll.dll: No such file or directory.
 Error while mapping shared library sections:
 /cygdrive/c/WINDOWS/system32/kernel32.dll: No such file or directory.
 ...
 /usr/bin/cygiconv-2.dll: No such file or directory.
 Symbol file not found for /cygdrive/c/WINDOWS/system32/ntdll.dll
 Symbol file not found for /cygdrive/c/WINDOWS/system32/kernel32.dll
 ...
 [Switching to Thread 2232]
 Stopped due to shared library event
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

 On initial connections, we're preventing gdbserver to report
 dll changes in the T stop reply packet, because GDB will always query
 the dll list anyway.  We missed doing that on vAttach handling.
 The "signal 0" notice happens even though gdbserver sends T05, because
 the stop reason is transformed into a TARGET_WAITKIND_LOADED, and the
 signal number sent isn't really parsed (from remote.c):

	  if (solibs_changed)
	    status->kind = TARGET_WAITKIND_LOADED;
	  else
	    {
	      status->kind = TARGET_WAITKIND_STOPPED;
	      status->value.sig = (enum target_signal)
		(((fromhex (buf[1])) << 4) + (fromhex (buf[2])));


Windows host -> Windows gdbserver

 Due to ATTACH_NO_WAIT being defined, the T stop reply
 gdbserver sends on attaches is just being ignored.  That's why
 the "stopped by library event" notice bug went unnoticed.
 I had worked on the Windows port of --multi for gdbserver
 from a Windows host :-)
 I'm posting a patch next that gets rid of ATTACH_NO_WAIT, which
 then triggers the same "problems" as seen while attaching
 to a Windows gdbserver from a Linux host.

Windows host -> Linux gdbserver

 Same rationale as above.

Tested manually on all those combinations, and regression tested
with local gdbservers on x86_64-unknown-linux-gnu, x86-pc-cygwin.

OK?

-- 
Pedro Alves

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

2008-06-28  Pedro Alves  <pedro@codesourcery.com>

	* server.c (attach_inferior): Add vattach parameter.  If attaching
	due to a vAttach, don't hide a SIGSTOP, and, if attaching finishes
	with a SIGTRAP, pretend it was a SIGSTOP.
	(handle_v_attach): Pass 1 as vattach parameter to attach_inferior.
	Don't report dll changes.
	(main): Pass 0 as vattach parameter to attach_inferior.

---
 gdb/gdbserver/server.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c
+++ src/gdb/gdbserver/server.c
@@ -160,7 +160,7 @@ start_inferior (char **argv, char *statu
 }
 
 static int
-attach_inferior (int pid, char *statusptr, int *sigptr)
+attach_inferior (int pid, char *statusptr, int *sigptr, int vattach)
 {
   /* myattach should return -1 if attaching is unsupported,
      0 if it succeeded, and call error() otherwise.  */
@@ -180,11 +180,17 @@ attach_inferior (int pid, char *statuspt
 
   *sigptr = mywait (statusptr, 0);
 
-  /* GDB knows to ignore the first SIGSTOP after attaching to a running
-     process using the "attach" command, but this is different; it's
-     just using "target remote".  Pretend it's just starting up.  */
-  if (*statusptr == 'T' && *sigptr == TARGET_SIGNAL_STOP)
+  /* GDB knows to ignore the first SIGSTOP after attaching to a
+     running process using the "attach" command, but if we're
+     attaching due to --attach this is different; it's just using
+     "target remote".  Pretend it's just starting up.  */
+  if (!vattach && *statusptr == 'T' && *sigptr == TARGET_SIGNAL_STOP)
     *sigptr = TARGET_SIGNAL_TRAP;
+  /* Similarly, on some targets, attaching finishes with a SIGTRAP
+     (e.g., on Windows it's called the initial breakpoint).  Pretend
+     it was a SIGSTOP, so GDB hushes it.  */
+  else if (vattach && *statusptr == 'T' && *sigptr == TARGET_SIGNAL_TRAP)
+    *sigptr = TARGET_SIGNAL_STOP;
 
   return 0;
 }
@@ -1011,8 +1017,13 @@ handle_v_attach (char *own_buf, char *st
   int pid;
 
   pid = strtol (own_buf + 8, NULL, 16);
-  if (pid != 0 && attach_inferior (pid, status, signal) == 0)
+  if (pid != 0 && attach_inferior (pid, status, signal, 1) == 0)
     {
+      /* Don't report shared library events after attaching, even if
+	 some libraries are preloaded.  GDB will always poll the
+	 library list.  Avoids the "stopped by shared library event"
+	 notice on the GDB side.  */
+      dlls_changed = 0;
       prepare_resume_reply (own_buf, *status, *signal);
       return 1;
     }
@@ -1336,7 +1347,7 @@ main (int argc, char *argv[])
     }
   else if (pid != 0)
     {
-      if (attach_inferior (pid, &status, &signal) == -1)
+      if (attach_inferior (pid, &status, &signal, 0) == -1)
 	error ("Attaching not supported on this target");
 
       /* Otherwise succeeded.  */

Re: [gdbserver] Fix attaching notices

[Daniel Jacobowitz]

On Sat, Jun 28, 2008 at 12:11:12AM +0100, Pedro Alves wrote:
> Hi,
> 
> This patch fixes several issues I noticed with attaching
> to processes with gdbserver's --multi support, on several
> host/target combinations:
> 
> Linux host -> Linux gdbserver
> 
>  There's code in gdbserver to pretend the initial SIGSTOP
>  was a SIGTRAP, that's been put there so
>  "gdbserver --attach PID"; "target remote", doesn't see
>  the SIGSTOP.  This smudging should not be done when
>  handling a vAttach, otherwise, the user is presented
>  with:
> 
>  (gdb) tar extended-remote :9999
>  Remote debugging using :9999
>  (gdb) attach 32762
>  Attached to Thread 32762
>  [New Thread 32762]
> 
>  Program received signal SIGTRAP, Trace/breakpoint trap.
>  0x00007fc30f5d0b30 in ?? ()
> 
>  ... a bogus SIGTRAP.

Did you reproduce this with an unmodified GDB or a patched one?  I
can't reproduce it - I was curious since I did test vAttach and I
don't remember seeing these SIGTRAPs.

Actually, wait... trying another GDB I see that this problem has
appeared on the GDB side between 6.8 and HEAD.  6.8 doesn't print out
the message about a SIGTRAP.

>  [Switching to Thread 2232]
>  Stopped due to shared library event
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This part looks fine.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [gdbserver] Fix attaching notices

[Pedro Alves]

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

A Monday 07 July 2008 18:55:10, Daniel Jacobowitz wrote:
> On Sat, Jun 28, 2008 at 12:11:12AM +0100, Pedro Alves wrote:
> >  (gdb) tar extended-remote :9999
> >  Remote debugging using :9999
> >  (gdb) attach 32762
> >  Attached to Thread 32762
> >  [New Thread 32762]
> >
> >  Program received signal SIGTRAP, Trace/breakpoint trap.
> >  0x00007fc30f5d0b30 in ?? ()
> >
> >  ... a bogus SIGTRAP.
>
> Did you reproduce this with an unmodified GDB or a patched one?  I
> can't reproduce it - I was curious since I did test vAttach and I
> don't remember seeing these SIGTRAPs.

Unmodified HEAD.

> Actually, wait... trying another GDB I see that this problem has
> appeared on the GDB side between 6.8 and HEAD.  6.8 doesn't print out
> the message about a SIGTRAP.

You're right, I just tried with 6.8 too, and don't see the SIGTRAP
notice...  I'll try to pinpoint what changed this, and see if it
was a spurious change.

>
> >  [Switching to Thread 2232]
> >  Stopped due to shared library event
> >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This part looks fine.

In the mean time, I've checked the bit to fix this in, as attached.

-- 
Pedro Alves

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

2008-07-07  Pedro Alves  <pedro@codesourcery.com>

	* server.c (handle_v_attach): Inhibit reporting dll changes.

---
 gdb/gdbserver/server.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2008-07-07 21:17:28.000000000 +0100
+++ src/gdb/gdbserver/server.c	2008-07-07 21:19:07.000000000 +0100
@@ -1024,6 +1024,11 @@ handle_v_attach (char *own_buf, char *st
   pid = strtol (own_buf + 8, NULL, 16);
   if (pid != 0 && attach_inferior (pid, status, signal) == 0)
     {
+      /* Don't report shared library events after attaching, even if
+	 some libraries are preloaded.  GDB will always poll the
+	 library list.  Avoids the "stopped by shared library event"
+	 notice on the GDB side.  */
+      dlls_changed = 0;
       prepare_resume_reply (own_buf, *status, *signal);
       return 1;
     }

Re: [gdbserver] Fix attaching notices

[Pedro Alves]

A Tuesday 08 July 2008 00:51:45, Pedro Alves wrote:

> You're right, I just tried with 6.8 too, and don't see the SIGTRAP
> notice...  I'll try to pinpoint what changed this, and see if it
> was a spurious change.
>

Unsurprisingly,

> 2008-05-01  Daniel Jacobowitz  <dan@codesourcery.com>
> 	    Pedro Alves  <pedro@codesourcery.com>
> 
> 	Based on work by Jan Kratochvil <jan.kratochvil@redhat.com> and Jeff
>  	Johnston <jjohnstn@redhat.com>.
> 
> 	* NEWS: Mention attach to stopped process fix.
> 	* infcmd.c (detach_command, disconnect_command): Discard the thread
> 	list.

> 	* infrun.c (handle_inferior_event): Do not ignore non-SIGSTOP while
> 	attaching.  Use signal_stop_state.

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

> 	(signal_stop_state): Check stop_soon.
> 	* linux-nat.c (kill_lwp): Declare earlier.
> 	(pid_is_stopped, linux_nat_post_attach_wait): New.
> 	(lin_lwp_attach_lwp): Use linux_nat_post_attach_wait.  Update
> 	comments.
> 	(linux_nat_attach): Use linux_nat_post_attach_wait.
> 	(detach_callback, linux_nat_detach): Improve handling for signalled
> 	processes.
> 	(linux_nat_pid_to_str): Always print out the LWP ID if it differs
> 	from the process ID.
> 	* Makefile.in (infcmd.o): Update.


Before,

    /* This originates from attach_command().  We need to overwrite
         the stop_signal here, because some kernels don't ignore a
         SIGSTOP in a subsequent ptrace(PTRACE_SONT,SOGSTOP) call.
         See more comments in inferior.h.  */
      if (stop_soon == STOP_QUIETLY_NO_SIGSTOP)
	{
	  stop_stepping (ecs);
	  if (stop_signal == TARGET_SIGNAL_STOP)
	    stop_signal = TARGET_SIGNAL_0;
	  return;
	}

After,

      /* This originates from attach_command().  We need to overwrite
         the stop_signal here, because some kernels don't ignore a
         SIGSTOP in a subsequent ptrace(PTRACE_CONT,SIGSTOP) call.
         See more comments in inferior.h.  On the other hand, if we
	 get a non-SIGSTOP, report it to the user - assume the backend
	 will handle the SIGSTOP if it should show up later.  */
      if (stop_soon == STOP_QUIETLY_NO_SIGSTOP
	  && stop_signal == TARGET_SIGNAL_STOP)
	{
	  stop_stepping (ecs);
	  stop_signal = TARGET_SIGNAL_0;
	  return;
	}

Since gdbserver is reporting a TARGET_SIGNAL_TRAP, this now doesn't
match, and we go on handling the event until we decide it was a 
random signal.

So, we either

1) go with my patch (on which the win32 part was a hack, but I
   can live with it), and live with the bogus notice
against older gdbservers, or

2) change the test to:

      if (stop_soon == STOP_QUIETLY_NO_SIGSTOP
	  && stop_signal == TARGET_SIGNAL_STOP
	  && stop_signal == TARGET_SIGNAL_TRAP)
	{
	  stop_stepping (ecs);
	  stop_signal = TARGET_SIGNAL_0;
	  return;
	}

   Or even add a `&& stop_signal == TARGET_SIGNAL_0', and merge
   this stop_soon with STOP_QUIETLY_REMOTE.

3) other?

-- 
Pedro Alves

Re: [gdbserver] Fix attaching notices

[Daniel Jacobowitz]

On Tue, Jul 08, 2008 at 02:58:33AM +0100, Pedro Alves wrote:
> > 	* infrun.c (handle_inferior_event): Do not ignore non-SIGSTOP while
> > 	attaching.  Use signal_stop_state.
> 
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Bah.

> So, we either
> 
> 1) go with my patch (on which the win32 part was a hack, but I
>    can live with it), and live with the bogus notice
> against older gdbservers, or
> 
> 2) change the test to:
> 
>       if (stop_soon == STOP_QUIETLY_NO_SIGSTOP
> 	  && stop_signal == TARGET_SIGNAL_STOP
> 	  && stop_signal == TARGET_SIGNAL_TRAP)

Ought to be some || in there :-)

> 	{
> 	  stop_stepping (ecs);
> 	  stop_signal = TARGET_SIGNAL_0;
> 	  return;
> 	}
> 
>    Or even add a `&& stop_signal == TARGET_SIGNAL_0', and merge
>    this stop_soon with STOP_QUIETLY_REMOTE.

I don't think I understand.

Allowing both stop and trap sounds reasonable to me.  We already rely
on there not being other sources of SIGTRAP.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [gdbserver] Fix attaching notices

[Pedro Alves]

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

A Friday 11 July 2008 15:13:26, Daniel Jacobowitz wrote:
> On Tue, Jul 08, 2008 at 02:58:33AM +0100, Pedro Alves wrote:

> > 2) change the test to:
> >
> >       if (stop_soon == STOP_QUIETLY_NO_SIGSTOP
> > 	  && stop_signal == TARGET_SIGNAL_STOP
> > 	  && stop_signal == TARGET_SIGNAL_TRAP)
>
> Ought to be some || in there :-)
>

No kidding!  No more posting at 3AM for me.  :-)

> > 	{
> > 	  stop_stepping (ecs);
> > 	  stop_signal = TARGET_SIGNAL_0;
> > 	  return;
> > 	}
> >
> >    Or even add a `&& stop_signal == TARGET_SIGNAL_0', and merge
> >    this stop_soon with STOP_QUIETLY_REMOTE.
>
> I don't think I understand.
>

I think this is the same insanity as above :-)

> Allowing both stop and trap sounds reasonable to me.  We already rely
> on there not being other sources of SIGTRAP.

Here a patch for that.  I confirmed that the SIGTRAP notice is gone against
gdbserver, and ran the the attaching into signals tests, and the
attach.exp test nativelly, and all pass cleanly.

-- 
Pedro Alves

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

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

	* infrun.c (handle_inferior_event): Also ignore a
	TARGET_SIGNAL_TRAP on a STOP_QUIETLY_NO_SIGSTOP.

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

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-07-11 16:33:00.000000000 +0100
+++ src/gdb/infrun.c	2008-07-11 16:41:51.000000000 +0100
@@ -2543,13 +2543,20 @@ targets should add new threads to the th
 	}
 
       /* This originates from attach_command().  We need to overwrite
-         the stop_signal here, because some kernels don't ignore a
-         SIGSTOP in a subsequent ptrace(PTRACE_CONT,SIGSTOP) call.
-         See more comments in inferior.h.  On the other hand, if we
+	 the stop_signal here, because some kernels don't ignore a
+	 SIGSTOP in a subsequent ptrace(PTRACE_CONT,SIGSTOP) call.
+	 See more comments in inferior.h.  On the other hand, if we
 	 get a non-SIGSTOP, report it to the user - assume the backend
-	 will handle the SIGSTOP if it should show up later.  */
+	 will handle the SIGSTOP if it should show up later.
+
+	 Also consider that the attach is complete when we see a
+	 SIGTRAP.  Some systems (e.g. Windows), and stubs supporting
+	 target extended-remote report it instead of a SIGSTOP
+	 (e.g. gdbserver).  We already rely on SIGTRAP being our
+	 signal, so this is no exception.  */
       if (stop_soon == STOP_QUIETLY_NO_SIGSTOP
-	  && stop_signal == TARGET_SIGNAL_STOP)
+	  && (stop_signal == TARGET_SIGNAL_STOP
+	      || stop_signal == TARGET_SIGNAL_TRAP))
 	{
 	  stop_stepping (ecs);
 	  stop_signal = TARGET_SIGNAL_0;

Re: [gdbserver] Fix attaching notices

[Daniel Jacobowitz]

On Fri, Jul 11, 2008 at 06:12:01PM +0100, Pedro Alves wrote:
> 2008-07-11  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (handle_inferior_event): Also ignore a
> 	TARGET_SIGNAL_TRAP on a STOP_QUIETLY_NO_SIGSTOP.

OK.

-- 
Daniel Jacobowitz
CodeSourcery