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

[Hui Zhu <hui_zhu@mentor.com>]

Hi Pedro,

Thanks for your help.

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.  */
>

Updated.

>> +
>> +	  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));
>

Fixed.

> 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.
>

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


Added a new function linux_disable_event_reporting and call it in the 
part before let child do single step.

>>
>>    	  do_cleanups (old_chain);
>>
>

This is the new patch for the issue.  Please help me review it.

Best,
Hui

2014-06-05  Hui Zhu  <hui@codesourcery.com>

	* common/linux-ptrace.c (linux_disable_event_reporting): New.
	* common/linux-ptrace.h (linux_disable_event_reporting): New extern.
	* linux-nat.c (linux_child_follow_fork): do a single step before
	detach

--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -476,6 +476,15 @@ linux_enable_event_reporting (pid_t pid)
  	  (PTRACE_TYPE_ARG4) (uintptr_t) current_ptrace_options);
  }

+/* Disable reporting of all currently supported ptrace events.  */
+
+void
+linux_disable_event_reporting (pid_t pid)
+{
+  /* Set the options.  */
+  ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0, 0);
+}
+
  /* Returns non-zero if PTRACE_OPTIONS is contained within
     CURRENT_PTRACE_OPTIONS, therefore supported.  Returns 0
     otherwise.  */
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -86,6 +86,7 @@ struct buffer;
  extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer 
*buffer);
  extern void linux_ptrace_init_warnings (void);
  extern void linux_enable_event_reporting (pid_t pid);
+extern void linux_disable_event_reporting (pid_t pid);
  extern int linux_supports_tracefork (void);
  extern int linux_supports_traceclone (void);
  extern int linux_supports_tracevforkdone (void);
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -414,6 +414,7 @@ holding the child stopped.  Try \"set de
        if (detach_fork)
  	{
  	  struct cleanup *old_chain;
+	  int status = 0;

  	  /* Before detaching from the child, remove all breakpoints
  	     from it.  If we forked, then this has already been taken
@@ -447,7 +448,28 @@ holding the child stopped.  Try \"set de

  	  if (linux_nat_prepare_to_resume != NULL)
  	    linux_nat_prepare_to_resume (child_lp);
-	  ptrace (PTRACE_DETACH, child_pid, 0, 0);
+
+	  /* 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;
+
+	      linux_disable_event_reporting (child_pid);
+	      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, WSTOPSIG (status));

  	  do_cleanups (old_chain);
  	}