From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26056 invoked by alias); 12 Jun 2012 16:14:47 -0000 Received: (qmail 26040 invoked by uid 22791); 12 Jun 2012 16:14:43 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 12 Jun 2012 16:14:25 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5CGELCX029650 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 12 Jun 2012 12:14:21 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q5CGEJVp017967; Tue, 12 Jun 2012 12:14:20 -0400 Message-ID: <4FD76ADB.7090302@redhat.com> Date: Tue, 12 Jun 2012 16:14:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver References: <1339246002-1987-1-git-send-email-yao@codesourcery.com> <1339246002-1987-4-git-send-email-yao@codesourcery.com> In-Reply-To: <1339246002-1987-4-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-06/txt/msg00359.txt.bz2 On 06/09/2012 01:46 PM, Yao Qi wrote: > When GDB or GDBserver terminated inferior, agent has no chance to > remove socket file, even in atexit hook. This patch adds a call > to send command 'kill' to agent in function kill_inferior, in > order to tell agent that inferior will be killed, and then agent > can do cleanup. > > Of course, new agent command 'kill' is added and documented. > > When GDB or GDBserver sends 'kill' command to an older agent, in > which 'kill is not known, and it will be ignored. > > gdb/gdbserver: > > 2012-06-09 Yao Qi > > * server.h: Declare run_inferior_command. > * target.c (kill_inferior): New. Send command 'kill'. > * target.h (kill_inferior): Removed macro. > * tracepoint.c (run_inferior_command): Remove static. > Handle command 'kill'. Missing context function for this sentence? > > gdb/doc: > > 2012-06-09 Yao Qi > > * gdb.texinfo (IPA Protocol Commands): Document new command > 'kill'. > --- > gdb/doc/gdb.texinfo | 4 ++++ > gdb/gdbserver/server.h | 1 + > gdb/gdbserver/target.c | 8 ++++++++ > gdb/gdbserver/target.h | 5 ++--- > gdb/gdbserver/tracepoint.c | 17 +++++++++++++++-- > 5 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index ad227a4..09339e5 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -33585,6 +33585,10 @@ for an error > > @end table > > +@item kill > +Kills in-process agent. That's not really true. The command doesn't kill the in-process agent. That'd be the same as killing the inferior, given that the agent is in-process. Maybe calling the command "close" or "closecon" would be better. > Usually, this command is sent when @value{GDBN} > +or GDBserver is about to kill inferiors. Drop the "usually". There's only one use for this presently, so let's be specific. > + > @item qTfSTM > @xref{qTfSTM}. > @item qTsSTM > diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h > index 02dfa29..452e400 100644 > --- a/gdb/gdbserver/server.h > +++ b/gdb/gdbserver/server.h > @@ -558,4 +558,5 @@ int emit_error; > extern const char version[]; > extern const char host_name[]; > > +int run_inferior_command (char *cmd, int len); > #endif /* SERVER_H */ > diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c > index 7539476..35aaecd 100644 > --- a/gdb/gdbserver/target.c > +++ b/gdb/gdbserver/target.c > @@ -182,3 +182,11 @@ target_waitstatus_to_string (const struct target_waitstatus *ws) > > return buf; > } > + > +int > +kill_inferior(int pid) > +{ > + run_inferior_command ("kill", strlen ("kill")); - Missing space before parens. - What happens if there's no IPA to talk to? - Also, all callers of run_inferior_command command currently pass "cmd, strlen (cmd) + 1". - The kill_inferior function takes a PID as argument. It's not right to assume that PID is the current process. IOW, in a multi-process scenario, you may well run the inferior command on the wrong inferior as is. > + > + return (*the_target->kill) (pid); > +} > diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h > index 0cf5687..101c3c4 100644 > --- a/gdb/gdbserver/target.h > +++ b/gdb/gdbserver/target.h > @@ -409,9 +409,6 @@ void set_target_ops (struct target_ops *); > #define myattach(pid) \ > (*the_target->attach) (pid) > > -#define kill_inferior(pid) \ > - (*the_target->kill) (pid) > - > #define detach_inferior(pid) \ > (*the_target->detach) (pid) > > @@ -555,4 +552,6 @@ const char *target_pid_to_str (ptid_t); > > const char *target_waitstatus_to_string (const struct target_waitstatus *); > > +int kill_inferior (int); I'd prefer leaving this declaration where the old macro is, close to the cousins attach, detach, etc. > + > #endif /* TARGET_H */ > diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c > index 4e6c3ec..bb2df8f 100644 > --- a/gdb/gdbserver/tracepoint.c > +++ b/gdb/gdbserver/tracepoint.c > @@ -412,7 +412,7 @@ static int flush_trace_buffer_handler (CORE_ADDR); > static void download_trace_state_variables (void); > static void upload_fast_traceframes (void); > > -static int run_inferior_command (char *cmd, int len); > +int run_inferior_command (char *cmd, int len); This is now declared in server.h, so this declaration should be removed, not adjusted. > > static int > read_inferior_integer (CORE_ADDR symaddr, int *val) > @@ -6662,7 +6662,7 @@ static struct ltt_available_probe gdb_ust_probe = > thread by means of direct memory xfering, and a socket for > synchronization. */ > > -static int > +int > run_inferior_command (char *cmd, int len) > { > int err = -1; > @@ -7053,6 +7053,17 @@ gdb_agent_helper_thread (void *arg) > > if (cmd_buf[0]) > { > + if (strncmp ("kill", cmd_buf, 4) == 0) > + { > + ret = write (fd, buf, 1); > + close (fd); > + > + close (listen_fd); > + unlink (agent_socket_name); > + > + return NULL; > + } > + else > #ifdef HAVE_UST > if (strcmp ("qTfSTM", cmd_buf) == 0) > { > @@ -7080,7 +7091,9 @@ gdb_agent_helper_thread (void *arg) > { > cmd_qtstmat (cmd_buf); > } > + else > #endif /* HAVE_UST */ > + {} Why do we need these "else" and "{}" lines? AFAICS, you can just remove them. You could do: - if (strcmp ("qTfSTM", cmd_buf) == 0) + else if (strcmp ("qTfSTM", cmd_buf) == 0) to be extra neat. > } > > /* Fix compiler's warning: ignoring return value of 'write'. */ -- Pedro Alves