Patchwork [v2] Enable 'set print inferior-events' and cleanup attach/detach messages

login
register
mail settings
Submitter Pedro Alves
Date Feb. 1, 2018, 5:17 p.m.
Message ID <79c5d1bc-d3de-494d-d559-9d4c848ebc0c@redhat.com>
Download mbox | patch
Permalink /patch/25727/
State New
Headers show

Comments

Pedro Alves - Feb. 1, 2018, 5:17 p.m.
On 01/31/2018 04:57 PM, Sergio Durigan Junior wrote:
> This is a followup of Pedro's suggestion to turn 'set print
> inferior-events' always on, and do some cleanup on the messages
> printed by GDB when attaching/detaching a inferior, or when it exits.
> 
> To make sure that the patch is correct, I've tested it with a handful
> of combinations of 'set follow-fork-mode', 'set detach-on-fork' and
> 'set print inferior-events'.  In the end, I decided to make my
> hand-made test into an official testcase.  More on that below.
> 
> I've also made a few modifications to messages that are printed when
> 'set print inferior-events' is on.  For example, a few of the messages
> did not contain the '[' and ']' as prefix/suffix, which led to a few
> inconsistencies like:
> 
>   Attaching after process 22995 fork to child process 22995.
>   [New inferior 22999]
>   Detaching after fork from child process 22999.
>   [Inferior 22995 detached]
>   [Inferior 2 (process 22999) exited normally]
> 
> So I took the opportunity and included the square brackets where
> applicable.
> 
> As suggested by Pedro, the "[Inferior %d exited]" message from
> 'exit_inferior' has been removed, because it got duplicated when
> 'inferior-events' is on.  I'm also using the
> 'add_{thread,inferior}_silent' versions (instead of their verbose
> counterparts) on some locations, also to avoid duplicated messages.

Can you give examples of the latter?  (what led to 'add_{thread,inferior}_silent')

> As for the tests, the configuration options being exercised are:
> 
> - follow-fork-mode: child/parent
> - detach-on-fork: on/off
> - print inferior-events: on/off
> 
> Built and regtested on BuildBot, without regressions.

Can you update the log to include examples of what the new
output looks like?  Both "set inferior-events on/off".
That makes it much easier to review/discuss/evaluate.

Can you say what happens to the output with
"set print inferior-events off" compared to current master?
Does output change in that case compared to today?  Not changing
output is not the goal, but it'd be good to be clear.

I'm experimenting a bit, and I'm seeing some inconsistencies
that I wonder whether we should consider revising.

For example, with "set print inferior-events on/off", we see,
for detach:

 (gdb) detach
 Detaching from program: /home/pedro/gdb/build/gdb/testsuite/outputs/gdb.base/foll-fork/foll-fork, process 2685
 [Inferior 2685 detached]
 (gdb) 

But for kill, we see:

 (gdb) k
 Kill the program being debugged? (y or n) y
 (gdb) 

Should detach vs kill be consistent?  I'd think so off hand.

Another thing is related to what I was proposing in the other
thread.  With the patch, we say:

 (gdb) c
 Continuing.
 [New inferior 3239]
 [Inferior 5 (process 3236) exited normally]

The "New inferior 3239" one is showing the target process ID,
which is confusing.  I think we should tweak to mention the
inferior ID, like:

 (gdb) c
 Continuing.
 [New inferior 5 (process 3236)]
 [Inferior 5 (process 3236) exited normally]


Looking at follow-fork child, detach-on-fork on, we see:

 [Attaching after process 12069 fork to child process 12069.]
 [New inferior 12073]
 [Detaching after fork from child process 12073.]
 [Inferior 12069 detached]
 [Inferior 2 (process 12073) exited normally]

That could use some normalization to make messages consistent
with one another.

 [Attaching after process 12069 fork to child process 12069.]
 [New inferior 2 (process 12073)]
 [Detaching after fork from child process 12073.]
 [Inferior 1 (process 12069) detached]
 [Inferior 2 (process 12073) exited normally]

(see more about this case below)

>  
>  	      target_terminal::ours_for_output ();
>  	      fprintf_filtered (gdb_stdlog,
> -				_("Detaching after %s from child %s.\n"),
> +				_("[Detaching after %s from child %s.]\n"),
>  				has_vforked ? "vfork" : "fork",
>  				target_pid_to_str (process_ptid));

I also noticed that this ends up with a period at the end, while
other [] notices don't, like (playing with "set detach-on-fork on"):

 [Detaching after fork from child process 3417.]
 [Inferior 5 (process 3416) exited normally]

> diff --git a/gdb/testsuite/gdb.base/fork-detach-info.exp b/gdb/testsuite/gdb.base/fork-detach-info.exp
> new file mode 100644
> index 0000000000..aa9a85c0d5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/fork-detach-info.exp

Maybe this could use a different name, since this isn't
just about _detach_ info?

fork-print-inferior-events.exp, maybe.

> @@ -0,0 +1,68 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2007-2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +

Please add an intro comment mentioning what the testcase is about.

> +if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
> +    untested "not supported on gdbserver"
> +    return
> +}

I see that this relies on "run", so can't work with use_gdb_stub.
But why is this not supported with extended-remote gdbserver?

> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +# This is the expected output for each of the test combinations
> +# below.  The order here is important:
> +#
> +#    follow-fork: child; detach-on-fork: on; inferior-events: on
> +#    follow-fork: child; detach-on-fork: on; inferior-events: off
> +#    follow-fork: child; detach-on-fork: off; inferior-events: on
> +#    follow-fork: child; detach-on-fork: off; inferior-events: off
> +#    follow-fork: parent; detach-on-fork: on; inferior-events: on
> +#    follow-fork: parent; detach-on-fork: on; inferior-events: off
> +#    follow-fork: parent; detach-on-fork: off; inferior-events: on
> +#    follow-fork: parent; detach-on-fork: off; inferior-events: off
> +set expected_output [list \
> +    "\\\[Attaching after process $decimal fork to child process $decimal\\.\\\]\r\n\\\[New inferior $decimal\\\]\r\n\\\[Detaching after fork from child process $decimal\\.\\\]\r\n\\\[Inferior $decimal detached\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Attaching after process $decimal fork to child process $decimal\\.\\\]\r\n\\\[New inferior $decimal\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Detaching after fork from child process $decimal\\.\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[New inferior $decimal\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
> +    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]"]
> +

I had a lot of trouble making sense of this big blurb of expected output.
I didn't know exactly what to suggest, so I played with it locally, and
I think putting the individual messages in variables helps a lot.
And then, I noticed that all the cases end up with 

  \\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]

so we can factor it out.

And then, if we iterate over print_inferior_events first, then
it's super clear that the "set print-inferior-events off"
case is always "no output".  See attached patch on top of yours.

The infrun.c hunk is there in my patch because while playing with
this, I noticed that gdb says that it's detaching the child
(and uses the child's pid), when in fact it is detaching the
parent!

I.e., currently, "set follow-fork child", "set detach-on-fork on":

 [Attaching after process 12069 fork to child process 12069.]
 [New inferior 2 12073]
 [Detaching after fork from child process 12073.]   <<< nope, it's detaching 12069 / the parent
 [Inferior 12069 detached]                          <<< see?
 [Inferior 2 (process 12073) exited normally]

after that fix:

 [Attaching after process 18500 fork to child process 18500.]
 [New inferior 18504]
 [Detaching after fork from parent process 18500.]
 [Inferior 18500 detached]
 [Inferior 2 (process 18504) exited normally]

Thanks,
Pedro Alves

Patch

From 7394aafa8f9126521c12e3a822666f75906059a3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 1 Feb 2018 17:11:43 +0000
Subject: [PATCH] inferior-events

---
 gdb/infrun.c                                |  4 +--
 gdb/testsuite/gdb.base/fork-detach-info.exp | 51 +++++++++++++++++------------
 2 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4ac5bde3035..85feedf6d4e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -597,12 +597,12 @@  holding the child stopped.  Try \"set detach-on-fork\" or \
 	  if (print_inferior_events)
 	    {
 	      /* Ensure that we have a process ptid.  */
-	      ptid_t process_ptid = pid_to_ptid (ptid_get_pid (child_ptid));
+	      ptid_t process_ptid = pid_to_ptid (ptid_get_pid (parent_ptid));
 
 	      target_terminal::ours_for_output ();
 	      fprintf_filtered (gdb_stdlog,
 				_("[Detaching after fork from "
-				  "child %s.]\n"),
+				  "parent %s.]\n"),
 				target_pid_to_str (process_ptid));
 	    }
 
diff --git a/gdb/testsuite/gdb.base/fork-detach-info.exp b/gdb/testsuite/gdb.base/fork-detach-info.exp
index aa9a85c0d5d..bc4aa5edcdd 100644
--- a/gdb/testsuite/gdb.base/fork-detach-info.exp
+++ b/gdb/testsuite/gdb.base/fork-detach-info.exp
@@ -29,38 +29,47 @@  if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
 # This is the expected output for each of the test combinations
 # below.  The order here is important:
 #
-#    follow-fork: child; detach-on-fork: on; inferior-events: on
-#    follow-fork: child; detach-on-fork: on; inferior-events: off
-#    follow-fork: child; detach-on-fork: off; inferior-events: on
-#    follow-fork: child; detach-on-fork: off; inferior-events: off
-#    follow-fork: parent; detach-on-fork: on; inferior-events: on
-#    follow-fork: parent; detach-on-fork: on; inferior-events: off
-#    follow-fork: parent; detach-on-fork: off; inferior-events: on
-#    follow-fork: parent; detach-on-fork: off; inferior-events: off
+#    inferior-events: on;  follow-fork: child;  detach-on-fork: on
+#    inferior-events: on;  follow-fork: child;  detach-on-fork: off
+#    inferior-events: on;  follow-fork: parent; detach-on-fork: on
+#    inferior-events: on;  follow-fork: parent; detach-on-fork: off
+#    inferior-events: off; follow-fork: child;  detach-on-fork: on
+#    inferior-events: off; follow-fork: child;  detach-on-fork: off
+#    inferior-events: off; follow-fork: parent; detach-on-fork: on
+#    inferior-events: off; follow-fork: parent; detach-on-fork: off
+
+set exited_normally_re "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]"
+set attach_child_re "\\\[Attaching after process $decimal fork to child process $decimal\\.\\\]\r\n"
+set detach_child_re "\\\[Detaching after fork from child process $decimal\\.\\\]\r\n"
+set detach_parent_re "\\\[Detaching after fork from parent process $decimal\\.\\\]\r\n"
+set new_inf_re "\\\[New inferior $decimal\\\]\r\n"
+set inf_detached_re "\\\[Inferior $decimal detached\\\]\r\n"
+
 set expected_output [list \
-    "\\\[Attaching after process $decimal fork to child process $decimal\\.\\\]\r\n\\\[New inferior $decimal\\\]\r\n\\\[Detaching after fork from child process $decimal\\.\\\]\r\n\\\[Inferior $decimal detached\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Attaching after process $decimal fork to child process $decimal\\.\\\]\r\n\\\[New inferior $decimal\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Detaching after fork from child process $decimal\\.\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[New inferior $decimal\\\]\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
-    "\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]"]
+			 "${attach_child_re}${new_inf_re}${detach_parent_re}${inf_detached_re}" \
+			 "${attach_child_re}${new_inf_re}" \
+			 "${detach_child_re}" \
+			 "${new_inf_re}" \
+			 "" \
+			 "" \
+			 "" \
+			 "" \
+			]
 
 set i 0
 
-foreach_with_prefix follow_fork_mode { "child" "parent" } {
-    foreach_with_prefix detach_on_fork { "on" "off" } {
-	foreach_with_prefix print_inferior_events { "on" "off" } {
+foreach_with_prefix print_inferior_events { "on" "off" } {
+    foreach_with_prefix follow_fork_mode { "child" "parent" } {
+	foreach_with_prefix detach_on_fork { "on" "off" } {
 	    clean_restart $binfile
+	    gdb_test_no_output "set print inferior-events $print_inferior_events"
 	    gdb_test_no_output "set follow-fork-mode $follow_fork_mode"
 	    gdb_test_no_output "set detach-on-fork $detach_on_fork"
-	    gdb_test_no_output "set print inferior-events $print_inferior_events"
 
 	    set output [lindex $expected_output $i]
 	    # Always add the "Starting program..." string so that we
 	    # match exactly the lines we want.
-	    set output "Starting program: $binfile\\s*\r\n$output"
+	    set output "Starting program: $binfile\\s*\r\n$output${exited_normally_re}"
 	    set i [expr $i + 1]
 	    gdb_test "run" $output
 	}
-- 
2.14.3