Re: [PATCH 02/16 v2] Refactor follow-fork message printing

Pedro Alves
2014-09-26T19:52:13Z

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.

> +{
> + 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