[02/16,v2] Refactor follow-fork message printing
Commit Message
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"
}
}
@@ -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);
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
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