Re: [PING][PATCH] Fix for PR gdb/14808, vfork/exec inferior problem

["Breazeal, Don" <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 {} {
>