Re: [PATCH 1/3] Add fbsd_nat_add_target.

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> From: John Baldwin <jhb@freebsd.org>
> Date: Mon, 27 Apr 2015 16:17:52 -0400
> 
> On Monday, April 27, 2015 09:54:24 PM Mark Kettenis wrote:
> > > From: John Baldwin <jhb@freebsd.org>
> > > 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