This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: New "attach" and "rsh" features for GDB/gdbserver on PowerPC


Lots of comments follow.  Some of this patch is quite interesting - I
like the idea of a resident daemon mode.  But you have a few problems
that will need to be fixed first.

On Mon, Mar 04, 2002 at 10:13:57AM +0100, Wolfgang Denk wrote:
> Hello,
> 
> please find attached a patch to gdb-5.1 that  adds  support  for  the
> "attach"  command  and  a new "remote shell" command to GDB/gdbserver
> for PowerPC systems.

GDBserver has been almost entirely rewritten since 5.1 was released. 
I'd appreciate it if you could try to merge this to current CVS.  That
should let it go into 5.3 (5.2 is scheduled for release in about a
month and has already branched).

> ChangeLog entry:
> 
> ---------------------------------------------------------------------
> 
> 2002-03-04  Wolfgang Denk  <wd@denx.de>
> 
>         * add support for "attach" command and new "rshell" (remote
>         shell) command to GDB/gdbserver for PowerPC systems.
> 
>         * make gdbserver "resident" if started without process
>         argument (i. e. when started a server it does not terminate
>         when a debugged process terminates)
> 
> ---------------------------------------------------------------------

The (i.e. ) bit can already be accomplished using target
extended-remote instead of target remote.  This still has some
interesting applications though.

> Description:
> 
> Many  embedded  systems  use  gdbserver  for  application  debugging.
> However  the  current  implementation  (on PowerPC) requires that the
> processes to be debugged are started under control of gdbserver.  But
> often  you  want to debug (examine) some process on the target system
> that is already running. This is supported by the new support for the
> "attach" command.

I added --attach to gdbserver on Jan. 17th.

> This allows to have always one instance of gdbserver running  on  the
> target  as  a general purpose debug server that can be used to attach
> to any of the running application processes. In  this  "server  mode"
> (when  no  command  to  debug is given on the gdbserver command line)
> gdbserver will not terminate when the debugged  process  exits,  thus
> making sure you can continue to use the debug server.
> 
> The "rshell" (remote shell) extension allows to use GDB/gdbserver  to
> run arbitrary commands on the target system. The main intention is to
> be  able  to find out the PIDs of the processes you want to attach to
> by running a "ps" command without need for additional services on the
> target.

This needs to be enabled by an explicit command line option to
gdbserver, and well documented.  Bear in mind that the remote protocol
has -NO- authentication, even IP based.  This makes it much too trivial
to gain access to the target system by intercepting a debug session. 
It was already possible, but at least it took a little thought...


[Non-MIME patches are prefered on this list.]

> --- gdb-5.1.1/gdb/configure.ORIG	Thu Aug  2 23:30:20 2001
> +++ gdb-5.1.1/gdb/configure	Sun Feb 24 23:14:45 2002

Also, please don't include patches to generated files.  And do include
ChangeLog entries.  Standard GNU policy...

> + diff -Nrup gdb-5.1.1/gdb/acconfig.h.ORIG gdb-5.1.1/gdb/acconfig.h
> --- gdb-5.1.1/gdb/acconfig.h.ORIG	Sat Mar 31 20:09:02 2001
> +++ gdb-5.1.1/gdb/acconfig.h	Sun Feb 24 23:14:45 2002
> @@ -170,3 +170,10 @@
>  
>  /* nativefile */
>  #undef GDB_NM_FILE
> +
> +/* define to have target attach feature */
> +#undef HAVE_TARGET_ATTACH
> +
> +/* define to have target remote shell feature */
> +#undef HAVE_TARGET_SHELL
> +

Why do you bother doing this...

> + diff -Nrup gdb-5.1.1/gdb/configure.in.ORIG gdb-5.1.1/gdb/configure.in
> --- gdb-5.1.1/gdb/configure.in.ORIG	Thu Aug  2 23:30:22 2001
> +++ gdb-5.1.1/gdb/configure.in	Sun Feb 24 23:14:45 2002
> @@ -50,6 +50,8 @@ CONFIG_ALL=
>  CONFIG_CLEAN=
>  CONFIG_INSTALL=
>  CONFIG_UNINSTALL=
> +AC_DEFINE(HAVE_TARGET_ATTACH)
> +AC_DEFINE(HAVE_TARGET_SHELL)
>  
>  configdirs="doc testsuite"
>  

If you are just going to do that?  I believe that by doing this you
also broke compilation of gdbserver for any target that didn't
implement process_attach.

> + static char *shells[] = {
> +	"/bin/sh", "/bin/bash", "/bin/tcsh",
> +	"/usr/bin/sh", "/usr/bin/bash", "/usr/bin/tsch", (char *)0};

Why not assume /bin/sh?  I've never seen a Linux system that would
successfully boot without it, and silently switching to tcsh can cause
nothing but confusion.

> +    default:	/* Parent - gdbserver */
> +      close(fd[1]);	/* To get EOF properly */
> +      enable_async_io();
> +      inferior_pid = pid;		/* fake process ID */
> +      while (1)
> +	{
> +          if ((len = read(fd[0], buf, sizeof buf)) > 0)
> +	    {
> +	      write(remote_desc, &len, 1);
> +	      write(remote_desc, buf, len);
> +	    }
> +	  else if (len != -1 || errno != EINTR)
> +	    break;
> +	}
> +      write(remote_desc, &len, 1);	/* end mark (0) */
> +      waitpid(pid, (void *)0, 0);
> +      inferior_pid = saved_pid;
> +      disable_async_io();
> +    }
> +}

This should support receiving a control-c from GDB and passing it on to
the target process (or perhaps SIGTERM/SIGKILL ing the target
process!).  Otherwise it's a real easy way to hang your resident
gdbserver.  I see some support for that; is it really enough?

Also, if the program receives a signal at an inopportune time you'll
not collect it properly.  Should that waitpid check WIFEXITED?

> @@ -2040,6 +2052,20 @@ remote_start_remote (PTR dummy)
>    get_offsets ();		/* Get text, data & bss offsets */
>  
>    putpkt ("?");			/* initiate a query from remote machine */
> +#ifdef HAVE_TARGET_ATTACH
> +  getpkt(buf, PBUFSIZ, 0);
> +  if (buf[0] == 'E')
> +    {
> +      error ("Remote failure reply: %s", buf);
> +    } else
> + if (buf[0] == 'I')
> +    {
> +      inferior_ptid = null_ptid;
> +      return 1;			/* Remote server is idle waiting for attach */
> +    }
> +  putpkt ("?");			/* wait_for_inferior() called in start_remote
> +				   expects "wait status" packet */
> +#endif
>    immediate_quit--;
>  
>    return remote_start_remote_dummy (dummy);

You've checked that this still works with an unmodified stub, right?

> @@ -2148,6 +2174,9 @@ serial device is attached to the remote 
>  
>    unpush_target (target);
>  
> +#ifdef HAVE_TARGET_ATTACH
> + reread_symbols();
> +#endif 
>    remote_desc = serial_open (name);
>    if (!remote_desc)
>      perror_with_name (name);

This is necessary even now, and should probably be submitted as a
separate patch.  If you detach and then re-attach and the
executable has changed, we should reread symbols.  Thanks.

> @@ -2245,6 +2278,9 @@ serial device is attached to the remote 
>  
>    unpush_target (target);
>  
> +#ifdef HAVE_TARGET_ATTACH
> + reread_symbols();
> +#endif 
>    remote_desc = serial_open (name);
>    if (!remote_desc)
>      perror_with_name (name);

Likewise, of course.


For the rest, you are extending the remote protocol; you can't do that
just by submitting the feature, it needs to be discussed first.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]