From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13003 invoked by alias); 4 Mar 2002 17:02:00 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 12885 invoked from network); 4 Mar 2002 17:01:55 -0000 Received: from unknown (HELO nevyn.them.org) (128.2.145.6) by sources.redhat.com with SMTP; 4 Mar 2002 17:01:55 -0000 Received: from drow by nevyn.them.org with local (Exim 3.34 #1 (Debian)) id 16hvqV-0005Xk-00; Mon, 04 Mar 2002 12:01:55 -0500 Date: Mon, 04 Mar 2002 09:02:00 -0000 From: Daniel Jacobowitz To: Wolfgang Denk Cc: gdb-patches@sources.redhat.com Subject: Re: New "attach" and "rsh" features for GDB/gdbserver on PowerPC Message-ID: <20020304120155.A20045@nevyn.them.org> Mail-Followup-To: Wolfgang Denk , gdb-patches@sources.redhat.com References: <20020304091402.DE57C109E9@denx.denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20020304091402.DE57C109E9@denx.denx.de> User-Agent: Mutt/1.3.23i X-SW-Source: 2002-03/txt/msg00032.txt.bz2 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 > > * 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