Re: [PATCH] Fix new inferior events output (Re: [PATCH v7] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages)

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, April 25 2018, Pedro Alves wrote:

> Hi,
>
> -- On 04/24/2018 08:49 PM, Sergio Durigan Junior wrote:
>>> On 04/19/2018 08:53 PM, Sergio Durigan Junior wrote:
>>>> Changes from v6:
>>>
>>>> - Readjusted a bunch of tests in order to be more flexible when
>>>>   matching the "process $decimal" or "Thread $decimal.$decimal"
>>>>   strings.
>>>
>>> I think you meant "process $decimal" or "Remote target", because
>>> "Thread $decimal.$decimal" won't be used with pid-only ptids.
>> 
>> I actually had to adjust a few regexps (in previous versions of the
>> patch) to accept "Thread $decimal.$decimal".  But that's a different
>> problem than the pid-only ptids, and I don't include the "Changes from
>> v..." in the commit log.
> Hmm, now that I try current master I think I see what you are
> saying.  But that shows an actual problem with the patch, we really
> shouldn't be seeing those "Thread $decimal.$decimal".  See patch below.

Thanks for catching this problem.  I don't think I have anything to add,
other than "sorry for not catching this earlier".  For whatever it is
worth, looks good to me.

> From 4b17a2be78349e6912f68b924a5e3761035ee831 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 25 Apr 2018 17:28:25 +0100
> Subject: [PATCH] Fix new inferior events output
>
> Since f67c0c917150 ("Enable 'set print inferior-events' and improve
> detach/fork/kill/exit messages"), when detaching a remote process, we
> get, for detach against a remote target:
>
>  (gdb) detach
>  Detaching from program: ...., process 5388
>  Ending remote debugging.
>  [Inferior 1 (Thread 5388.5388) detached]
>               ^^^^^^^^^^^^^^^^
>
> That is incorrect, for it is printing a thread id as string while we
> should be printing the process id instead.  I.e., either one of:
>
>  [Inferior 1 (process 5388) detached]
>  [Inferior 1 (Remote target) detached]
>
> depending on remote stub support for the multi-process extensions.
>
>
> Similarly, after killing a process, we're printing of thread ids while
> we should be printing process ids.  E.g., on native GNU/Linux:
>
>  (gdb) k
>  Kill the program being debugged? (y or n) y
>  [Inferior 1 (Thread 0x7ffff7faa8c0 (LWP 30721)) has been killed]
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> while it should have been:
>
>  Kill the program being debugged? (y or n) y
>  [Inferior 1 (process 30721) has been killed]
>               ^^^^^^^^^^^^^
>
> There's a wording inconsistency between detach and kill:
>
>  [Inferior 1 (process 30721) has been killed]
>  [Inferior 1 (process 30721) detached]
>
> Given we were already saying "detached" instead of "has been
> detached", and we used to say just "exited", and given that the "has
> been" doesn't really add any information, this commit changes the the
> message to just "killed":
>
>  [Inferior 1 (process 30721) killed]
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* infcmd.c (kill_command): Print the pid as string, not the whole
> 	thread's ptid.  Add comment.  s/has been killed/killed/ in output
> 	message.
> 	* remote.c (remote_detach_1): Print the pid as string, not the
> 	whole thread's ptid.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* gdb.base/hook-stop.exp: Expect "killed" instead of "has been
> 	killed".
> 	* gdb.base/kill-after-signal.exp: Likewise.
> 	* gdb.threads/kill.exp: Likewise.
> ---
>  gdb/infcmd.c                                 | 7 +++++--
>  gdb/remote.c                                 | 4 +++-
>  gdb/testsuite/gdb.base/hook-stop.exp         | 2 +-
>  gdb/testsuite/gdb.base/kill-after-signal.exp | 2 +-
>  gdb/testsuite/gdb.threads/kill.exp           | 2 +-
>  5 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 6c9f885badd..21772b6cff0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2596,13 +2596,16 @@ kill_command (const char *arg, int from_tty)
>    if (!query (_("Kill the program being debugged? ")))
>      error (_("Not confirmed."));
>  
> -  std::string pid_str = target_pid_to_str (inferior_ptid);
> +  int pid = current_inferior ()->pid;
> +  /* Save the pid as a string before killing the inferior, since that
> +     may unpush the current target, and we need the string after.  */
> +  std::string pid_str = target_pid_to_str (pid_to_ptid (pid));
>    int infnum = current_inferior ()->num;
>  
>    target_kill ();
>  
>    if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d (%s) has been killed]\n"),
> +    printf_unfiltered (_("[Inferior %d (%s) killed]\n"),
>  		       infnum, pid_str.c_str ());
>  
>    /* If we still have other inferiors to debug, then don't mess with
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 61d1dcb5738..193037b6e7a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5138,7 +5138,9 @@ remote_detach_1 (int from_tty, inferior *inf)
>       breakpoints that should be available for the followed inferior.  */
>    if (!is_fork_parent)
>      {
> -      std::string infpid = target_pid_to_str (inferior_ptid);
> +      /* Save the pid as a string before mourning, since that will
> +	 unpush the remote target, and we need the string after.  */
> +      std::string infpid = target_pid_to_str (pid_to_ptid (pid));
>  
>        target_mourn_inferior (inferior_ptid);
>        if (print_inferior_events)
> diff --git a/gdb/testsuite/gdb.base/hook-stop.exp b/gdb/testsuite/gdb.base/hook-stop.exp
> index 5717f94fc2a..b3f4084af1e 100644
> --- a/gdb/testsuite/gdb.base/hook-stop.exp
> +++ b/gdb/testsuite/gdb.base/hook-stop.exp
> @@ -86,7 +86,7 @@ proc hook_stop_kill {} {
>  
>  	set test "run hook-stop"
>  	gdb_test_multiple "continue" "$test" {
> -	    -re "Continuing.\r\n\\\[Inferior $decimal \\(.*\\) has been killed\\\]\r\n${gdb_prompt} $" {
> +	    -re "Continuing.\r\n\\\[Inferior $decimal \\(.*\\) killed\\\]\r\n${gdb_prompt} $" {
>  		pass $test
>  	    }
>  	}
> diff --git a/gdb/testsuite/gdb.base/kill-after-signal.exp b/gdb/testsuite/gdb.base/kill-after-signal.exp
> index 67e6f627504..8830d492383 100644
> --- a/gdb/testsuite/gdb.base/kill-after-signal.exp
> +++ b/gdb/testsuite/gdb.base/kill-after-signal.exp
> @@ -39,6 +39,6 @@ gdb_test "continue" "Program received signal SIGUSR1, .*"
>  gdb_test "stepi" "\r\nhandler .*"
>  gdb_test_multiple "kill" "kill" {
>      -re "Kill the program being debugged\\? \\(y or n\\) $" {
> -       gdb_test "y" "\\\[Inferior $decimal \\(.*\\) has been killed\\\]" "kill"
> +       gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"
>      }
>  }
> diff --git a/gdb/testsuite/gdb.threads/kill.exp b/gdb/testsuite/gdb.threads/kill.exp
> index aea4a986c64..82f221eef67 100644
> --- a/gdb/testsuite/gdb.threads/kill.exp
> +++ b/gdb/testsuite/gdb.threads/kill.exp
> @@ -70,7 +70,7 @@ proc test {threaded} {
>  
>  	gdb_test_multiple "kill" "kill" {
>  	    -re "Kill the program being debugged\\? \\(y or n\\) $" {
> -		gdb_test "y" "\\\[Inferior $decimal \\(.*\\) has been killed\\\]" "kill"
> +		gdb_test "y" "\\\[Inferior $decimal \\(.*\\) killed\\\]" "kill"
>  	    }
>  	}
>      }
> -- 
> 2.14.3

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/