Re: [PATCH v6] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages

[Pedro Alves <palves@redhat.com>]

On 04/16/2018 09:04 PM, Sergio Durigan Junior wrote:
> Changes from v5:
> 
> - Save result from 'target_pid_to_str' using std::string instead of
>   'const char *', avoiding a possible race condition.

Sorry Sergio, we're getting there, but this will need more work.

> Built and regtested on BuildBot, without regressions.
> 

> 	* gdb.threads/process-dies-while-detaching.c
> 	(parent_function): Use 'usleep' in order to avoid
> 	race-conditions.

This is stale.  There's no 'usleep' any more.

> @@ -121,9 +120,11 @@ struct inferior *
>  add_inferior (int pid)
>  {
>    struct inferior *inf = add_inferior_silent (pid);
> +  std::string infpid = target_pid_to_str (pid_to_ptid (pid));
>  
>    if (print_inferior_events)
> -    printf_unfiltered (_("[New inferior %d]\n"), pid);
> +    printf_unfiltered (_("[New inferior %d (%s)]\n"),
> +		       inf->num, infpid.c_str ());

Let's call target_pid_to_str directly here without the
deep copy.

You needed to call target_pid_to_str early in kill_command
because there first kill the target, and print the
string _afterwards_.  "target_kill" may pop the target from the
target stack, so we can't use target_pid_to_str after the
target is gone already.

But here, we're not messing with the target stack.

(Nor are we calling target_pid_to_str twice in a row in
the same expression.)

>  void
> @@ -261,12 +259,13 @@ void
>  detach_inferior (inferior *inf)
>  {
>    /* Save the pid, since exit_inferior_1 will reset it.  */
> -  int pid = inf->pid;
> +  std::string infpid = target_pid_to_str (pid_to_ptid (inf->pid));
>  
>    exit_inferior_1 (inf, 0);
>  
>    if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
> +    printf_unfiltered (_("[Inferior %d (%s) detached]\n"),
> +		       inf->num, infpid.c_str ());

Same here.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index f54a38833b..be118c5d97 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5137,7 +5137,12 @@ remote_detach_1 (int from_tty, inferior *inf)
>    /* If doing detach-on-fork, we don't mourn, because that will delete
>       breakpoints that should be available for the followed inferior.  */
>    if (!is_fork_parent)
> -    target_mourn_inferior (inferior_ptid);
> +    {
> +      target_mourn_inferior (inferior_ptid);
> +      if (print_inferior_events)
> +	printf_unfiltered (_("[Inferior %d (process %d) detached]\n"),
> +			   inf->num, pid);

This is still using "(process %d)", which is incorrect
for target remote.

Also, here, if debugging in "target remote" mode, and this is 
the last live process, that target_mourn_inferior call will
unpush the remote target from the target stack.  So this is another
case where you need to call target_pid_to_str _before_ calling
target_mourn_inferior.

As mentioned before, not all targets print "process $decimal" in
their target_pid_to_str implementation.  So your testsuite changes
will need to adapt too.

Please try this: hack away gdbserver/server.c's support for
the RSP multi-process extensions (look for "multiprocess+"), and
test with "target remote".  That hack will emulate older gdbservers and
other stubs that don't support telling gdb the process's pid.
Manually confirm that you get "Remote target" instead of
"process $pid" -- see remote.c's implementation of
target_pid_to_str.  Then run the testsuite against that gdbserver.

Thanks,
Pedro Alves