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

[Pedro Alves <palves at redhat dot com>]

On 04/05/2018 07:46 PM, Sergio Durigan Junior wrote:
> Changes from v3:
> 
> - Revisited commit log and fixed wrong copy&paste from GDB output.
> 
> - Use target_pid_to_str where applicable.  Use
>   gdb::unique_xmalloc_ptr<char> to save results from
>   target_pid_to_str in some cases.

Arguably target_pid_to_str implementations should be
using get_print_cell instead of a single static buffer, avoiding
the problem.  But that's a larger change, so local copy is fine
with me.

> @@ -2598,8 +2598,16 @@ kill_command (const char *arg, int from_tty)
>      error (_("The program is not being run."));
>    if (!query (_("Kill the program being debugged? ")))
>      error (_("Not confirmed."));
> +
> +  const char *pid_str = target_pid_to_str (inferior_ptid);
> +  int infnum = current_inferior ()->num;
> +
>    target_kill ();
>  
> +  if (print_inferior_events)
> +    printf_unfiltered (_("[Inferior %d (process %s) has been killed]\n"),
> +		       infnum, pid_str);

This still seem risky -- Target backends use target_pid_to_str inside
target_kill, e.g., when logging is enabled.  E.g., 

linux_nat_kill -> -> stop_callback -> target_pid_to_str

ISTM a deep copy like:

  std::string pid_str = target_pid_to_str (inferior_ptid);

would be safer/better.

>  /* The Current Inferior.  This is a strong reference.  I.e., whenever
>     an inferior is the current inferior, its refcount is
> @@ -123,7 +122,8 @@ add_inferior (int pid)
>    struct inferior *inf = add_inferior_silent (pid);
>  
>    if (print_inferior_events)
> -    printf_unfiltered (_("[New inferior %d]\n"), pid);
> +    printf_unfiltered (_("[New inferior %d (process %d)]\n"),
> +		       inf->num, pid);

As discussed in a previous revision, using hardcoded "(process %d)"
doesn't work properly, because PID can be a fake PID number, or the
target may have no concept of processes, e.g., when debugging remote
targets.  ISTRM this should use target_pid_to_str as well.

> @@ -266,7 +263,8 @@ detach_inferior (inferior *inf)
>    exit_inferior_1 (inf, 0);
>  
>    if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d detached]\n"), pid);
> +    printf_unfiltered (_("[Inferior %d (process %d) detached]\n"),
> +		       inf->num, pid);

Ditto.

> -      if (info_verbose || debug_infrun)
> +      if (print_inferior_events)
>  	{
> +	  gdb::unique_xmalloc_ptr<char>
> +	    parent_pid (xstrdup (target_pid_to_str (parent_ptid)));
> +	  gdb::unique_xmalloc_ptr<char>
> +	    child_pid (xstrdup (target_pid_to_str (child_ptid)));

std::string would be simpler than gdb::unique_xmalloc_ptr here:

	  std::string parent_pid = target_pid_to_str (parent_ptid));
	  std::string child_pid = target_pid_to_str (child_ptid));

gdb::unique_xmalloc_ptr is handy when you have no way around
malloc/free, but here you're in charge of the dup yourself.


> diff --git a/gdb/remote.c b/gdb/remote.c
> index 68c43f8312..4cb4badd8a 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);

Hardcoded "(process %d)" here too.

Fixing this issue in the several spots may affect your
testsuite changes -- please be sure to rerun tests with 
"target remote" afterwards.

> +    }
>    else
>      {

>  }
>  
> diff --git a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
> index e05acb1711..616b6cf7a4 100644
> --- a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
> +++ b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
> @@ -81,8 +81,15 @@ proc detach_and_expect_exit {inf_output_re test} {
>      global inferior_spawn_id
>      global gdb_prompt
>  
> +    set saw_inf_exit 0
>      return_if_fail [gdb_test_multiple "detach" $test {
> -	-re "Detaching from .*, process $decimal" {
> +	-re "Detaching from .*, process $decimal\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" {
> +	}
> +	# inf_output_re can also appear in the middle, so we catch
> +	# this case here in order to avoid racy results.
> +	-re "Detaching from .*, process $decimal\r\n${inf_output_re}\r\n\\\[Inferior $decimal \\(process $decimal\\) detached\\\]" {
> +	    verbose -log "saw inferior exit"
> +	    set saw_inf_exit 1
>  	}

I'm not sure I understand the need for this.  If you left this
gdb_test_multiple exactly as it was before your patch, wouldn't it all
work the same?

Thanks,
Pedro Alves