diff mbox

Fix gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw) (timeout) with Linux 2.6.32 and older version

Message ID 533D17E2.9070402@mentor.com
State Superseded
Headers show

Commit Message

Hui Zhu April 3, 2014, 8:12 a.m. UTC
Got gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw)
(timeout) with Linux 2.6.32 and older version.

The rootcause is after the test use "set can-use-hw-watchpoints 0" let GDB
doesn't use hardware breakpoint and set a watchpoint on "global", GDB
continue will keep single step inside function "vfork".
The Linux 2.6.32 and older version doesn't have commit
6580807da14c423f0d0a708108e6df6ebc8bc83d (get more info please goto
http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=6580807da14c423f0d0a708108e6df6ebc8bc83d).
When the function "vfork" do syscall, the single step flag TIF_SINGLESTEP
will copy to child process.
Then GDB detach it, child process and parent process will be hanged.

So I make a patch that do a single step before detach.  Then TIF_SINGLESTEP
of child process in old Linux kernel will be cleared before detach.
Child process in new Linux kernel will not be affected by this single step.

The patch was tested and pass regression in new linux
kernel (3.13.6-200.fc20.x86_64) and old Linux kernel (2.6.32-38-server).

Please help me review it.

Thanks,
Hui

2014-04-03  Hui Zhu  <hui@codesourcery.com>

	* linux-nat.c (linux_child_follow_fork): do a single step before
	detach.

Comments

Pedro Alves May 28, 2014, 7:19 p.m. UTC | #1
On 04/03/2014 09:12 AM, Hui Zhu wrote:
> Got gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw)
> (timeout) with Linux 2.6.32 and older version.
> 
> The rootcause is after the test use "set can-use-hw-watchpoints 0" let GDB
> doesn't use hardware breakpoint and set a watchpoint on "global", GDB
> continue will keep single step inside function "vfork".
> The Linux 2.6.32 and older version doesn't have commit
> 6580807da14c423f0d0a708108e6df6ebc8bc83d (get more info please goto
> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=6580807da14c423f0d0a708108e6df6ebc8bc83d).
> When the function "vfork" do syscall, the single step flag TIF_SINGLESTEP
> will copy to child process.
> Then GDB detach it, child process and parent process will be hanged.
> 
> So I make a patch that do a single step before detach.  Then TIF_SINGLESTEP
> of child process in old Linux kernel will be cleared before detach.
> Child process in new Linux kernel will not be affected by this single step.
> 
> The patch was tested and pass regression in new linux
> kernel (3.13.6-200.fc20.x86_64) and old Linux kernel (2.6.32-38-server).
> 
> Please help me review it.

Thanks.

> 2014-04-03  Hui Zhu  <hui@codesourcery.com>
> 
> 	* linux-nat.c (linux_child_follow_fork): do a single step before
> 	detach.
> 
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -442,6 +442,26 @@ holding the child stopped.  Try \"set de
>   
>   	  if (linux_nat_prepare_to_resume != NULL)
>   	    linux_nat_prepare_to_resume (child_lp);
> +
> +	  /* When debug a inferior in the architecture that support
> +	     hardware single step and the Linux kernel without commit
> +	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
> +	     process will starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits
> +	     if the parent process has it.
> +	     So let child process do a single step under GDB control
> +	     before detach it to remove this flags.  */

From the kernel patch's looks, this doesn't sound like architecture
specific, otherwise I'd suggest clearing TF instead.

So it sounds like a good solution.

I suggested this updated comment, copy/edited a bit from yours:

	  /* When debugging an inferior in an architecture that supports
	     hardware single stepping on a kernel without commit
	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
	     process starts with the TIF_SINGLESTEP/X86_EFLAGS_TF bits
	     set if the parent process had them set.
	     To work around this, single step the child process
	     once before detaching to clear the flags.  */

> +
> +	  if (!gdbarch_software_single_step_p (target_thread_architecture
> +						   (child_lp->ptid)))
> +	    {
> +	      int status;
> +
> +	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
> +		perror_with_name (_("Couldn't do single step"));
> +	      if (my_waitpid (child_pid, &status, 0) < 0)
> +		perror_with_name (_("Couldn't wait vfork process"));

If the child gets a signal here, we should pass it on to the child.

> +	    }
> +
>   	  ptrace (PTRACE_DETACH, child_pid, 0, 0);

That is:

      ptrace (PTRACE_DETACH, child_pid, 0, WSTOPSIG (status));

And I think we should disable all ptrace options in the child
before stepping it, in case some event is reported right
at that point, and we mishandle it.  Otherwise we'd need to
make sure we didn't get an extended wait status before passing
it on.  But disabling events is just safer.

>   
>   	  do_cleanups (old_chain);
>
Hui Zhu June 4, 2014, 8:43 a.m. UTC | #2
On 05/29/14 03:19, Pedro Alves wrote:
> On 04/03/2014 09:12 AM, Hui Zhu wrote:
>> Got gdb.base/watch-vfork.exp: Watchpoint triggers after vfork (sw)
>> (timeout) with Linux 2.6.32 and older version.
>>
>> The rootcause is after the test use "set can-use-hw-watchpoints 0" let GDB
>> doesn't use hardware breakpoint and set a watchpoint on "global", GDB
>> continue will keep single step inside function "vfork".
>> The Linux 2.6.32 and older version doesn't have commit
>> 6580807da14c423f0d0a708108e6df6ebc8bc83d (get more info please goto
>> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=6580807da14c423f0d0a708108e6df6ebc8bc83d).
>> When the function "vfork" do syscall, the single step flag TIF_SINGLESTEP
>> will copy to child process.
>> Then GDB detach it, child process and parent process will be hanged.
>>
>> So I make a patch that do a single step before detach.  Then TIF_SINGLESTEP
>> of child process in old Linux kernel will be cleared before detach.
>> Child process in new Linux kernel will not be affected by this single step.
>>
>> The patch was tested and pass regression in new linux
>> kernel (3.13.6-200.fc20.x86_64) and old Linux kernel (2.6.32-38-server).
>>
>> Please help me review it.
>
> Thanks.
>
>> 2014-04-03  Hui Zhu  <hui@codesourcery.com>
>>
>> 	* linux-nat.c (linux_child_follow_fork): do a single step before
>> 	detach.
>>
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -442,6 +442,26 @@ holding the child stopped.  Try \"set de
>>
>>    	  if (linux_nat_prepare_to_resume != NULL)
>>    	    linux_nat_prepare_to_resume (child_lp);
>> +
>> +	  /* When debug a inferior in the architecture that support
>> +	     hardware single step and the Linux kernel without commit
>> +	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
>> +	     process will starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits
>> +	     if the parent process has it.
>> +	     So let child process do a single step under GDB control
>> +	     before detach it to remove this flags.  */
>
>  From the kernel patch's looks, this doesn't sound like architecture
> specific, otherwise I'd suggest clearing TF instead.
>
> So it sounds like a good solution.
>
> I suggested this updated comment, copy/edited a bit from yours:
>
> 	  /* When debugging an inferior in an architecture that supports
> 	     hardware single stepping on a kernel without commit
> 	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
> 	     process starts with the TIF_SINGLESTEP/X86_EFLAGS_TF bits
> 	     set if the parent process had them set.
> 	     To work around this, single step the child process
> 	     once before detaching to clear the flags.  */
>
>> +
>> +	  if (!gdbarch_software_single_step_p (target_thread_architecture
>> +						   (child_lp->ptid)))
>> +	    {
>> +	      int status;
>> +
>> +	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
>> +		perror_with_name (_("Couldn't do single step"));
>> +	      if (my_waitpid (child_pid, &status, 0) < 0)
>> +		perror_with_name (_("Couldn't wait vfork process"));
>
> If the child gets a signal here, we should pass it on to the child.
>
>> +	    }
>> +
>>    	  ptrace (PTRACE_DETACH, child_pid, 0, 0);
>
> That is:
>
>        ptrace (PTRACE_DETACH, child_pid, 0, WSTOPSIG (status));
>
> And I think we should disable all ptrace options in the child
> before stepping it, in case some event is reported right
> at that point, and we mishandle it.  Otherwise we'd need to
> make sure we didn't get an extended wait status before passing
> it on.  But disabling events is just safer.

Could you give me some help on this part?
I don't know how to disable all ptrace options.

Thanks,
Hui

>
>>
>>    	  do_cleanups (old_chain);
>>
>
Pedro Alves June 4, 2014, 4:11 p.m. UTC | #3
On 06/04/2014 09:43 AM, Hui Zhu wrote:
> On 05/29/14 03:19, Pedro Alves wrote:

>> And I think we should disable all ptrace options in the child
>> before stepping it, in case some event is reported right
>> at that point, and we mishandle it.  Otherwise we'd need to
>> make sure we didn't get an extended wait status before passing
>> it on.  But disabling events is just safer.
> 
> Could you give me some help on this part?
> I don't know how to disable all ptrace options.

There's a linux_enable_event_reporting function in common/linux-ptrace.c.
Add a linux_disable_event_reporting counterpart, and call that.
diff mbox

Patch

--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -442,6 +442,26 @@  holding the child stopped.  Try \"set de
  
  	  if (linux_nat_prepare_to_resume != NULL)
  	    linux_nat_prepare_to_resume (child_lp);
+
+	  /* When debug a inferior in the architecture that support
+	     hardware single step and the Linux kernel without commit
+	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
+	     process will starts with TIF_SINGLESTEP/X86_EFLAGS_TF bits
+	     if the parent process has it.
+	     So let child process do a single step under GDB control
+	     before detach it to remove this flags.  */
+
+	  if (!gdbarch_software_single_step_p (target_thread_architecture
+						   (child_lp->ptid)))
+	    {
+	      int status;
+
+	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
+		perror_with_name (_("Couldn't do single step"));
+	      if (my_waitpid (child_pid, &status, 0) < 0)
+		perror_with_name (_("Couldn't wait vfork process"));
+	    }
+
  	  ptrace (PTRACE_DETACH, child_pid, 0, 0);
  
  	  do_cleanups (old_chain);