Re: [PATCH 1/4] catch syscall -- try 4 -- Architecture-independent part

[Daniel Jacobowitz <drow at false dot org>]

Hi Sergio,

On Mon, Jan 26, 2009 at 03:02:34PM -0200, Sérgio Durigan Júnior wrote:
> +/* Used in run_command to reset syscall catchpoints fields.  */
> +
> +void
> +clear_syscall_catchpoints_info (void)
> +{
> +  struct breakpoint *b;
> +
> +  ALL_BREAKPOINTS (b)
> +    if (syscall_catchpoint_p (b))
> +      b->syscall_number = UNKNOWN_SYSCALL;
> +}
> +
>  /* Used in run_command to zero the hit count when a new run starts. */
>  
>  void

I see that the other catchpoints put mutable data in struct breakpoint
too.  But it's really not a good idea :-(

In non-stop GDB, suppose we have two threads running.  They make
different system calls and stop at the catchpoint.  There's only one
'struct breakpoint', though, so do we report that they've both made
the same system call even though they might not have?

Can this go somewhere else?  There's already a copy in the waitstatus,
maybe it can stay there if we copy the whole waitstatus into 'struct
thread_info', instead of copying just stop_signal.

> +  get_last_target_status (&ptid, &last);
> +
> +  annotate_catchpoint (b->number);
> +
> +  if (s.name == NULL)
> +    syscall_id = xstrprintf ("%d", b->syscall_number);
> +  else
> +    syscall_id = xstrprintf ("'%s'", s.name);

For instance, here we've got the waitstatus in addition to the breakpoint.

> @@ -4654,7 +4670,241 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
>    print_mention_catch_vfork
>  };
>  
> -/* Create a new breakpoint of the bp_catchpoint kind and return it.
> +/* We keep a count of the number of times the user has requested a
> +   particular syscall to be tracked, and pass this information to the
> +   target.  This lets capable targets implement filtering directly.  */
> +
> +/* Number of times that "any" syscall is requested.  */
> +static int any_syscall_count;
> +
> +/* Count of each system call.  */
> +static int *syscalls_counts;
> +
> +/* Number of system entries in SYSCALLS_COUNTS.  */
> +static int syscalls_size;
> +
> +/* This counts all syscall catch requests, so we can readily determine
> +   if any catching is necessary.  */
> +static int total_syscalls_count;

At some point, this information should be per-process.  That can be a
future enhancement though.  Just because we want to trace system calls
in one inferior does't mean we need to use PTRACE_SYSCALL in the
other.

> +/* Implement the "print_one" breakpoint_ops method for syscall
> +   catchpoints.  */
> +
> +static void
> +print_one_catch_syscall (struct breakpoint *b, CORE_ADDR *last_addr)
> +{

Have you tried hitting a syscall catchpoint in MI mode, and is the
output anything useful?

> +          if (s.name == NULL)
> +            /* We can issue just a warning, but still create the catchpoint.
> +               This is because, even not knowing the syscall name that
> +               this number represents, we can still try to catch the syscall
> +               number.  */
> +            warning (_("The number '%d' does not represent a known syscall."),
> +                     syscall_number);

I don't think this warning is useful; if they're entering numbers,
hopefully they know what they're doing.

> @@ -8352,36 +8803,50 @@ Set temporary catchpoints to catch events."),
>  Catch an exception, when caught.\n\
>  With an argument, catch only exceptions with the given name."),
>  		     catch_catch_command,
> +                     NULL,
>  		     CATCH_PERMANENT,
>  		     CATCH_TEMPORARY);

Careful about spaces vs tabs; in general, you should not have any
sequence of eight spaces in an added line in a patch.

> @@ -337,6 +338,17 @@ enum watchpoint_triggered
>    watch_triggered_yes  
>  };
>  
> +/* A syscall filter is represented as a linked list of syscall
> +   numbers.  */
> +struct syscall_filter
> +{
> +  /* The system call to accept.  */
> +  int syscall;
> +
> +  /* The next filter.  */
> +  struct syscall_filter *next;
> +};
> +
>  typedef struct bp_location *bp_location_p;
>  DEF_VEC_P(bp_location_p);
>  

Does this need to be a linked list?  Both this, and syscall_counts /
syscall_size, could be represented as VEC(int) (basically the same as
a C++ std::vector).  That will simplify the code a bit.

> +# Fills the struct syscall (passed as argument) with the corresponding
> +# system call represented by syscall_number.
> +M:void:get_syscall_by_number:int syscall_number, struct syscall *s:syscall_number, s
> +
> +# Fills the struct syscall (passed as argument) with the corresponding
> +# system call represented by syscall_name.
> +M:void:get_syscall_by_name:const char *syscall_name, struct syscall *s:syscall_name, s
> +
> +# Returns the array containing the syscall names for the architecture.
> +M:const char **:get_syscall_names:void:

If every target is going to use XML for this, these three do not need
to be gdbarch methods and the support code can move from linux-tdep.c
to xml-syscall.c.

> +  if (target_passed_by_entrypoint () > 0
> +      && catch_syscall_enabled () > 0)
> +    request = PT_SYSCALL;
> +  else
> +    request = PT_CONTINUE;

Why is target_passed_by_entrypoint still necessary?  If we understand
why, I think there'll be some other more appropriate flag.  Is it to
avoid using PTRACE_SYSCALL when the shell is running, before the
application starts?

> @@ -2109,6 +2114,50 @@ ensure_not_running (void)
>      error_is_running ();
>  }
>  
> +/* Auxiliary function that handles syscall entry/return events.
> +   It returns 1 if the inferior should keep going (and GDB
> +   should ignore the event), or 0 if the event deserves to be
> +   processed.  */
> +static int
> +deal_with_syscall_event (struct execution_control_state *ecs)

A debug_infrun message here that logs the syscall number could be very
handy during debugging.

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 9a7e39c..1d0f66f 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -676,6 +676,7 @@ linux_child_post_attach (int pid)
>  {
>    linux_enable_event_reporting (pid_to_ptid (pid));
>    check_for_thread_db ();
> +  linux_enable_tracesysgood (pid_to_ptid (pid));
>  }
>  
>  static void
> @@ -683,6 +684,7 @@ linux_child_post_startup_inferior (ptid_t ptid)
>  {
>    linux_enable_event_reporting (ptid);
>    check_for_thread_db ();
> +  linux_enable_tracesysgood (ptid);
>  }
>  
>  static int
> @@ -4160,6 +4162,7 @@ linux_target_install_ops (struct target_ops *t)
>    t->to_follow_fork = linux_child_follow_fork;
>    t->to_find_memory_regions = linux_nat_find_memory_regions;
>    t->to_make_corefile_notes = linux_nat_make_corefile_notes;
> +  t->to_passed_by_entrypoint = linux_passed_by_entrypoint;
>  
>    super_xfer_partial = t->to_xfer_partial;
>    t->to_xfer_partial = linux_xfer_partial;

These bits must be for another patch in the series :-)

> diff --git a/gdb/main.c b/gdb/main.c
> index 0eb9596..a4405e7 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -62,6 +62,9 @@ int dbx_commands = 0;
>  /* System root path, used to find libraries etc.  */
>  char *gdb_sysroot = 0;
>  
> +/* GDB datadir, used to store data files.  */
> +char *gdb_datadir = 0;
> +

This is an example of a piece of the patch which could be separated
out easily.  Locating a data directory is logically independent of
system call tracing.

> diff --git a/gdb/maint.c b/gdb/maint.c
> index 489234c..fae0724 100644
> --- a/gdb/maint.c
> +++ b/gdb/maint.c
> @@ -906,4 +906,12 @@ When enabled GDB is profiled."),
>  			   show_maintenance_profile_p,
>  			   &maintenance_set_cmdlist,
>  			   &maintenance_show_cmdlist);
> +  add_setshow_filename_cmd ("gdb_datadir", class_maintenance,
> +                           &gdb_datadir, _("Set GDB's datadir path."),
> +                           _("Show GDB's datadir path."),
> +                           _("\
> +When set, GDB uses the specified path to search for data files."),
> +                           NULL, NULL,
> +                           &maintenance_set_cmdlist,
> +                           &maintenance_show_cmdlist);
>  }

Just "datadir" (gdb_ is obvious, since this is GDB :-).  Or
"data-directory", which will be easier to understand for users, I
think.

-- 
Daniel Jacobowitz
CodeSourcery