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

Pedro Alves
2012-06-12T16:14:47Z

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