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

[Pedro Alves <palves at redhat dot com>]

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