[1/3] Add fbsd_nat_add_target.
Commit Message
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.
---
Comments
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
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.
> 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.
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).
> 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
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
@@ -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);
@@ -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);
+}
@@ -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 */
@@ -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);
@@ -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);
@@ -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;