This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PING][PATCH] Fix for PR gdb/14808, vfork/exec inferior problem
- From: "Breazeal, Don" <donb at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Fri, 23 May 2014 11:47:39 -0700
- Subject: Re: [PING][PATCH] Fix for PR gdb/14808, vfork/exec inferior problem
- Authentication-results: sourceware.org; auth=none
- References: <1400186622-3321-1-git-send-email-donb at codesourcery dot com>
Ping!
On 5/15/2014 1:43 PM, Don Breazeal wrote:
> On 5/15/2014 12:06 AM, Yao Qi wrote:
>> 05/13/2014 07:34 AM, donb@codesourcery.com wrote:
>>> @@ -649,6 +649,7 @@ handle_vfork_child_exec_or_exit (int exec)
>>> struct cleanup *old_chain;
>>> struct program_space *pspace;
>>> struct address_space *aspace;
>>> + struct inferior *parent_inf;
>>>
>>
>> Local parent_inf is only used in the "if (exec)" block below, so better
>> to declare it there.
> OK, done.
>
> --snip--
>
>>> + where the parent called vfork. Now that the child has
>>> + called exec and we are detaching from the parent, the
>>> + parent inferior needs to have its own pspace and aspace
>>
>> parent inferior has its own pspace, but may not have its own aspace,
>> depending on gdbarch_has_shared_address_space.
> Fixed.
>
>>
>>> + so that changes in the child don't affect it. We have
>>> + to give the new spaces to the parent since we saved the
>>> + child's spaces as the current spaces above. Even though
>>> + we are detaching the parent, we want to keep the
>>> + corresponding entry in the inferiors list intact. */
>>> + parent_inf = current_inferior ();
>>> + parent_inf->aspace = new_address_space ();
>>
>> Rather than creating a new address space, use maybe_new_address_space, like
>>> + parent_inf->pspace = add_program_space (parent_inf->aspace);
>>
>> parent_inf->pspace
>> = add_program_space (maybe_new_address_space ());
>> parent_inf->aspace = parent_inf->pspace->aspace;
> Done.
>
>>
>>> + parent_inf->removable = inf->removable;
>>
>> Field removable of parent inferior should be unchanged, IMO.
> It turns out this assignment was a NOP. Removed.
>
>>
>>> + set_current_program_space (parent_inf->pspace);
>>> + clone_program_space (parent_inf->pspace, pspace);
>>
>> Do we need to unlink parent and child? I am not very sure.
>>
>> /* Break the bonds. */
>> inf->vfork_parent->vfork_child = NULL;
>>
> We don't need to do it here, since this happens in target_detach. Since parent_inf is the parent, it's fork_child pointer is set. The unlinking happens in inferior.c:exit_inferior_1, which is ultimately called via target_detach. The child's vfork_parent pointer is cleared by existing code near the end of handle_vfork_child_exec_or_exit.
>
> Here is the updated patch.
> --Don
>
> gdb/
> 2014-05-15 Don Breazeal <donb@codesourcery.com>
>
> * infrun.c (handle_vfork_child_exec_or_exit): For the case
> of a vfork where we follow the child and detach the parent,
> and the child execs, create a new pspace for the parent
> inferior.
>
> gdb/testsuite
> 2014-05-15 Don Breazeal <donb@codesourcery.com>
>
> * gdb.base/foll-vfork.exp (vfork_relations_in_info_inferiors):
> Test that after a vfork and child exec, the parent's exec file
> name has not been changed.
>
> ---
> gdb/infrun.c | 44 ++++++++++++++++++++++----------
> gdb/testsuite/gdb.base/foll-vfork.exp | 11 ++++++++
> 2 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index ab39b6e..20ab003 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -665,27 +665,42 @@ handle_vfork_child_exec_or_exit (int exec)
> else
> old_chain = save_current_space_and_thread ();
>
> - /* We're letting loose of the parent. */
> + /* Make the parent the current inferior for target_detach. */
> tp = any_live_thread_of_process (inf->vfork_parent->pid);
> switch_to_thread (tp->ptid);
>
> - /* We're about to detach from the parent, which implicitly
> - removes breakpoints from its address space. There's a
> - catch here: we want to reuse the spaces for the child,
> - but, parent/child are still sharing the pspace at this
> - point, although the exec in reality makes the kernel give
> - the child a fresh set of new pages. The problem here is
> - that the breakpoints module being unaware of this, would
> - likely chose the child process to write to the parent
> - address space. Swapping the child temporarily away from
> - the spaces has the desired effect. Yes, this is "sort
> - of" a hack. */
> -
> + /* The child inferior may be dead, so avoid giving the
> + breakpoints module the option to write through to it
> + by swapping the child temporarily away from the spaces
> + (cloning a program space resets breakpoints). */
> pspace = inf->pspace;
> aspace = inf->aspace;
> inf->aspace = NULL;
> inf->pspace = NULL;
>
> + if (exec)
> + {
> + struct inferior *parent_inf;
> +
> + /* The parent and child inferiors have been sharing
> + program and address space structures from the point
> + where the parent called vfork. Now that the child has
> + called exec and we are detaching from the parent, the
> + parent inferior needs to have its own pspace so that
> + changes in the child don't affect it. We have to give
> + the new pspace to the parent (instead of the child)
> + since we saved the child's spaces as the current spaces
> + above. Even though we are detaching the parent, we
> + want to keep the corresponding entry in the inferiors
> + list intact. */
> + parent_inf = current_inferior ();
> + parent_inf->pspace
> + = add_program_space (maybe_new_address_space ());
> + parent_inf->aspace = parent_inf->pspace->aspace;
> + set_current_program_space (parent_inf->pspace);
> + clone_program_space (parent_inf->pspace, pspace);
> + }
> +
> if (debug_infrun || info_verbose)
> {
> target_terminal_ours ();
> @@ -702,9 +717,10 @@ handle_vfork_child_exec_or_exit (int exec)
> inf->vfork_parent->pid);
> }
>
> + /* Detach the parent. */
> target_detach (NULL, 0);
>
> - /* Put it back. */
> + /* Put the child spaces back. */
> inf->pspace = pspace;
> inf->aspace = aspace;
>
> diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
> index fe3663c..e9b0110 100644
> --- a/gdb/testsuite/gdb.base/foll-vfork.exp
> +++ b/gdb/testsuite/gdb.base/foll-vfork.exp
> @@ -442,6 +442,17 @@ proc vfork_relations_in_info_inferiors { variant } {
> pass $test
> }
> }
> +
> + # Make sure the exec file name of the vfork parent is not
> + # changed when the child's is changed.
> + if { $variant == "exec" } {
> + set test "exec file name change"
> + gdb_test_multiple "info inferiors" $test {
> + -re " 2 .*vforked-prog.* 1 .*foll-vfork.*$gdb_prompt " {
> + pass $test
> + }
> + }
> + }
> }}
>
> proc do_vfork_and_follow_parent_tests {} {
>