Patchwork Fix for gdb/PR 14808, vfork/exec inferior problem

login
register
mail settings
Submitter Don Breazeal
Date May 27, 2014, 5:01 p.m.
Message ID <5384C4D6.8090608@codesourcery.com>
Download mbox | patch
Permalink /patch/1172/
State New
Headers show

Comments

Don Breazeal - May 27, 2014, 5:01 p.m.
On 5/23/2014 5:57 PM, Yao Qi wrote:
> A nit on changelog entry:
> 
> On 05/16/2014 04:43 AM, Don Breazeal wrote:
>> gdb/
>> 2014-05-15  Don Breazeal  <donb@codesourcery.com>
>>
> 	PR gdb/14808
>> 	* 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.
> 
> With this, the magic script can connect this commit to the right PR.
> 
> The patch looks good to me, but I can't approve it.
> 
Thanks for checking this.  An updated ChangeLog and (unchanged) patch
follow.
--Don

gdb/
2014-05-12  Don Breazeal  <donb@codesourcery.com>

	PR gdb/14808
	* 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 and aspace for the
	parent inferior.

gdb/testsuite
2014-05-12  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                          |   42
++++++++++++++++++++++-----------
 gdb/testsuite/gdb.base/foll-vfork.exp |   11 ++++++++
 2 files changed, 39 insertions(+), 14 deletions(-)
Don Breazeal - June 4, 2014, 6:35 p.m.
Ping.

On 5/27/2014 10:01 AM, Breazeal, Don wrote:
> On 5/23/2014 5:57 PM, Yao Qi wrote:
>> A nit on changelog entry:
>>
>> On 05/16/2014 04:43 AM, Don Breazeal wrote:
>>> gdb/
>>> 2014-05-15  Don Breazeal  <donb@codesourcery.com>
>>>
>> 	PR gdb/14808
>>> 	* 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.
>>
>> With this, the magic script can connect this commit to the right PR.
>>
>> The patch looks good to me, but I can't approve it.
>>
> Thanks for checking this.  An updated ChangeLog and (unchanged) patch
> follow.
> --Don
> 
> gdb/
> 2014-05-12  Don Breazeal  <donb@codesourcery.com>
> 
> 	PR gdb/14808
> 	* 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 and aspace for the
> 	parent inferior.
> 
> gdb/testsuite
> 2014-05-12  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                          |   42
> ++++++++++++++++++++++-----------
>  gdb/testsuite/gdb.base/foll-vfork.exp |   11 ++++++++
>  2 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index ab39b6e..82a67d5 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -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;
> 
>  	  /* follow-fork child, detach-on-fork on.  */
> 
> @@ -665,27 +666,39 @@ 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 INF 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)
> +	    {
> +	      /* 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 and aspace
> +		 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 ();
> +	      parent_inf->pspace = add_program_space (parent_inf->aspace);
> +	      parent_inf->removable = inf->removable;
> +	      set_current_program_space (parent_inf->pspace);
> +	      clone_program_space (parent_inf->pspace, pspace);
> +	    }
> +
>  	  if (debug_infrun || info_verbose)
>  	    {
>  	      target_terminal_ours ();
> @@ -702,9 +715,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 {} {
>
Don Breazeal - June 19, 2014, 3:50 p.m.
Ping.
thanks
--Don

On 6/4/2014 11:35 AM, Breazeal, Don wrote:
> Ping.
> 
> On 5/27/2014 10:01 AM, Breazeal, Don wrote:
>> On 5/23/2014 5:57 PM, Yao Qi wrote:
>>> A nit on changelog entry:
>>>
>>> On 05/16/2014 04:43 AM, Don Breazeal wrote:
>>>> gdb/
>>>> 2014-05-15  Don Breazeal  <donb@codesourcery.com>
>>>>
>>> 	PR gdb/14808
>>>> 	* 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.
>>>
>>> With this, the magic script can connect this commit to the right PR.
>>>
>>> The patch looks good to me, but I can't approve it.
>>>
>> Thanks for checking this.  An updated ChangeLog and (unchanged) patch
>> follow.
>> --Don
>>
>> gdb/
>> 2014-05-12  Don Breazeal  <donb@codesourcery.com>
>>
>> 	PR gdb/14808
>> 	* 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 and aspace for the
>> 	parent inferior.
>>
>> gdb/testsuite
>> 2014-05-12  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                          |   42
>> ++++++++++++++++++++++-----------
>>  gdb/testsuite/gdb.base/foll-vfork.exp |   11 ++++++++
>>  2 files changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index ab39b6e..82a67d5 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -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;
>>
>>  	  /* follow-fork child, detach-on-fork on.  */
>>
>> @@ -665,27 +666,39 @@ 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 INF 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)
>> +	    {
>> +	      /* 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 and aspace
>> +		 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 ();
>> +	      parent_inf->pspace = add_program_space (parent_inf->aspace);
>> +	      parent_inf->removable = inf->removable;
>> +	      set_current_program_space (parent_inf->pspace);
>> +	      clone_program_space (parent_inf->pspace, pspace);
>> +	    }
>> +
>>  	  if (debug_infrun || info_verbose)
>>  	    {
>>  	      target_terminal_ours ();
>> @@ -702,9 +715,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 {} {
>>
>

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index ab39b6e..82a67d5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -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;

 	  /* follow-fork child, detach-on-fork on.  */

@@ -665,27 +666,39 @@  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 INF 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)
+	    {
+	      /* 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 and aspace
+		 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 ();
+	      parent_inf->pspace = add_program_space (parent_inf->aspace);
+	      parent_inf->removable = inf->removable;
+	      set_current_program_space (parent_inf->pspace);
+	      clone_program_space (parent_inf->pspace, pspace);
+	    }
+
 	  if (debug_infrun || info_verbose)
 	    {
 	      target_terminal_ours ();
@@ -702,9 +715,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 {} {