Always print "Detaching after fork from child..."

Message ID 20180124194714.26222-1-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Jan. 24, 2018, 7:47 p.m. UTC
  I'd like to propose another patch that we carry on Fedora GDB.

It's a simple patch, whose purpose is to always print the "Detaching
after fork from child %s." message after a fork/vfork happens.
Currently the user can see this message on upstream GDB when she
enables "debug infrun" or sets verbosity to "on".

The first version of this patch was posted by Jan Kratochvil here:

  https://www.sourceware.org/ml/gdb-patches/2007-12/msg00087.html

But after a reply from Daniel Jacobowitz nothing else happened, until
6c95b8df7fe, which actually introduced the messages as part of a
bigger patch by Pedro.

I guess the point here is that informing the user that a fork happened
is a good thing, and shouldn't be something done only when "debug
inferior" or "verbose" are on.  I remember a few occasions when, while
debugging something using Fedora's GDB, I saw this message and was
able to better track the problem.  Last, but not least, this patch is
being carried with Fedora GDB because of an actual complaint:

  https://bugzilla.redhat.com/show_bug.cgi?id=235197

Below you can see what changes on the output.

With a simple test program:

  #include <unistd.h>

  int
  main (int argc, char *argv[])
  {
    fork ();

    return 0;
  }

Here's the current output, without the patch:

  (gdb) r
  Starting program: /tmp/a.out
  [Inferior 1 (process 3151) exited normally]

And here's the output with the patch:

  (gdb) r
  Starting program: /tmp/a.out
  Detaching after fork from child process 24905.
  [Inferior 1 (process 24901) exited normally]

I've had to adapt gdb.base/catch-syscall.exp's usage of
"gdb_continue_to_end", but other than that there are no regressions
introduced by this change.

gdb/ChangeLog:
2018-01-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* infrun.c (): Always print 'fork detach' message.

gdb/testsuite/ChangeLog:
2018-01-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/catch-syscall.exp (check_for_program_end): Adjust
	"gdb_continue_to_end".
	(test_catch_syscall_with_wrong_args): Likewise.
	* gdb.base/fork-detach-info.c: New file.
	* gdb.base/fork-detach-info.exp: New file.
---
 gdb/infrun.c                                | 17 ++++++-------
 gdb/testsuite/gdb.base/catch-syscall.exp    |  4 ++--
 gdb/testsuite/gdb.base/fork-detach-info.c   | 37 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/fork-detach-info.exp | 27 +++++++++++++++++++++
 4 files changed, 73 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/fork-detach-info.c
 create mode 100644 gdb/testsuite/gdb.base/fork-detach-info.exp
  

Comments

Jan Kratochvil Jan. 24, 2018, 8:43 p.m. UTC | #1
On Wed, 24 Jan 2018 20:47:14 +0100, Sergio Durigan Junior wrote:
>   https://bugzilla.redhat.com/show_bug.cgi?id=235197

As a justification for this patch:

------------------------------------------------------------------------------
cat >fork2.c <<EOH
#include <unistd.h>
#include <stdio.h>
static void printit(void) {
  puts("printed");
}
int main(void) {
  if (!fork()) printit();
  return 0;
}
EOH
gcc -o fork2 fork2.c -Wall -g
gdb -q ./fork2
(gdb) b printit
Breakpoint 1 at 0x40052b: file fork2.c, line 4.
(gdb) r
Starting program: /quad/home/jkratoch/t/fork2 
printed
[Inferior 1 (process 15812) exited normally]
(gdb) q
------------------------------------------------------------------------------

As the GDB user does not expect the program could do any forks s/he is
confused the breakpoint did not get hit and assumes GDB is just broken.

But then I cannot say this patch is too great, it produces many uninteresting 
	Detaching after fork from child process 24905.
messages rather just annoying in most cases.  So nowadays I feel the message
is more an excuse how to show it is user's fault s/he did not read it.
But I think nobody reads them as there are too many such messages.

I believe the right fix would be to make "set detach-on-fork off" the default.
But that is sure a new can of worms I do not want to speculate about.


Jan
  
Sergio Durigan Junior Jan. 24, 2018, 8:56 p.m. UTC | #2
On Wednesday, January 24 2018, Jan Kratochvil wrote:

> On Wed, 24 Jan 2018 20:47:14 +0100, Sergio Durigan Junior wrote:
>>   https://bugzilla.redhat.com/show_bug.cgi?id=235197
>
> As a justification for this patch:
>
> ------------------------------------------------------------------------------
> cat >fork2.c <<EOH
> #include <unistd.h>
> #include <stdio.h>
> static void printit(void) {
>   puts("printed");
> }
> int main(void) {
>   if (!fork()) printit();
>   return 0;
> }
> EOH
> gcc -o fork2 fork2.c -Wall -g
> gdb -q ./fork2
> (gdb) b printit
> Breakpoint 1 at 0x40052b: file fork2.c, line 4.
> (gdb) r
> Starting program: /quad/home/jkratoch/t/fork2 
> printed
> [Inferior 1 (process 15812) exited normally]
> (gdb) q
> ------------------------------------------------------------------------------
>
> As the GDB user does not expect the program could do any forks s/he is
> confused the breakpoint did not get hit and assumes GDB is just broken.

Thanks for the extra justification and the useful example.

> But then I cannot say this patch is too great, it produces many uninteresting 
> 	Detaching after fork from child process 24905.
> messages rather just annoying in most cases.  So nowadays I feel the message
> is more an excuse how to show it is user's fault s/he did not read it.
> But I think nobody reads them as there are too many such messages.

I understand where you're coming from, but I still think this is a good
patch because I read the messages, and as I said, they even helped me in
one occasion.

> I believe the right fix would be to make "set detach-on-fork off" the default.
> But that is sure a new can of worms I do not want to speculate about.

Yeah, I can see the rationale for this, and I think it's an idea worth
discussing.  Not sure what others think, but I also don't want deviate
much from what the current patch is proposing.

Thanks,
  

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 45fe36a717..cca6e22bb4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -461,17 +461,14 @@  holding the child stopped.  Try \"set detach-on-fork\" or \
 	      remove_breakpoints_pid (ptid_get_pid (inferior_ptid));
 	    }
 
-	  if (info_verbose || debug_infrun)
-	    {
-	      /* Ensure that we have a process ptid.  */
-	      ptid_t process_ptid = pid_to_ptid (ptid_get_pid (child_ptid));
+	  /* Ensure that we have a process ptid.  */
+	  ptid_t process_ptid = pid_to_ptid (ptid_get_pid (child_ptid));
 
-	      target_terminal::ours_for_output ();
-	      fprintf_filtered (gdb_stdlog,
-				_("Detaching after %s from child %s.\n"),
-				has_vforked ? "vfork" : "fork",
-				target_pid_to_str (process_ptid));
-	    }
+	  target_terminal::ours_for_output ();
+	  fprintf_filtered (gdb_stdlog,
+			    _("Detaching after %s from child %s.\n"),
+			    has_vforked ? "vfork" : "fork",
+			    target_pid_to_str (process_ptid));
 	}
       else
 	{
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index 2a8bf27e5c..20fa041155 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -179,7 +179,7 @@  proc check_for_program_end {} {
     # Deleting the catchpoints
     delete_breakpoints
 
-    gdb_continue_to_end
+    gdb_continue_to_end "" continue 1
 }
 
 proc test_catch_syscall_without_args {} {
@@ -250,7 +250,7 @@  proc test_catch_syscall_with_wrong_args {} {
 	# If it doesn't, everything is right (since we don't have
 	# a syscall named "mlock" in it).  Otherwise, this is a failure.
 	set thistest "catch syscall with unused syscall ($syscall_name)"
-	gdb_continue_to_end $thistest
+	gdb_continue_to_end $thistest continue 1
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/fork-detach-info.c b/gdb/testsuite/gdb.base/fork-detach-info.c
new file mode 100644
index 0000000000..182a363dcc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-detach-info.c
@@ -0,0 +1,37 @@ 
+/* 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/>.  */
+
+#include <stdlib.h>
+#include <unistd.h>
+
+int
+main (int argc, char *argv[])
+{
+  pid_t child;
+
+  child = fork ();
+  switch (child)
+    {
+      case -1:
+	abort ();
+      case 0:
+      default:
+	break;
+    }
+
+  return 0;
+}
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..b58c4ba6bf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fork-detach-info.exp
@@ -0,0 +1,27 @@ 
+# 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/>.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+set test "print detach message after fork"
+gdb_test "r" \
+    "Detaching after fork from child process $decimal\.\r\n\\\[Inferior $decimal \\(process $decimal\\) exited normally\\\]" \
+    $test