From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26351 invoked by alias); 26 Sep 2014 19:52:13 -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 26339 invoked by uid 89); 26 Sep 2014 19:52:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS 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; Fri, 26 Sep 2014 19:52:11 +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 s8QJq6K3011829 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 26 Sep 2014 15:52:07 -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 s8QJq4PD004604; Fri, 26 Sep 2014 15:52:05 -0400 Message-ID: <5425C3E4.3060305@redhat.com> Date: Fri, 26 Sep 2014 19:52:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Don Breazeal , 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> In-Reply-To: <1408580964-27916-3-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-09/txt/msg00797.txt.bz2 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); } 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. Thanks, Pedro Alves