[PATCH 1/3] Add fbsd_nat_add_target.

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] Add fbsd_nat_add_target.

Pedro Alves-7
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

Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] Add fbsd_nat_add_target.

Mark Kettenis
> From: John Baldwin <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
> > 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] Add fbsd_nat_add_target.

Mark Kettenis
> From: John Baldwin <[hidden email]>
> Date: Mon, 27 Apr 2015 16:17:52 -0400
>
> On Monday, April 27, 2015 09:54:24 PM Mark Kettenis wrote:
> > > From: John Baldwin <[hidden email]>
> > > 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/3] Add fbsd_nat_add_target.

Pedro Alves-7
On 04/27/2015 09:50 PM, Mark Kettenis wrote:
>> From: John Baldwin <[hidden email]>
>> 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