From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16194 invoked by alias); 15 Oct 2014 16:08:56 -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 16178 invoked by uid 89); 15 Oct 2014 16:08:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 15 Oct 2014 16:08:54 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9FG8oeM013125 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 15 Oct 2014 12:08:51 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9FG8mrC021927; Wed, 15 Oct 2014 12:08:48 -0400 Message-ID: <543E9C0F.9030701@redhat.com> Date: Wed, 15 Oct 2014 16:08:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: "Breazeal, Don" , 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> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-10/txt/msg00402.txt.bz2 On 09/26/2014 09: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. >> >>> +{ >>> + 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? Sorry, I saw wrongness in more places, and then picked the worst example. Still, there's no reason to assume that: 1 - the best translation would put the child/parent word in the same position. 2 - the translation for "parent" and "child" would be the same in all occurrences. But a worse example is here: + fprintf_filtered (gdb_stdlog, + "Detaching vfork parent process %d after" + " child %s.\n", pid, vfork_action); vfork_action is either "exec" or "exit". Here, when I try to figure out how I'd translate this to Portuguese, I'd probably want to translate the two sentences differently: I'd probably want to translate "child exit" naturally (maybe "depois de o processo filho terminar"), while "child exec" can't be translated so directly: it'd probably need an auxiliary verb, something like "depois do o processo filho chamar exec". Where ".. chamar exec" means "... _called/calling_ exec". > 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. Sorry, I still don't think you're new patch (sent as follow up) is an improvement... Having to explain the "Hardcoded 1's" in a comment is a red sign to me. :-/ Could you do a patch that just adds the missing output, and fixes fork/vfork without moving the printing code to a separate function? For the fork vs vfork issue, doing ' is_vfork ? "vfork" : "fork" ' is fine. Thanks, Pedro Alves