[PATCH 1/3] Add fbsd_nat_add_target.

[PATCH 1/3] Add fbsd_nat_add_target.

[John Baldwin]

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.
---
diff --git a/gdb/amd64fbsd-nat.c b/gdb/amd64fbsd-nat.c
index a721f48..4745b44 100644
--- a/gdb/amd64fbsd-nat.c
+++ b/gdb/amd64fbsd-nat.c
@@ -227,9 +227,7 @@ _initialize_amd64fbsd_nat (void)
   t->to_mourn_inferior = amd64fbsd_mourn_inferior;
   t->to_read_description = amd64fbsd_read_description;
 
-  t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
-  t->to_find_memory_regions = fbsd_find_memory_regions;
-  add_target (t);
+  fbsd_nat_add_target (t);
 
   /* Support debugging kernel virtual memory images.  */
   bsd_kvm_add_target (amd64fbsd_supply_pcb);
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 1ce197d..68b8e65 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -37,7 +37,7 @@
 /* Return the name of a file that can be opened to get the symbols for
    the child process identified by PID.  */
 
-char *
+static char *
 fbsd_pid_to_exec_file (struct target_ops *self, int pid)
 {
   ssize_t len = PATH_MAX;
@@ -71,7 +71,7 @@ fbsd_pid_to_exec_file (struct target_ops *self, int pid)
    calling FUNC for each memory region.  OBFD is passed as the last
    argument to FUNC.  */
 
-int
+static int
 fbsd_find_memory_regions (struct target_ops *self,
 			  find_memory_region_ftype func, void *obfd)
 {
@@ -149,7 +149,7 @@ fbsd_read_mapping (FILE *mapfile, unsigned long *start, unsigned long *end,
    calling FUNC for each memory region.  OBFD is passed as the last
    argument to FUNC.  */
 
-int
+static int
 fbsd_find_memory_regions (struct target_ops *self,
 			  find_memory_region_ftype func, void *obfd)
 {
@@ -200,3 +200,11 @@ fbsd_find_memory_regions (struct target_ops *self,
   return 0;
 }
 #endif
+
+void
+fbsd_nat_add_target (struct target_ops *t)
+{
+  t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
+  t->to_find_memory_regions = fbsd_find_memory_regions;
+  add_target (t);
+}
diff --git a/gdb/fbsd-nat.h b/gdb/fbsd-nat.h
index e6e88ff..03f6bb1 100644
--- a/gdb/fbsd-nat.h
+++ b/gdb/fbsd-nat.h
@@ -20,16 +20,8 @@
 #ifndef FBSD_NAT_H
 #define FBSD_NAT_H
 
-/* Return the name of a file that can be opened to get the symbols for
-   the child process identified by PID.  */
-
-extern char *fbsd_pid_to_exec_file (struct target_ops *self, int pid);
-
-/* Iterate over all the memory regions in the current inferior,
-   calling FUNC for each memory region.  OBFD is passed as the last
-   argument to FUNC.  */
-
-extern int fbsd_find_memory_regions (struct target_ops *self,
-				     find_memory_region_ftype func, void *obfd);
+/* Register the customized FreeBSD target.  This should be used
+   instead of calling add_target directly.  */
+extern void fbsd_nat_add_target (struct target_ops *);
 
 #endif /* fbsd-nat.h */
diff --git a/gdb/i386fbsd-nat.c b/gdb/i386fbsd-nat.c
index 6c43f2c..f5d2ee3 100644
--- a/gdb/i386fbsd-nat.c
+++ b/gdb/i386fbsd-nat.c
@@ -176,9 +176,7 @@ _initialize_i386fbsd_nat (void)
 #endif
 
   t->to_resume = i386fbsd_resume;
-  t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
-  t->to_find_memory_regions = fbsd_find_memory_regions;
-  add_target (t);
+  fbsd_nat_add_target (t);
 
   /* Support debugging kernel virtual memory images.  */
   bsd_kvm_add_target (i386fbsd_supply_pcb);
diff --git a/gdb/ppcfbsd-nat.c b/gdb/ppcfbsd-nat.c
index 778b4bb..778e19a 100644
--- a/gdb/ppcfbsd-nat.c
+++ b/gdb/ppcfbsd-nat.c
@@ -212,9 +212,7 @@ _initialize_ppcfbsd_nat (void)
   t = inf_ptrace_target ();
   t->to_fetch_registers = ppcfbsd_fetch_inferior_registers;
   t->to_store_registers = ppcfbsd_store_inferior_registers;
-  t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
-  t->to_find_memory_regions = fbsd_find_memory_regions;
-  add_target (t);
+  fbsd_nat_add_target (t);
 
   /* Support debugging kernel virtual memory images.  */
   bsd_kvm_add_target (ppcfbsd_supply_pcb);
diff --git a/gdb/sparc64fbsd-nat.c b/gdb/sparc64fbsd-nat.c
index 1a2397f..f197f74 100644
--- a/gdb/sparc64fbsd-nat.c
+++ b/gdb/sparc64fbsd-nat.c
@@ -70,9 +70,7 @@ _initialize_sparc64fbsd_nat (void)
 
   /* Add some extra features to the generic SPARC target.  */
   t = sparc_target ();
-  t->to_pid_to_exec_file = fbsd_pid_to_exec_file;
-  t->to_find_memory_regions = fbsd_find_memory_regions;
-  add_target (t);
+  fbsd_nat_add_target (t);
 
   sparc_gregmap = &sparc64fbsd_gregmap;
 
-- 
2.2.1

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[Pedro Alves]

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.

Thanks,
Pedro Alves

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[John Baldwin]

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.

-- 
John Baldwin

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[Mark Kettenis]

> 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.

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[John Baldwin]

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).

-- 
John Baldwin

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[Mark Kettenis]

> 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

Re: [PATCH 1/3] Add fbsd_nat_add_target.

[Pedro Alves]

On 04/27/2015 09:50 PM, Mark Kettenis wrote:
>> From: John Baldwin <jhb@freebsd.org>
>> Date: Mon, 27 Apr 2015 16:17:52 -0400

>> 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 ;).

I'm happy with it too.  I was just curious.

Sounds like we'll need to revisit this if/when we make target_ops a
proper class hierarchy, but we'll cross that bridge when we come to it.

> As far as I'm concerned you're the expert here and to me the series
> looks reasonable as posted.

Agreed.

Thanks,
Pedro Alves