Re: [PATCH 3/4] New agent command 'kill' and used by gdbserver

[Pedro Alves <palves at redhat dot com>]

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  <yao@codesourcery.com>
> 
> 	* 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  <yao@codesourcery.com>
> 
> 	* 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