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

[Pedro Alves <palves at redhat dot com>]

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