| Message ID | 4032488.W8nPzteMFC@ralph.baldwin.cx |
|---|---|
| State | New |
| Headers | show |
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
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;