Re: [PATCH v5 1/5] Implemement support for groups of syscalls in the xml-syscall interface.

[Doug Evans <dje@google.com>]

Gabriel Krisman Bertazi writes:
  > This is done by maintaining a list of syscall_group_desc for each
  > syscall group inside the syscalls_info structure.  Inside each
  > syscall_group_desc we have a vector of pointers to the syscalls that are
  > part of that group.
  >
  > gdb/
  >
  > 	* syscalls/gdb-syscalls.dtd: Include group attribute to the
  > 	syscall element.
  > 	* xml-syscall.c (get_syscalls_by_group): New.
  > 	(get_syscall_group_names): New.
  > 	(struct syscall_group_desc): New structure to store group data.
  > 	(struct syscalls_info): Include field to store the group list.
  > 	(sysinfo_free_syscall_group_desc): New.
  > 	(free_syscalls_info): Free group list.
  > 	(syscall_group_create_syscall_group_desc): New.
  > 	(syscall_group_add_syscall): New.
  > 	(syscall_create_syscall_desc): Add syscall to its groups.
  > 	(syscall_start_syscall): Load group attribute.
  > 	(syscall_group_get_group_by_name): New.
  > 	(xml_list_syscalls_by_group): New.
  > 	(xml_list_of_groups): New.
  > 	* xml-syscall.h (get_syscalls_by_group): Export function
  > 	to retrieve a list of syscalls filtered by the group name.
  > 	(get_syscall_group_names): Export function to retrieve the list
  > 	of syscall groups.

Hi. Just some nits.
[I haven't read the entire series yet though.]

  > diff --git a/gdb/syscalls/gdb-syscalls.dtd  
b/gdb/syscalls/gdb-syscalls.dtd
  > index 3deda12..de47d4d 100644
  > --- a/gdb/syscalls/gdb-syscalls.dtd
  > +++ b/gdb/syscalls/gdb-syscalls.dtd
  > @@ -11,4 +11,5 @@
  >  <!ELEMENT syscall		EMPTY>
  >  <!ATTLIST syscall
  >  	name			CDATA	#REQUIRED
  > -	number			CDATA	#REQUIRED>
  > +	number			CDATA	#REQUIRED
  > +	groups			CDATA	#IMPLIED>

I'm assuming "IMPLIED" is ok here, I don't actually know.

  > diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
  > index ceaf750..dafbb1e 100644
  > --- a/gdb/xml-syscall.c
  > +++ b/gdb/xml-syscall.c
  > @@ -77,6 +77,20 @@ get_syscall_names (struct gdbarch *gdbarch)
  >    return NULL;
  >  }
  >
  > +struct syscall *
  > +get_syscalls_by_group (struct gdbarch *gdbarch, const char *group)
  > +{
  > +  syscall_warn_user ();
  > +  return NULL;
  > +}
  > +
  > +const char **
  > +get_syscall_group_names (struct gdbarch *gdbarch)
  > +{
  > +  syscall_warn_user ();
  > +  return NULL;
  > +}
  > +
  >  #else /* ! HAVE_LIBEXPAT */
  >
  >  /* Structure which describes a syscall.  */
  > @@ -92,6 +106,19 @@ typedef struct syscall_desc
  >  } *syscall_desc_p;
  >  DEF_VEC_P(syscall_desc_p);
  >
  > +/* Structure of a syscall group.  */
  > +typedef struct syscall_group_desc
  > +{
  > +  /* The group name.  */
  > +
  > +  char *name;
  > +
  > +  /* The syscalls that are part of the group.  */
  > +
  > +  VEC(syscall_desc_p) *syscalls;
  > +} *syscall_group_desc_p;
  > +DEF_VEC_P(syscall_group_desc_p);
  > +
  >  /* Structure that represents syscalls information.  */
  >  struct syscalls_info
  >  {
  > @@ -99,6 +126,10 @@ struct syscalls_info
  >
  >    VEC(syscall_desc_p) *syscalls;
  >
  > +  /* The syscall groups.  */
  > +
  > +  VEC(syscall_group_desc_p) *groups;
  > +
  >    /* Variable that will hold the last known data-directory.  This is
  >       useful to know whether we should re-read the XML info for the
  >       target.  */
  > @@ -126,11 +157,21 @@ syscalls_info_free_syscalls_desc (struct  
syscall_desc *sd)
  >    xfree (sd->name);
  >  }
  >
  > +/* Free syscall_group_desc members but not the structure itself.  */
  > +
  > +static void
  > +syscalls_info_free_syscall_group_desc (struct syscall_group_desc *sd)
  > +{
  > +  VEC_free (syscall_desc_p, sd->syscalls);
  > +  xfree (sd->name);
  > +}
  > +
  >  static void
  >  free_syscalls_info (void *arg)
  >  {
  >    struct syscalls_info *syscalls_info = (struct syscalls_info *) arg;
  >    struct syscall_desc *sysdesc;
  > +  struct syscall_group_desc *groupdesc;
  >    int i;
  >
  >    xfree (syscalls_info->my_gdb_datadir);
  > @@ -144,6 +185,17 @@ free_syscalls_info (void *arg)
  >        VEC_free (syscall_desc_p, syscalls_info->syscalls);
  >      }
  >
  > +  if (syscalls_info->groups != NULL)
  > +    {
  > +      for (i = 0;
  > +	   VEC_iterate (syscall_group_desc_p,
  > +			syscalls_info->groups, i, groupdesc);
  > +	   i++)
  > +	syscalls_info_free_syscall_group_desc (groupdesc);
  > +
  > +      VEC_free (syscall_group_desc_p, syscalls_info->groups);
  > +    }
  > +
  >    xfree (syscalls_info);
  >  }
  >
  > @@ -153,16 +205,73 @@ make_cleanup_free_syscalls_info (struct  
syscalls_info *syscalls_info)
  >    return make_cleanup (free_syscalls_info, syscalls_info);
  >  }
  >
  > +/* Create a new syscall group.  Return pointer to the
  > +   syscall_group_desc structure that represents the new group.  */
  > +
  > +static struct syscall_group_desc *
  > +syscall_group_create_syscall_group_desc (struct syscalls_info  
*syscalls_info,
  > +					 const char *group)
  > +{
  > +  struct syscall_group_desc *groupdesc = XCNEW (struct  
syscall_group_desc);
  > +
  > +  groupdesc->name = xstrdup (group);
  > +
  > +  VEC_safe_push (syscall_group_desc_p, syscalls_info->groups,  
groupdesc);
  > +
  > +  return groupdesc;
  > +}
  > +
  > +/* Add a syscall to the group.  If group doesn't exist, create it.  */
  > +
  > +static void
  > +syscall_group_add_syscall (struct syscalls_info *syscalls_info,
  > +			   struct syscall_desc *syscall,
  > +			   const char *group)
  > +{
  > +  struct syscall_group_desc *groupdesc;
  > +  int i;
  > +
  > +  /* Search for an existing group.  */
  > +  for (i = 0;
  > +       VEC_iterate (syscall_group_desc_p, syscalls_info->groups, i,  
groupdesc);
  > +       i++)
  > +    {
  > +      if (strcmp (groupdesc->name, group) == 0)
  > +	break;
  > +    }
  > +
  > +  if (groupdesc == NULL)
  > +    {
  > +      /* No group was found with this name.  We must create a new
  > +	 one.  */
  > +      groupdesc = syscall_group_create_syscall_group_desc  
(syscalls_info,
  > +							   group);
  > +    }
  > +
  > +  VEC_safe_push (syscall_desc_p, groupdesc->syscalls, syscall);
  > +}
  > +
  >  static void
  >  syscall_create_syscall_desc (struct syscalls_info *syscalls_info,
  > -                             const char *name, int number)
  > +			     const char *name, int number,
  > +			     char *groups)
  >  {
  >    struct syscall_desc *sysdesc = XCNEW (struct syscall_desc);
  > +  char *group;
  >
  >    sysdesc->name = xstrdup (name);
  >    sysdesc->number = number;
  >
  >    VEC_safe_push (syscall_desc_p, syscalls_info->syscalls, sysdesc);
  > +
  > +  /*  Add syscall to its groups.  */
  > +  if (groups != NULL)
  > +    {
  > +      for (group = strtok (groups, ",");
  > +	   group != NULL;
  > +	   group = strtok (NULL, ","))
  > +	syscall_group_add_syscall (syscalls_info, sysdesc, group);
  > +    }
  >  }
  >
  >  /* Handle the start of a <syscall> element.  */
  > @@ -177,6 +286,7 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
  >    /* syscall info.  */
  >    char *name = NULL;
  >    int number = 0;
  > +  char *groups = NULL;
  >
  >    len = VEC_length (gdb_xml_value_s, attributes);
  >
  > @@ -186,13 +296,15 @@ syscall_start_syscall (struct gdb_xml_parser  
*parser,
  >          name = (char *) attrs[i].value;
  >        else if (strcmp (attrs[i].name, "number") == 0)
  >          number = * (ULONGEST *) attrs[i].value;
  > +      else if (strcmp (attrs[i].name, "groups") == 0)
  > +        groups = (char *) attrs[i].value;
  >        else
  >          internal_error (__FILE__, __LINE__,
  >                          _("Unknown attribute name '%s'."),  
attrs[i].name);
  >      }
  >
  >    gdb_assert (name);
  > -  syscall_create_syscall_desc (data->syscalls_info, name, number);
  > +  syscall_create_syscall_desc (data->syscalls_info, name, number,  
groups);
  >  }
  >
  >
  > @@ -200,6 +312,7 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
  >  static const struct gdb_xml_attribute syscall_attr[] = {
  >    { "number", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
  >    { "name", GDB_XML_AF_NONE, NULL, NULL },
  > +  { "groups", GDB_XML_AF_OPTIONAL, NULL, NULL },
  >    { NULL, GDB_XML_AF_NONE, NULL, NULL }
  >  };
  >
  > @@ -321,6 +434,32 @@ init_syscalls_info (struct gdbarch *gdbarch)
  >    set_gdbarch_syscalls_info (gdbarch, syscalls_info);
  >  }
  >
  > +/* Search for a syscall group by its name.  Return syscall_group_desc
  > +   structure for the group if found or NULL otherwise.  */
  > +
  > +static struct syscall_group_desc *
  > +syscall_group_get_group_by_name (const struct syscalls_info  
*syscalls_info,
  > +				 const char *group)
  > +{
  > +  struct syscall_group_desc *groupdesc;
  > +  int i;
  > +
  > +  if (syscalls_info == NULL)
  > +    return NULL;
  > +
  > +  if (group == NULL)
  > +    return NULL;
  > +
  > +   /* Search for existing group.  */
  > +  for (i = 0;
  > +       VEC_iterate (syscall_group_desc_p, syscalls_info->groups, i,  
groupdesc);
  > +       i++)
  > +    if (strcmp (groupdesc->name, group) == 0)
  > +      return groupdesc;

Wrap if in {}.
If more than one line is involved, it goes in braces (even if just one  
statement).

  > +
  > +  return NULL;
  > +}
  > +
  >  static int
  >  xml_get_syscall_number (struct gdbarch *gdbarch,
  >                          const char *syscall_name)
  > @@ -388,6 +527,75 @@ xml_list_of_syscalls (struct gdbarch *gdbarch)
  >    return names;
  >  }
  >
  > +/* Iterate over the syscall_group_desc element to return a list of
  > +   syscalls that are part of the given group, terminated by an empty
  > +   element.  If the syscall group doesn't exist, return NULL.  */
  > +
  > +static struct syscall *
  > +xml_list_syscalls_by_group (struct gdbarch *gdbarch, const char *group)
  > +{
  > +  struct syscalls_info *syscalls_info = gdbarch_syscalls_info (gdbarch);
  > +  struct syscall_group_desc *groupdesc;
  > +  struct syscall_desc *sysdesc;
  > +  struct syscall *syscalls = NULL;
  > +  int nsyscalls;
  > +  int i;
  > +
  > +  if (syscalls_info == NULL)
  > +    return NULL;
  > +
  > +  groupdesc = syscall_group_get_group_by_name (syscalls_info, group);
  > +  if (groupdesc == NULL)
  > +    return NULL;
  > +
  > +  nsyscalls = VEC_length (syscall_desc_p, groupdesc->syscalls);
  > +  syscalls = (struct syscall*) xmalloc ((nsyscalls + 1)
  > +					* sizeof (struct syscall));
  > +
  > +  for (i = 0;
  > +       VEC_iterate (syscall_desc_p, groupdesc->syscalls, i, sysdesc);
  > +       i++)
  > +    {
  > +      syscalls[i].name = sysdesc->name;
  > +      syscalls[i].number = sysdesc->number;
  > +    }
  > +
  > +  /* Add final element marker.  */
  > +  syscalls[i].name = NULL;
  > +  syscalls[i].number = 0;
  > +
  > +  return syscalls;
  > +}
  > +
  > +/* Return a NULL terminated list of syscall groups or an empty list, if
  > +   no syscall group is available.  Return NULL, if there is no syscall
  > +   information available.  */
  > +
  > +static const char **
  > +xml_list_of_groups (struct gdbarch *gdbarch)
  > +{
  > +  struct syscalls_info *syscalls_info = gdbarch_syscalls_info (gdbarch);
  > +  struct syscall_group_desc *groupdesc;
  > +  const char **names = NULL;
  > +  int i;
  > +  int ngroups;
  > +
  > +  if (syscalls_info == NULL)
  > +    return NULL;
  > +
  > +  ngroups = VEC_length (syscall_group_desc_p, syscalls_info->groups);
  > +  names = (const char**) xmalloc ((ngroups + 1) * sizeof (char *));
  > +
  > +  for (i = 0;
  > +       VEC_iterate (syscall_group_desc_p, syscalls_info->groups, i,  
groupdesc);
  > +       i++)
  > +    names[i] = groupdesc->name;
  > +
  > +  names[i] = NULL;
  > +
  > +  return names;
  > +}
  > +
  >  void
  >  set_xml_syscall_file_name (struct gdbarch *gdbarch, const char *name)
  >  {
  > @@ -422,4 +630,24 @@ get_syscall_names (struct gdbarch *gdbarch)
  >    return xml_list_of_syscalls (gdbarch);
  >  }
  >
  > +/* See comment in xml-syscall.h.  */
  > +
  > +struct syscall *
  > +get_syscalls_by_group (struct gdbarch *gdbarch, const char *group)
  > +{
  > +  init_syscalls_info (gdbarch);
  > +
  > +  return xml_list_syscalls_by_group (gdbarch, group);
  > +}
  > +
  > +/* See comment in xml-syscall.h.  */
  > +
  > +const char **
  > +get_syscall_group_names (struct gdbarch *gdbarch)
  > +{
  > +  init_syscalls_info (gdbarch);
  > +
  > +  return xml_list_of_groups (gdbarch);
  > +}
  > +
  >  #endif /* ! HAVE_LIBEXPAT */
  > diff --git a/gdb/xml-syscall.h b/gdb/xml-syscall.h
  > index b0dd401..407613e 100644
  > --- a/gdb/xml-syscall.h
  > +++ b/gdb/xml-syscall.h
  > @@ -50,4 +50,20 @@ void get_syscall_by_name (struct gdbarch *gdbarch,
  >
  >  const char **get_syscall_names (struct gdbarch *gdbarch);
  >
  > +/* Function used to retrieve the list of syscalls of a given group in
  > +   the system.  Return a list of syscalls that are element of the
  > +   group, terminated by an empty element. The list is malloc'ed
  > +   and must be freed by the caller.  If group doesn't exist, return
  > +   NULL.  */
  > +
  > +struct syscall *get_syscalls_by_group (struct gdbarch *gdbarch,
  > +				       const char *group);
  > +
  > +/* Function used to retrieve the list of syscall groups in the system.
  > +   Return an array of strings terminated by a NULL element.  The list
  > +   must be freed by the caller.  Return NULL if there is no syscall
  > +   information available.  */
  > +
  > +const char **get_syscall_group_names (struct gdbarch *gdbarch);
  > +
  >  #endif /* XML_SYSCALL_H */
  > --
  > 2.4.3