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

Yao Qi
2014-05-08T05:23:05Z

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?

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

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

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

--
Yao (é½å°§)