From patchwork Fri Oct 3 23:51:46 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Don Breazeal X-Patchwork-Id: 3101 Received: (qmail 13993 invoked by alias); 3 Oct 2014 23:51:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 13983 invoked by uid 89); 3 Oct 2014 23:51:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 03 Oct 2014 23:51:51 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1XaCdX-00014Z-Li from Don_Breazeal@mentor.com ; Fri, 03 Oct 2014 16:51:47 -0700 Received: from [127.0.0.1] ([172.30.12.49]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 3 Oct 2014 16:51:47 -0700 Message-ID: <542F3692.6090402@codesourcery.com> Date: Fri, 03 Oct 2014 16:51:46 -0700 From: "Breazeal, Don" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 02/16 v2] Refactor follow-fork message printing References: <1407434395-19089-1-git-send-email-donb@codesourcery.com> <1408580964-27916-3-git-send-email-donb@codesourcery.com> <5425C3E4.3060305@redhat.com> <5425C92B.1010101@codesourcery.com> In-Reply-To: <5425C92B.1010101@codesourcery.com> X-IsSubscribed: yes 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 >>> >>> * 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 >>> >>> * 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 * 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 * 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 --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