From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31696 invoked by alias); 13 Nov 2014 18:53:17 -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 31684 invoked by uid 89); 13 Nov 2014 18:53:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,UNSUBSCRIBE_BODY autolearn=no 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, 13 Nov 2014 18:53:10 +0000 Received: from svr-orw-fem-06.mgc.mentorg.com ([147.34.97.120]) by relay1.mentorg.com with esmtp id 1XozVy-0005kp-AG from Don_Breazeal@mentor.com ; Thu, 13 Nov 2014 10:53:06 -0800 Received: from [172.30.1.21] (147.34.91.1) by svr-orw-fem-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server (TLS) id 14.3.181.6; Thu, 13 Nov 2014 10:53:05 -0800 Message-ID: <5464FE11.1080001@codesourcery.com> Date: Thu, 13 Nov 2014 18:53:00 -0000 From: "Breazeal, Don" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Pedro Alves , Subject: Re: [PATCH 06/16 v3] Extended-remote Linux follow fork References: <1408580964-27916-1-git-send-email-donb@codesourcery.com> <1414798134-11536-4-git-send-email-donb@codesourcery.com> <5464AB62.5040100@redhat.com> In-Reply-To: <5464AB62.5040100@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00277.txt.bz2 On 11/13/2014 5:00 AM, Pedro Alves wrote: > On 10/31/2014 11:28 PM, Don Breazeal wrote: >> This patch implements basic support for follow-fork and detach-on-fork on >> extended-remote Linux targets. Only 'fork' is supported in this patch; >> 'vfork' support is added n a subsequent patch. Sufficient extended-remote >> functionality has been implemented here to pass gdb.base/foll-fork.exp with >> the catchpoint tests commented out. >> > > Thanks. Thanks for the review. > > Let's try to shake out the higher-level concepts first before > focusing on specific details. > >> >> * implementing a new RSP 'T' Stop Reply Packet stop reason: "fork", in >> gdbserver/remote-utils.c and remote.c. > > "f" is an hexadecimal digit. Last time we added such a packet, > "core", we broke backwards compatibility with older GDBs... :-/ > We need to careful with that. Yes, I assume you saw the check in remote_parse_stop_reply next to the checks for "core" and "awatch". if (strncmp (p, "awatch", strlen("awatch")) != 0 - && strncmp (p, "core", strlen ("core") != 0)) + && strncmp (p, "core", strlen ("core") != 0) + && strncmp (p, "fork", strlen ("fork") != 0)) I ran into the same thing with "exec". Maybe these stop reason strings should all have a common prefix, like "stop_fork" and "stop_core". > >> >> * implementing new target and RSP support for target_follow_fork with >> target extended-remote. (The RSP components were actually defined in >> patch 4, but they see their first use here). >> >> - extended_remote target routine extended_remote_follow_fork >> >> - RSP packet vFollowFork > > Reading through this, I don't think this is the model we should be exposing > at the RSP level, and requiring servers to support. The hiding of the child > fork until the users resumes is a current detail that we may want to change in > the future. It seems better to me to _not_ hide the child from GDB, > and then implement the hide-child-until-resume detail in GDB. That is, > I think we should model fork events at the RSP level similarly to > how ptrace and ttrace expose them. So, e.g., I think switching to the > child to write to its memory should be done with the regular Hg packet. > Handling detach_fork would be done by GDB calling the regular > detach packet (D;PID), etc. I'm not even seeing a fundamental need > to keep this for "extended-remote" alone, given gdb nowadays supports > the multiprocess extensions with "target remote" too. My initial approach was to do just that, but I ended up with linux-specific code in remote.c (the code that lives in linux-nat.c for the native implementation). I guess the direction of recent changes would be to put that code into a common file in gdb/nat, if possible. Would that be the approach you would recommend? > > Also, I don't see how this packet could work correctly with non-stop > mode. You're assuming only one thread/process has stopped for a fork. > >> else >> { >> /* Silently skip unknown optional info. */ >> @@ -9418,8 +9447,11 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid) >> if (ptid_equal (magic_null_ptid, ptid)) >> xsnprintf (buf, sizeof buf, "Thread
"); >> else if (rs->extended && remote_multi_process_p (rs)) >> - xsnprintf (buf, sizeof buf, "Thread %d.%ld", >> - ptid_get_pid (ptid), ptid_get_lwp (ptid)); >> + if (ptid_get_lwp (ptid) == 0) >> + return normal_pid_to_str (ptid); > > Can you explain this bit? > > Thanks, > Pedro Alves > To goal was to make the messages printed in the remote case match the messages printed in the native case by printing "Process" ID info instead of "Thread" ID info given a process-style ptid (pid, 0, 0). It was not entirely successful, but if I recall correctly it was sufficient for matching test results. BTW, sorry for the extra noise on the list with my duplicated response to your comments on patch #4. I have had some mail server issues today, and mistakenly thought the first copy had not gone out. The changes requested here will probably take me a little while to implement and test. Thanks --Don