From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12117 invoked by alias); 9 May 2014 21:03:42 -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 12106 invoked by uid 89); 9 May 2014 21:03:42 -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; Fri, 09 May 2014 21:03:40 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1WirxA-0005Pr-Tr from donb@codesourcery.com ; Fri, 09 May 2014 14:03:36 -0700 Received: from [127.0.0.1] ([172.30.1.203]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 9 May 2014 14:03:36 -0700 Message-ID: <536D42A7.3030102@codesourcery.com> Date: Fri, 09 May 2014 21:03:00 -0000 From: "Breazeal, Don" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Yao Qi , gdb-patches@sourceware.org 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> <536B1429.2050601@codesourcery.com> In-Reply-To: <536B1429.2050601@codesourcery.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00119.txt.bz2 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. >