Re: [RFC] Support command "catch syscall" properly on different targets

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, February 27 2015, Yao Qi wrote:

> Nowadays, "catch syscall" is supported on linux-nat target of
> different gdbarch and inf-ttrace target.  However, in
> breakpoint.c:catch_syscall_command_1, we have this check
>
>    /* Checking if the feature if supported.  */
>    if (gdbarch_get_syscall_number_p (gdbarch) == 0)
>      error (_("The feature 'catch syscall' is not supported on \
> this architecture yet."));
>
> On one hand, gdbarch method get_syscall_number isn't installed on any
> HP-UX targets.  That means users will get such error message even
> syscall catchpoint is supported on HP-UX.

You mean it's possible to use "catch syscall" on HP-UX targets?  I
wonder how it works.

>  On the other hand, on linux
> remote target (with GDBserver), "catch syscall" isn't supported
> (PR 13585), but no error is emitted:
>
>  (gdb) target remote :1234
>  Remote debugging using :1234
>  Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
>  0x00007ffff7ddb2d0 in ?? () from /lib64/ld-linux-x86-64.so.2
>  (gdb) catch syscall close
>  Catchpoint 1 (syscall 'close' [3])

Yes, this is wrong.

> The fix in this patch is to add a new target method
> supports_syscall_catchpoint, so that we can have different
> implementations on different targets.  On inf-ttrace, we
> can simply return one, while on linux-child,
> gdbarch_get_syscall_number_p is called.
>
> With this patch applied, on linux remote target, it becomes:
>
>  (gdb) target remote :1234
>  Remote debugging using :1234
>  Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
>  0x00007ffff7ddb2d0 in ?? () from /lib64/ld-linux-x86-64.so.2
>  (gdb) catch syscall close
>  The feature 'catch syscall' is not supported on this target yet.
>
> which looks more reasonable to me.  However, this patch causes some
> regressions in catch-syscall.exp,
>
>  catch syscall nonsense_syscall^M
>  The feature 'catch syscall' is not supported on this target yet.^M
>  (gdb) FAIL: gdb.base/catch-syscall.exp: catch syscall to a nonsense syscall is prohibited
>
> because syscall catchpoint isn't supported on exec target.
> I can move these tests to the place where inferior is created, before
> I go too far, I'd like to hear what do you think of this.

I don't think not being able to specify the syscall catchpoint before
starting the inferior is a good thing.

I mean, catch syscall relies on ptrace, just like catch fork or catch
exec.  You can do a catch fork before starting the inferior, so I think
it's natural that you do a catch syscall in this scenario too (not to
mention that sometimes you may want to catch syscalls before main).

What happens if, instead of error'ing out, you emit a warning saying
that GDB does not know if the target supports syscall catchpoints, and
that it will try to use the catchpoint, but if the target doesn't
support it, then the catchpoint will not work (sorry, something like
that, but obviously not this phrase).

Other than that, the patch looks straightforward.

Thanks,

> gdb:
>
> 2015-02-27  Yao Qi  <yao.qi@linaro.org>
>
> 	* breakpoint.c (catch_syscall_command_1): Call
> 	target_supports_syscall_catchpoint instead of
> 	gdbarch_get_syscall_number_p.
> 	* inf-ttrace.c (inf_ttrace_supports_syscall_catchpoint): New
> 	function.
> 	(inf_ttrace_target): Install field to_supports_syscall_catchpoint.
> 	* linux-nat.c (linux_child_supports_syscall_catchpoint): New function.
> 	(linux_target_install_ops): Install field
> 	to_supports_syscall_catchpoint.
> 	* target-delegates.c: Regenerated.
> 	* target.h (struct target_ops) <to_supports_syscall_catchpoint>:
> 	New field.
> 	(target_supports_syscall_catchpoint): New macro.
> ---
>  gdb/breakpoint.c       |  4 ++--
>  gdb/inf-ttrace.c       | 10 ++++++++++
>  gdb/linux-nat.c        | 10 ++++++++++
>  gdb/target-delegates.c | 33 +++++++++++++++++++++++++++++++++
>  gdb/target.h           |  9 +++++++++
>  5 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index db4b872..8e0ffb8 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -12135,9 +12135,9 @@ catch_syscall_command_1 (char *arg, int from_tty,
>    struct gdbarch *gdbarch = get_current_arch ();
>  
>    /* Checking if the feature if supported.  */
> -  if (gdbarch_get_syscall_number_p (gdbarch) == 0)
> +  if (!target_supports_syscall_catchpoint (gdbarch))
>      error (_("The feature 'catch syscall' is not supported on \
> -this architecture yet."));
> +this target yet."));
>  
>    tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
>  
> diff --git a/gdb/inf-ttrace.c b/gdb/inf-ttrace.c
> index 080e167..9c8c561 100644
> --- a/gdb/inf-ttrace.c
> +++ b/gdb/inf-ttrace.c
> @@ -1180,6 +1180,15 @@ inf_ttrace_get_ada_task_ptid (struct target_ops *self, long lwp, long thread)
>    return ptid_build (ptid_get_pid (inferior_ptid), lwp, 0);
>  }
>  
> +/* Implement the supports_syscall_catchpoint target_ops method.  */
> +
> +static int
> +inf_ttrace_supports_syscall_catchpoint (struct target_ops *self,
> +					struct gdbarch *gdbarch)
> +{
> +  return 1;
> +}
> +
>  
>  struct target_ops *
>  inf_ttrace_target (void)
> @@ -1206,6 +1215,7 @@ inf_ttrace_target (void)
>    t->to_pid_to_str = inf_ttrace_pid_to_str;
>    t->to_xfer_partial = inf_ttrace_xfer_partial;
>    t->to_get_ada_task_ptid = inf_ttrace_get_ada_task_ptid;
> +  t->to_supports_syscall_catchpoint = inf_ttrace_supports_syscall_catchpoint;
>  
>    return t;
>  }
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 2e1133d..6ebcc3e 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -594,6 +594,15 @@ linux_child_set_syscall_catchpoint (struct target_ops *self,
>    return 0;
>  }
>  
> +/* Implement the supports_syscall_catchpoint target_ops method.  */
> +
> +static int
> +linux_child_supports_syscall_catchpoint (struct target_ops *ops,
> +					 struct gdbarch *gdbarch)
> +{
> +  return gdbarch_get_syscall_number_p (gdbarch);
> +}
> +
>  /* On GNU/Linux there are no real LWP's.  The closest thing to LWP's
>     are processes sharing the same VM space.  A multi-threaded process
>     is basically a group of such processes.  However, such a grouping
> @@ -4277,6 +4286,7 @@ linux_target_install_ops (struct target_ops *t)
>    t->to_insert_exec_catchpoint = linux_child_insert_exec_catchpoint;
>    t->to_remove_exec_catchpoint = linux_child_remove_exec_catchpoint;
>    t->to_set_syscall_catchpoint = linux_child_set_syscall_catchpoint;
> +  t->to_supports_syscall_catchpoint = linux_child_supports_syscall_catchpoint;
>    t->to_pid_to_exec_file = linux_child_pid_to_exec_file;
>    t->to_post_startup_inferior = linux_child_post_startup_inferior;
>    t->to_post_attach = linux_child_post_attach;
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index e026179..9011acd 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -1144,6 +1144,35 @@ debug_set_syscall_catchpoint (struct target_ops *self, int arg1, int arg2, int a
>  }
>  
>  static int
> +delegate_supports_syscall_catchpoint (struct target_ops *self, struct gdbarch *arg1)
> +{
> +  self = self->beneath;
> +  return self->to_supports_syscall_catchpoint (self, arg1);
> +}
> +
> +static int
> +tdefault_supports_syscall_catchpoint (struct target_ops *self, struct gdbarch *arg1)
> +{
> +  return 0;
> +}
> +
> +static int
> +debug_supports_syscall_catchpoint (struct target_ops *self, struct gdbarch *arg1)
> +{
> +  int result;
> +  fprintf_unfiltered (gdb_stdlog, "-> %s->to_supports_syscall_catchpoint (...)\n", debug_target.to_shortname);
> +  result = debug_target.to_supports_syscall_catchpoint (&debug_target, arg1);
> +  fprintf_unfiltered (gdb_stdlog, "<- %s->to_supports_syscall_catchpoint (", debug_target.to_shortname);
> +  target_debug_print_struct_target_ops_p (&debug_target);
> +  fputs_unfiltered (", ", gdb_stdlog);
> +  target_debug_print_struct_gdbarch_p (arg1);
> +  fputs_unfiltered (") = ", gdb_stdlog);
> +  target_debug_print_int (result);
> +  fputs_unfiltered ("\n", gdb_stdlog);
> +  return result;
> +}
> +
> +static int
>  delegate_has_exited (struct target_ops *self, int arg1, int arg2, int *arg3)
>  {
>    self = self->beneath;
> @@ -3849,6 +3878,8 @@ install_delegators (struct target_ops *ops)
>      ops->to_remove_exec_catchpoint = delegate_remove_exec_catchpoint;
>    if (ops->to_set_syscall_catchpoint == NULL)
>      ops->to_set_syscall_catchpoint = delegate_set_syscall_catchpoint;
> +  if (ops->to_supports_syscall_catchpoint == NULL)
> +    ops->to_supports_syscall_catchpoint = delegate_supports_syscall_catchpoint;
>    if (ops->to_has_exited == NULL)
>      ops->to_has_exited = delegate_has_exited;
>    if (ops->to_mourn_inferior == NULL)
> @@ -4091,6 +4122,7 @@ install_dummy_methods (struct target_ops *ops)
>    ops->to_insert_exec_catchpoint = tdefault_insert_exec_catchpoint;
>    ops->to_remove_exec_catchpoint = tdefault_remove_exec_catchpoint;
>    ops->to_set_syscall_catchpoint = tdefault_set_syscall_catchpoint;
> +  ops->to_supports_syscall_catchpoint = tdefault_supports_syscall_catchpoint;
>    ops->to_has_exited = tdefault_has_exited;
>    ops->to_mourn_inferior = default_mourn_inferior;
>    ops->to_can_run = tdefault_can_run;
> @@ -4235,6 +4267,7 @@ init_debug_target (struct target_ops *ops)
>    ops->to_insert_exec_catchpoint = debug_insert_exec_catchpoint;
>    ops->to_remove_exec_catchpoint = debug_remove_exec_catchpoint;
>    ops->to_set_syscall_catchpoint = debug_set_syscall_catchpoint;
> +  ops->to_supports_syscall_catchpoint = debug_supports_syscall_catchpoint;
>    ops->to_has_exited = debug_has_exited;
>    ops->to_mourn_inferior = debug_mourn_inferior;
>    ops->to_can_run = debug_can_run;
> diff --git a/gdb/target.h b/gdb/target.h
> index 2811c47..0ef1ba2 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -541,6 +541,9 @@ struct target_ops
>      int (*to_set_syscall_catchpoint) (struct target_ops *,
>  				      int, int, int, int, int *)
>        TARGET_DEFAULT_RETURN (1);
> +    int (*to_supports_syscall_catchpoint) (struct target_ops *,
> +					   struct gdbarch *)
> +      TARGET_DEFAULT_RETURN (0);
>      int (*to_has_exited) (struct target_ops *, int, int, int *)
>        TARGET_DEFAULT_RETURN (0);
>      void (*to_mourn_inferior) (struct target_ops *)
> @@ -1523,6 +1526,12 @@ int target_follow_fork (int follow_child, int detach_fork);
>  						  pid, needed, any_count, \
>  						  table_size, table)
>  
> +/* Return true if GDBARCH on current target supports syscall catchpoint,
> +   otherwise, return false.  */
> +
> +#define target_supports_syscall_catchpoint(gdbarch)			\
> +  (*current_target.to_supports_syscall_catchpoint) (&current_target, gdbarch)
> +
>  /* Returns TRUE if PID has exited.  And, also sets EXIT_STATUS to the
>     exit code of PID, if any.  */
>  
> -- 
> 1.9.1

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/