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

Pedro Alves
2014-10-15T16:08:56Z

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