From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3199 invoked by alias); 8 May 2014 05:23:05 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3184 invoked by uid 89); 8 May 2014 05:23:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 08 May 2014 05:23:02 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1WiGnK-0007W6-Dj from Yao_Qi@mentor.com ; Wed, 07 May 2014 22:22:58 -0700 Received: from SVR-ORW-FEM-06.mgc.mentorg.com ([147.34.97.120]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 7 May 2014 22:22:58 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by SVR-ORW-FEM-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server id 14.2.247.3; Wed, 7 May 2014 22:22:57 -0700 Message-ID: <536B1429.2050601@codesourcery.com> Date: Thu, 08 May 2014 05:23:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Don Breazeal , CC: Don Breazeal Subject: Re: [PATCH 1/4][REPOST] Remote Linux ptrace exit events References: <1398885482-8449-1-git-send-email-donb@codesourcery.com> <1398885482-8449-2-git-send-email-donb@codesourcery.com> In-Reply-To: <1398885482-8449-2-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00075.txt.bz2 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 (齐尧)