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

Pedro Alves
2012-06-15T19:25:00Z

On 06/14/2012 03:50 PM, Yao Qi wrote:

> On Wednesday 13 June 2012 00:14:19 Pedro Alves wrote:
>>> > > + 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.
> This new patch addresses all your comments except this one above. IMO, the
> whole agent work in GDBserver doesn't consider much about multi-process, so
> this problem shall be addressed as a whole, when we start to support agent in
> multi-process. I leave a FIXME in comment there. WDYT?

I'd really rather see that fixed now. Even with single process,
you may get here without a selected inferior at all. We just need
to lookup a thread of PID, and select it as current_inferior for the
duration of run_inferior_command, I think? (wrapped with the
save_inferior dance).

> +int
> +kill_inferior (int pid)
> +{

> + char buf[IPA_CMD_BUF_SIZE];
> +
> + if (!maybe_write_ipa_not_loaded (buf))
> + {
> + strcpy (buf, "close");
> + /* FIXME: It is wrong to assume PID is the current process. In
> + a multiple-process scenario, we may run inferior command on
> + the wrong process. */
> + run_inferior_command (buf, strlen (buf) + 1);
> + }

I'd rather have this whole block be factored out to a function
in tracepoint.c (tracepoint_about_to_kill for example). More so
with a fix to set current_inferior around run_inferior_command.
Then we wouldn't have to export details of the IPA, such as
maybe_write_ipa_not_loaded or run_inferior_command.

--
Pedro Alves