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

[Pedro Alves <palves at redhat dot com>]

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  <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);
>>   }
> 
> 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