Re: [PATCH 1/4][REPOST] Remote Linux ptrace exit events

["Breazeal, Don" <donb@codesourcery.com>]

Hi Yao,
Thanks for the review.

On 5/7/2014 10:20 PM, Yao Qi wrote:
> Don,
> I am not the right person to review this patch series and approve them,
> but I can do a primary review.  Hope it helps to relieve feeling of the
> long time review :)
>
> On 05/01/2014 03:17 AM, Don Breazeal wrote:
>> This patch implements support for the extended ptrace event
>> PTRACE_EVENT_EXIT on Linux.  This is a preparatory patch for exec event
>> support.
>>
>> The use of this event is entirely internal to gdbserver; these events are
>> not reported to GDB or the user.  If an existing thread is not the last
>> thread in a process, its lwp entry is simply deleted, since this is what
>> happens in the absence of exit events.  If it is the last thread of a
>> process, the wait status is set to the actual wait status of the thread,
>> instead of the status that indicates the extended event, and the existing
>> mechanisms for handling thread exit proceed as usual.
>>
>> The only purpose in using the exit events instead of the existing wait
>> mechanisms is to ensure that the exit of a thread group leader is detected
>> reliably when a non-leader thread calls exec.
>
> Can you elaborate a little please?  why existing wait mechanism is not
> reliable?

I'm working on a write-up that I will include in the next version of the 
patch.  The short version is that there is a race condition in the 
current implementation that can leave a dangling entry in the lwp list 
(an entry that doesn't have a corresponding actual lwp).  In this case 
gdbserver will hang waiting for the non-existent lwp to stop.  Using the 
exit events eliminates this race condition.

>
>>   gdb/common/linux-ptrace.c |   42 ++++++++++++++++++-
>>   gdb/gdbserver/linux-low.c |   99 +++++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 125 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
>> index 7c1b78a..e3fc705 100644
>> --- a/gdb/common/linux-ptrace.c
>> +++ b/gdb/common/linux-ptrace.c
>> @@ -414,7 +414,7 @@ linux_test_for_tracefork (int child_pid)
>>     ret = ptrace (PTRACE_CONT, child_pid, (PTRACE_TYPE_ARG3) 0,
>>   		(PTRACE_TYPE_ARG4) 0);
>>     if (ret != 0)
>> -    warning (_("linux_test_for_tracefork: failed to resume child"));
>> +    warning (_("linux_test_for_tracefork: failed to resume child (a)"));
>>
>>     ret = my_waitpid (child_pid, &status, 0);
>>
>> @@ -455,10 +455,46 @@ linux_test_for_tracefork (int child_pid)
>>   		       "failed to kill second child"));
>>   	  my_waitpid (second_pid, &status, 0);
>>   	}
>> +      else
>> +	{
>> +	  /* Fork events are not supported.  */
>> +	  return;
>> +	}
>>       }
>>     else
>> -    warning (_("linux_test_for_tracefork: unexpected result from waitpid "
>> -	     "(%d, status 0x%x)"), ret, status);
>> +    {
>> +      warning (_("linux_test_for_tracefork: unexpected result from waitpid "
>> +	       "(%d, status 0x%x)"), ret, status);
>> +      return;
>> +    }
>> +
>> +#ifdef GDBSERVER
>> +  /* PTRACE_O_FORK is supported, so now test for PTRACE_O_TRACEEXIT.
>> +     First try to set the option.  If this fails, we know for sure that
>> +     it is not supported.  */
>> +  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
>> +		(PTRACE_TYPE_ARG4) PTRACE_O_TRACEEXIT);
>> +  if (ret != 0)
>> +    return;
>> +
>> +  /* We don't know for sure that the feature is available; old
>> +     versions of PTRACE_SETOPTIONS ignored unknown options.  So
>> +     see if the process exit will generate a PTRACE_EVENT_EXIT.  */
>> +  ret = ptrace (PTRACE_CONT, child_pid, (PTRACE_TYPE_ARG3) 0,
>> +		(PTRACE_TYPE_ARG4) 0);
>> +  if (ret != 0)
>> +    warning (_("linux_test_for_tracefork: failed to resume child (b)"));
>> +
>> +  ret = my_waitpid (child_pid, &status, 0);
>> +
>> +  /* Check if we received an exit event notification.  */
>> +  if (ret == child_pid && WIFSTOPPED (status)
>> +      && status >> 16 == PTRACE_EVENT_EXIT)
>> +    {
>> +        /* PTRACE_O_TRACEEXIT is supported.  */
>> +        current_ptrace_options |= PTRACE_O_TRACEEXIT;
>> +    }
>> +#endif
>>   }
>>
>
> The code looks clear to me, but it is strange to put them in function
> linux_test_for_tracefork.  IWBN to move them to a new function.

I had gone back and forth on this issue.  I will make a new function for 
this.

>
>>   /* Enable reporting of all currently supported ptrace events.  */
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 2d8d5f5..90e7b15 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -224,6 +224,7 @@ static void proceed_all_lwps (void);
>>   static int finish_step_over (struct lwp_info *lwp);
>>   static CORE_ADDR get_stop_pc (struct lwp_info *lwp);
>>   static int kill_lwp (unsigned long lwpid, int signo);
>> +static int num_lwps (int pid);
>>
>>   /* True if the low target can hardware single-step.  Such targets
>>      don't need a BREAKPOINT_REINSERT_ADDR callback.  */
>> @@ -367,13 +368,32 @@ linux_add_process (int pid, int attached)
>>     return proc;
>>   }
>>
>> +/* Check wait status for extended event */
>
> Period and one space is missing at the end of the comment.

I will clean up everything you mentioned from here to the bottom.
thanks
--Don

>
>> +
>> +static int
>> +is_extended_waitstatus (int wstat)
>> +{
>> +    return wstat >> 16 != 0;
>     ^^^^
> Two spaces instead of four.
>
>> +}
>> +
>> +/* Check wait status for extended event */
>
> Likewise.
>
>> +
>> +static int
>> +is_extended_exit (int wstat)
>> +{
>> +    return wstat >> 16 == PTRACE_EVENT_EXIT;
>     ^^^^
> Likewise.
>
>> +}
>> +
>>   /* Handle a GNU/Linux extended wait response.  If we see a clone
>>      event, we need to add the new LWP to our list (and not report the
>
> I know it is not about your change, but better to replace LWP with
> EVENT_CHILD.  We also need to update it for the exit event.
>
>> -   trap to higher layers).  */
>> +   trap to higher layers).  This function returns non-zero if the
>> +   event should be ignored and we should wait again.  The wait status
>> +   in WSTATP may be modified if an exit event occurred.  */
>>
>> -static void
>> -handle_extended_wait (struct lwp_info *event_child, int wstat)
>> +static int
>> +handle_extended_wait (struct lwp_info *event_child, int *wstatp)
>>   {
>> +  int wstat = *wstatp;
>>     int event = wstat >> 16;
>>     struct thread_info *event_thr = get_lwp_thread (event_child);
>>     struct lwp_info *new_lwp;
>> @@ -448,11 +468,54 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
>>   	    linux_resume_one_lwp (new_lwp, 0, WSTOPSIG (status), NULL);
>>   	}
>>
>> +      /* Enable extended events for the new thread.  */
>> +      linux_enable_event_reporting (new_pid);
>> +
>
> This change isn't mentioned in ChangeLog entry.  I also wonder why is it
> needed here.
>
>>         /* Always resume the current thread.  If we are stopping
>>   	 threads, it will have a pending SIGSTOP; we may as well
>>   	 collect it now.  */
>>         linux_resume_one_lwp (event_child, event_child->stepping, 0, NULL);
>> +
>> +      /* Don't report the event.  */
>> +      return 1;
>> +     }
>> +  else if (event == PTRACE_EVENT_EXIT)
>> +    {
>> +      unsigned long exit_status;
>> +      unsigned long lwpid = lwpid_of (event_thr);
>> +      int ret;
>> +
>> +      if (debug_threads)
>> +        debug_printf ("LHEW: Got exit event from LWP %ld\n",
>
> s/LHEW/HEW/
>
>> +                      lwpid_of (event_thr));
>> +
>> +      ptrace (PTRACE_GETEVENTMSG, lwpid_of (event_thr), (PTRACE_TYPE_ARG3) 0,
>> +	      &exit_status);
>> +
>> +      if (num_lwps (pid_of (event_thr)) > 1)
>> +        {
>> +	  /* If there is at least one more LWP, then the program still
>> +	     exists and the exit should not be reported to GDB.  */
>> +          delete_lwp (event_child);
>> +          ret = 1;
>> +        }
>> +      else
>> +        {
>> +          /* Set the exit status to the actual exit status, so normal
>> +             WIFEXITED/WIFSIGNALLED processing and reporting for the
>> +             last lwp in the process can proceed from here.  */
>> +          *wstatp = exit_status;
>> +          ret = 0;
>> +        }
>> +
>> +      /* Resume the thread so that it actually exits.  Subsequent exit
>> +         events for LWPs that were deleted above will be ignored.  */
>> +      ptrace (PTRACE_CONT, lwpid, (PTRACE_TYPE_ARG3) 0,
>> +              (PTRACE_TYPE_ARG4) 0);
>> +      return ret;
>>       }
>> +  internal_error (__FILE__, __LINE__,
>> +	          _("unknown ptrace event %d"), event);
>
> This should be mentioned in ChangeLog too.
>