From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19346 invoked by alias); 26 Sep 2014 20:14:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 19335 invoked by uid 89); 26 Sep 2014 20:14:42 -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, 26 Sep 2014 20:14:41 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1XXbuX-0004MH-HY from Don_Breazeal@mentor.com ; Fri, 26 Sep 2014 13:14:37 -0700 Received: from [127.0.0.1] ([172.30.12.10]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 26 Sep 2014 13:14:37 -0700 Message-ID: <5425C92B.1010101@codesourcery.com> Date: Fri, 26 Sep 2014 20:14:00 -0000 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> In-Reply-To: <5425C3E4.3060305@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00798.txt.bz2 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. > >> +{ >> + if (info_verbose || debug_infrun) >> + { >> + target_terminal_ours (); > > We should really be using target_terminal_ours_for_output for > output instead. > >> + 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"' > > >> + } >> +} >> + >> +/* 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. > >> + 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. > >> + } >> + 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? > > 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. Thanks for the review, --Don > > Thanks, > Pedro Alves >