diff mbox

[02/16,v2] Refactor follow-fork message printing

Message ID 542F3692.6090402@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Oct. 3, 2014, 11:51 p.m. UTC
On 9/26/2014 1:14 PM, Breazeal, Don wrote:
> On 9/26/2014 12:52 PM, Pedro Alves wrote:
>> On 08/21/2014 01:29 AM, Don Breazeal wrote:
>>> This patch refactors the code that prints messages related to follow-fork
>>> into functions, and adds a call so that a message is now printed when the
>>> parent process is detached.  Previously in this case the only message was
>>> notification of attaching to the child.  We still do not print any messages
>>> when following the parent and detaching the child (the default).  My 
>>> rationale for this is that from the user's perspective the new child was
>>> never attached.
>>>
>>> The messages now distinguish between fork and vfork.
>>>
>>> Note that all of these messages are only printed when 'verbose' is set or
>>> when debugging is turned on.
>>>
>>> This is preparatory work for follow-fork and detach-on-fork on
>>> extended-remote linux targets.
>>>
>>> The test gdb.base/foll-fork.exp was modified to check for the new message.
>>>
>>> Tested on x64 Ubuntu Lucid, native only.
>>>
>>> Thanks,
>>> --Don
>>>
>>> gdb/
>>> 2014-08-20  Don Breazeal  <donb@codesourcery.com>
>>>
>>> 	* gdb/infrun.c (print_fork_attach): New function.
>>> 	(print_fork_detach): New function.
>>> 	(follow_fork_inferior): Call print_fork_attach and print_fork_detach.
>>> 	(handle_vfork_child_exec_or_exit): Ditto.
>>>
>>> gdb/testsuite/
>>> 2014-08-20  Don Breazeal  <donb@codesourcery.com>
>>>
>>> 	* gdb.base/foll-fork.exp (test_follow_fork): Add check for new
>>> 	detach message.
>>> 	(catch_fork_child_follow): Ditto.
>>> 	* gdb.base/foll-vfork.exp (vfork_parent_follow_through_step):
>>> 	Modify to check for "vfork" instead of "fork".
>>> 	(vfork_parent_follow_to_bp): Ditto.
>>> 	(vfork_and_exec_child_follow_through_step): Ditto.
>>> 	(vfork_and_exec_child_follow_to_main_bp): Ditto, plus add check
>>> 	for new detach message.
>>>
>>> ---
>>>  gdb/infrun.c                          |   94 +++++++++++++++++++-------------
>>>  gdb/testsuite/gdb.base/foll-fork.exp  |   12 +++--
>>>  gdb/testsuite/gdb.base/foll-vfork.exp |    8 ++--
>>>  3 files changed, 68 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index a51c759..34e9295 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -567,6 +567,49 @@ follow_fork (void)
>>>    return should_resume;
>>>  }
>>>  
>>> +/* Print details about attaching to a process after a fork call.  */
>>> +
>>> +static void
>>> +print_fork_attach (pid_t child_pid, pid_t parent_pid, int is_vfork)
>>
>> As this is called for the child only, I think it'd be good to make
>> that explicit in the name.  E.g., print_attach_fork_child.

Done.

>>
>>> +{
>>> +  if (info_verbose || debug_infrun)
>>> +    {
>>> +      target_terminal_ours ();
>>
>> We should really be using target_terminal_ours_for_output for
>> output instead.

Done in both print routines.

>>
>>> +      fprintf_filtered (gdb_stdlog,
>>> +			_("Attaching after process %d "
>>> +			  "%s to child process %d.\n"),
>>> +			parent_pid, is_vfork?"vfork":"fork", child_pid);
>>
>> Spaces around "?" and ":":  'is_vfork ? "vfork" : "fork"'

Done.

>>
>>
>>> +    }
>>> +}
>>> +
>>> +/* Print details about detaching from a process after a fork call.  */
>>> +
>>> +static void
>>> +print_fork_detach (pid_t pid, int is_parent, int is_vfork, char *vfork_action)
>>> +{
>>> +  if (info_verbose || debug_infrun)
>>> +    {
>>> +      target_terminal_ours ();
>>> +
>>> +      if (is_parent && is_vfork)
>>> +	{
>>> +	  /* Detaching a vfork parent, so print what the child did
>>> +	     that allows the parent to resume.  */
>>> +	  gdb_assert (vfork_action != NULL && strlen (vfork_action) > 0);
>>
>> Write: '*vfork_action != '\0' instead of that strlen.

Changed the type to enum target_waitkind.

>>
>>> +	  fprintf_filtered (gdb_stdlog,
>>> +			    "Detaching vfork parent process %d after"
>>> +			    " child %s.\n", pid, vfork_action);
>>
>> This handling of vfork_action is bad for i18n.  While at it, this is
>> missing _().  More below.

Changed vfork_action to be of type enum target_waitkind, requiring
TARGET_WAITKIND_EXECD or TARGET_WAITKIND_EXITED for the vfork parent
detach messages.  It's now handled it much like the handling of the
"fork" vs. "vfork" strings.

>>
>>> +	}
>>> +      else
>>> +	{
>>> +	  fprintf_filtered (gdb_stdlog,
>>> +			    _("Detaching after %s from %s process %d.\n"),
>>> +			    is_vfork?"vfork":"fork",
>>> +			    is_parent?"parent":"child", pid);
>>
>> Spaces around operators.  "parent" and "child" really shouldn't
>> be passed as %s, as this will be awkward when translated.  We should
>> split those out instead, like:
>>
>> if (is_parent)
>>   {
>>      fprintf_filtered (gdb_stdlog,
>> 		       _("Detaching after %s from parent process %d.\n"),
>>                        is_vfork ? "vfork" : "fork", pid);
>>   }
>> else
>>   {
>>      fprintf_filtered (gdb_stdlog,
>> 		       _("Detaching after %s from child process %d.\n"),
>> 		       is_vfork ? "vfork" : "fork", pid);
>>   }
> 
> Just so I understand (and don't repeat the error), is the problem here
> that "parent" and "child" are (a) strings that would need to be
> translated, and (b) not in the printf format string?

I changed this more-or-less as you suggested.  It would help me to have
the translation problem explained.

> 
>>
>> But after unrolling this, is there really any benefit to
>> print_fork_detach?  It doesn't seem that it'll ever end
>> up called twice with the same arguments...  Seems like
>> we may be obfuscating more than clarifying with the patch.
> 
> My experience of reading and understanding the code was improved by
> moving the blocks of printing code out of follow-fork.  So for me, it
> would be desirable even with a print function for each permutation of
> the messages.  But it's just a personal preference, so if you'd rather
> just drop the whole patch, that's OK with me.  Let me know and I'll
> either make the requested changes above, or re-work my local branch to
> drop this patch.
>
I've updated this patch based on your comments, and included it
below.  It may not address your "obfuscating more than clarifying"
concern.  My take on that issue, stated in a different way, is
that moving the message printing code into separate functions
makes the code in follow_fork_inferior and
handle_vfork_child_exec_or_exit cleaner, especially given that there
is now an additional detach message.  I guess the question is
whether that benefit offsets the complexity of print_fork_detach.

Given the updated patch, WDYT?

Tested this by running gdb.base/foll-fork.exp and
gdb.base/foll-vfork.exp on x64 Ubuntu.

Thanks
--Don

gdb/
2014-10-03  Don Breazeal  <donb@codesourcery.com>

	* gdb/infrun.c (print_fork_attach_child): New function.
	(print_fork_detach): New function.
	(follow_fork_inferior): Call print_fork_attach_child and
	print_fork_detach.
	(handle_vfork_child_exec_or_exit): Call print_fork_detach.

gdb/testsuite/
2014-10-03  Don Breazeal  <donb@codesourcery.com>

	* gdb.base/foll-fork.exp (test_follow_fork): Add check for new
	detach message.
	(catch_fork_child_follow): Ditto.
	* gdb.base/foll-vfork.exp (vfork_parent_follow_through_step):
	Modify to check for "vfork" instead of "fork".
	(vfork_parent_follow_to_bp): Ditto.
	(vfork_and_exec_child_follow_through_step): Ditto.
	(vfork_and_exec_child_follow_to_main_bp): Ditto, plus add check
	for new detach message.

---
 gdb/infrun.c                          | 115
+++++++++++++++++++++++-----------
 gdb/testsuite/gdb.base/foll-fork.exp  |  12 ++--
 gdb/testsuite/gdb.base/foll-vfork.exp |  10 +--
 3 files changed, 92 insertions(+), 45 deletions(-)

        }
    }
@@ -171,7 +171,7 @@ proc vfork_child_follow_to_exit {} {
 	  # PR gdb/14766
 	  fail "$test"
       }
-      -re "Attaching after.* vfork to.*Detaching vfork parent .* after
child exit.*$gdb_prompt " {
+      -re "Attaching after.* vfork to.*Detaching from vfork parent .*
after child exit.*$gdb_prompt " {
 	  pass $test
       }
    }
@@ -195,7 +195,7 @@ proc vfork_and_exec_child_follow_to_main_bp {} {

    set test "continue to bp"
    gdb_test_multiple "continue" $test {
-      -re "Attaching after.* vfork to.*xecuting new
program.*Breakpoint.*vforked-prog.c:${linenum}.*$gdb_prompt " {
+      -re "Attaching after.* vfork to.*Detaching from vfork
parent.*xecuting new
program.*Breakpoint.*vforked-prog.c:${linenum}.*$gdb_prompt " {
 	  pass $test
       }
    }
@@ -239,7 +239,7 @@ proc vfork_and_exec_child_follow_through_step {} {
        #
        set linenum [gdb_get_line_number "printf(\"Hello from
vforked-prog" ${srcfile2}]
        gdb_test_multiple "next" $test {
-	   -re "Attaching after fork to.*Executing new
program.*Breakpoint.*vforked-prog.c:${linenum}.*$gdb_prompt " {
+	   -re "Attaching after vfork to.*Executing new
program.*Breakpoint.*vforked-prog.c:${linenum}.*$gdb_prompt " {
 	       pass "$test"
 	   }
        }
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 728c160..703ebfe 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -401,6 +401,66 @@  show_follow_fork_mode_string (struct ui_file *file,
int from_tty,
 }
 

+/* Print details about attaching to a child process after a fork call.  */
+
+static void
+print_attach_fork_child (pid_t child_pid, pid_t parent_pid, int is_vfork)
+{
+  if (info_verbose || debug_infrun)
+    {
+      target_terminal_ours_for_output ();
+      fprintf_filtered (gdb_stdlog,
+			_("Attaching after process %d %s to child "
+			  "process %d.\n"),
+			parent_pid, is_vfork ? "vfork" : "fork", child_pid);
+    }
+}
+
+/* Print details about detaching from a process after a fork call.
+   VFORK_ACTION must be one of TARGET_WAITKIND_EXECD,
TARGET_WAITKIND_EXITED,
+   or TARGET_WAITKIND_IGNORE.  Here TARGET_WAITKIND_IGNORE is a placeholder
+   for cases where we are detaching a vfork child and the argument is
+   unused.  */
+
+static void
+print_fork_detach (pid_t pid, int is_parent, int is_vfork,
+		   enum target_waitkind vfork_action)
+{
+  if (info_verbose || debug_infrun)
+    {
+      target_terminal_ours_for_output ();
+
+      if (is_parent)
+	{
+	  if (is_vfork)
+	    {
+	      /* Detaching a vfork parent, so print what the child did
+		 that allows the parent to resume.  */
+	      gdb_assert (vfork_action == TARGET_WAITKIND_EXECD
+			  || vfork_action == TARGET_WAITKIND_EXITED);
+	      fprintf_filtered (gdb_stdlog,
+				_("Detaching from vfork parent process %d"
+				  " after child %s.\n"),
+				pid, (vfork_action == TARGET_WAITKIND_EXECD
+				      ? "exec" : "exit"));
+	    }
+	  else
+	    {
+	      /* Detaching fork parent.  */
+	      fprintf_filtered (gdb_stdlog, _("Detaching after fork from "
+					      "parent process %d.\n"), pid);
+	    }
+	}
+      else
+	{
+	  /* Detaching fork or vfork child.  */
+	  fprintf_filtered (gdb_stdlog,
+			    _("Detaching after %s from child process %d.\n"),
+			    is_vfork ? "vfork" : "fork", pid);
+	}
+    }
+}
+
 /* Handle changes to the inferior list based on the type of fork,
    which process is being followed, and whether the other process
    should be detached.  On entry inferior_ptid must be the ptid of
@@ -459,14 +519,8 @@  holding the child stopped.  Try \"set
detach-on-fork\" or \
 	      remove_breakpoints_pid (ptid_get_pid (inferior_ptid));
 	    }

-	  if (info_verbose || debug_infrun)
-	    {
-	      target_terminal_ours ();
-	      fprintf_filtered (gdb_stdlog,
-				"Detaching after fork from "
-				"child process %d.\n",
-				child_pid);
-	    }
+	  print_fork_detach (child_pid, follow_child, has_vforked,
+			     TARGET_WAITKIND_IGNORE);
 	}
       else
 	{
@@ -547,20 +601,7 @@  holding the child stopped.  Try \"set
detach-on-fork\" or \
       struct inferior *parent_inf, *child_inf;
       struct program_space *parent_pspace;

-      if (info_verbose || debug_infrun)
-	{
-	  target_terminal_ours ();
-	  if (has_vforked)
-	    fprintf_filtered (gdb_stdlog,
-			      _("Attaching after process %d "
-				"vfork to child process %d.\n"),
-			      parent_pid, child_pid);
-	  else
-	    fprintf_filtered (gdb_stdlog,
-			      _("Attaching after process %d "
-				"fork to child process %d.\n"),
-			      parent_pid, child_pid);
-	}
+      print_attach_fork_child (child_pid, parent_pid, has_vforked);

       /* Add the new inferior first, so that the target_detach below
 	 doesn't unpush the target.  */
@@ -596,7 +637,11 @@  holding the child stopped.  Try \"set
detach-on-fork\" or \
 	  parent_inf->waiting_for_vfork_done = 0;
 	}
       else if (detach_fork)
-	target_detach (NULL, 0);
+	{
+	  print_fork_detach (parent_pid, follow_child, has_vforked,
+			     TARGET_WAITKIND_IGNORE);
+	  target_detach (NULL, 0);
+	}

       /* Note that the detach above makes PARENT_INF dangling.  */

@@ -928,20 +973,18 @@  handle_vfork_child_exec_or_exit (int exec)
 	  inf->aspace = NULL;
 	  inf->pspace = NULL;

-	  if (debug_infrun || info_verbose)
+	  /* Print verbose/debug info message.  Hardcoded 1's designate
+	     that we are detaching a parent and that it is after a vfork,
+	     respectively.  */
+	  if (exec)
 	    {
-	      target_terminal_ours ();
-
-	      if (exec)
-		fprintf_filtered (gdb_stdlog,
-				  "Detaching vfork parent process "
-				  "%d after child exec.\n",
-				  inf->vfork_parent->pid);
-	      else
-		fprintf_filtered (gdb_stdlog,
-				  "Detaching vfork parent process "
-				  "%d after child exit.\n",
-				  inf->vfork_parent->pid);
+	      print_fork_detach (inf->vfork_parent->pid, 1, 1,
+				 TARGET_WAITKIND_EXECD);
+	    }
+	  else
+	    {
+	      print_fork_detach (inf->vfork_parent->pid, 1, 1,
+				 TARGET_WAITKIND_EXITED);
 	    }

 	  target_detach (NULL, 0);
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp
b/gdb/testsuite/gdb.base/foll-fork.exp
index ad8b750..06ba1b5 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -115,7 +115,11 @@  proc test_follow_fork { who detach cmd } {
 	# Set up the output we expect to see after we run.
 	set expected_re ""
 	if {$who == "child"} {
-	    set expected_re "Attaching after.* fork to.*set breakpoint here.*"
+	    set expected_re "Attaching after.* fork to.*"
+	    if {$detach == "on"} {
+	        append expected_re "Detaching after fork from .*"
+	    }
+	    append expected_re "set breakpoint here.*"
 	} elseif {$who == "parent" && $detach == "on"} {
 	    set expected_re "Detaching after fork from .*set breakpoint here.*"
 	} else {
@@ -218,9 +222,9 @@  proc catch_fork_child_follow {} {
 	"Temporary breakpoint.*, line $bp_after_fork.*" \
 	"set follow-fork child, tbreak"

-    gdb_test "continue" \
-	"Attaching after.* fork to.* at .*$bp_after_fork.*" \
-	"set follow-fork child, hit tbreak"
+    set expected_re "Attaching after.* fork to.*Detaching after fork from"
+    append expected_re " .* at .*$bp_after_fork.*"
+    gdb_test "continue" $expected_re "set follow-fork child, hit tbreak"

     # The parent has been detached; allow time for any output it might
     # generate to arrive, so that output doesn't get confused with
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp
b/gdb/testsuite/gdb.base/foll-vfork.exp
index fe3663c..27ecdbc 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -121,7 +121,7 @@  proc vfork_parent_follow_through_step {} {

    set test "step"
    gdb_test_multiple "next" $test {
-       -re "Detaching after fork from.*if \\(pid == 0\\).*$gdb_prompt " {
+       -re "Detaching after vfork from.*if \\(pid == 0\\).*$gdb_prompt " {
 	   pass $test
        }
    }
@@ -146,7 +146,7 @@  proc vfork_parent_follow_to_bp {} {

    set test "continue to bp"
    gdb_test_multiple "continue" $test {
-       -re ".*Detaching after fork from child
process.*Breakpoint.*${bp_location}.*$gdb_prompt " {
+       -re ".*Detaching after vfork from child
process.*Breakpoint.*${bp_location}.*$gdb_prompt " {
 	   pass $test