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

Message ID 6839f603-5359-f2d3-efa9-b8d55accfff2@redhat.com
State New
Headers show
Series
  • Fix new inferior events output (Re: [PATCH v7] Enable 'set print inferior-events' and improve detach/fork/kill/exit messages)
Related show

Commit Message

Pedro Alves April 25, 2018, 5:41 p.m.
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.

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(-)

-- 
2.14.3

Comments

Sergio Durigan Junior April 25, 2018, 5:53 p.m. | #1
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/
Pedro Alves April 25, 2018, 6:07 p.m. | #2
On 04/25/2018 06:53 PM, Sergio Durigan Junior wrote:

> 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.


No worries.  Now pushed.

Thanks,
Pedro Alves

Patch

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"
 	    }
 	}
     }