From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30902 invoked by alias); 27 Apr 2015 20:50:26 -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 30888 invoked by uid 89); 27 Apr 2015 20:50:25 -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,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: glazunov.sibelius.xs4all.nl Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 27 Apr 2015 20:50:24 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id t3RKoC4E022408; Mon, 27 Apr 2015 22:50:12 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id t3RKoCSV018211; Mon, 27 Apr 2015 22:50:12 +0200 (CEST) Date: Tue, 28 Apr 2015 01:06:00 -0000 Message-Id: <201504272050.t3RKoCSV018211@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: jhb@freebsd.org CC: gdb-patches@sourceware.org, palves@redhat.com In-reply-to: <4869684.VuRe0HgzJ9@ralph.baldwin.cx> (message from John Baldwin on Mon, 27 Apr 2015 16:17:52 -0400) Subject: Re: [PATCH 1/3] Add fbsd_nat_add_target. References: <4032488.W8nPzteMFC@ralph.baldwin.cx> <2013405.YhOVhnvfYq@ralph.baldwin.cx> <201504271954.t3RJsOpO013326@glazunov.sibelius.xs4all.nl> <4869684.VuRe0HgzJ9@ralph.baldwin.cx> X-SW-Source: 2015-04/txt/msg01022.txt.bz2 > From: John Baldwin > Date: Mon, 27 Apr 2015 16:17:52 -0400 > > On Monday, April 27, 2015 09:54:24 PM Mark Kettenis wrote: > > > From: John Baldwin > > > Date: Mon, 27 Apr 2015 15:16:50 -0400 > > > > > > On Monday, April 27, 2015 08:10:18 PM Pedro Alves wrote: > > > > On 04/26/2015 02:24 AM, John Baldwin wrote: > > > > > Add a wrapper for add_target in fbsd-nat.c to override target operations > > > > > common to all native FreeBSD targets. > > > > > > > > > > gdb/ChangeLog: > > > > > > > > > > * fbsd-nat.c (fbsd_pid_to_exec_file): Mark static. > > > > > (fbsd_find_memory_regions): Mark static. > > > > > (fbsd_nat_add_target): New function. > > > > > * fbsd-nat.h: Export fbsd_nat_add_target and remove prototypes for > > > > > fbsd_pid_to_exec_file and fbsd_find_memory_regions. > > > > > * amd64fbsd-nat.c (_initialize_amd64fbsd_nat): Use fbsd_nat_add_target. > > > > > * i386fbsd-nat.c (_initialize_i386fbsd_nat): Likewise. > > > > > * ppcfbsd-nat.c (_initialize_ppcfbsd_nat): Likewise. > > > > > * sparc64fbsd-nat.c (_initialize_sparc64fbsd_nat): Likewise. > > > > > > > > OOC, any reason you didn't instead do it like: > > > > > > > > struct target_ops * > > > > fbsd_nat_target (void) > > > > { > > > > struct target_ops *t = inf_ptrace_target (); > > > > > > > > t->to_pid_to_exec_file = fbsd_pid_to_exec_file; > > > > t->to_find_memory_regions = fbsd_find_memory_regions; > > > > return t; > > > > } > > > > > > > > and then use fbsd_nat_target instead of inf_ptrace_target > > > > directly? > > > > > > > > This maps a little better to a C++ world. > > > > > > > > linux-nat.c does it the way you did as it keeps a separate > > > > linux_ops target instance around. > > > > > > I was probably just using linux-nat.c as a reference. One thing that > > > confuses me about the linux-nat target is that it keeps linux_ops > > > around so that it can call the original methods that it overrides, > > > and yet for a few methods it also uses a local 'super_foo' variable > > > to call an original method. I think that those are both doing the > > > same thing, but perhaps there is some subtlety I'm missing? > > > > > > I do use a 'super_wait' to call ptrace's wait method in the second > > > patch in this series, so I could certainly change this to return a > > > target rather than modifying an existing one if that is preferred. > > > > I'd say the linux-nat.c code is a bad example and recommend looking at > > the obsd-nat.c code instead. The linux-nat.c code is so complicated > > because of all the workarounds needed to support threads. The > > linux_ops stuff is pretty much an artifact of those workarounds. > > > > I found that to add threads-support I did need to make modifications > > to the _wait function that made it hard to re-use the > > inf_ptrace_wait() code. Sometimes code duplications just is the right > > answer. > > FWIW, obsd-nat.c (which I have looked at a bit), also uses a wrapper > around add_target rather than creating a generic OpenBSD native target. > To change the FreeBSD native targets I think would be an invasive change > since they use platform-specific native targets that are pan-BSD as their > initial target (e.g. amd64bsd_target) and customize from there. To make > fbsd_nat_target work I would need to rework things like amd64bsd_target > to modify an existing target instead of returning a new one I think (which > would also mean changing all the other BSD native targets). Which is why I'm perfectly happy with your current 1/3 diff ;). And I don't think your super_wait approach is a problem either. Just that I think that ultimately you'll end up with duplucating the core of inf_ptrace_wait() in fbsd_wait(). I'm not really familliar enough with the implementation of the fork following stuff in the FreeBSD kernel. Looks significantly more complicated to how this was done in HP-UX (which is the approach I used for OpenBSD). But then it seems more complete. As far as I'm concerned you're the expert here and to me the series looks reasonable as posted. Cheers, Mark