This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] Add an optional "alias" attribute to syscall entries.


On Wednesday, October 03 2018, John Baldwin wrote:

> When setting a syscall catchpoint by name, catch syscalls whose name
> or alias matches the requested string.

Thanks for the patch, John.

> When the ABI of a system call is changed in the FreeBSD kernel, this
> is implemented by leaving a compatability system call using the old
> ABI at the existing "slot" and allocating a new system call for the
> version using the new ABI.  For example, new fields were added to the
> 'struct kevent' used by the kevent() system call in FreeBSD 12.  The
> previous kevent() system call in FreeBSD 12 kernels is now called
> freebsd11_kevent() and is still used by older binaries compiled
> against the older ABI.  The freebsd11_kevent() system call can be
> tagged with an "alias" attribute of "kevent" permitting 'catch syscall
> kevent' to catch both system calls and providing the expected user
> behavior for both old and new binaries.  It also provides the expected
> behavior if GDB is compiled on an older host (such as a FreeBSD 11
> host).

OOC, do you envision the possibility of a syscall having more than 1
alias?

> gdb/ChangeLog:
>
> 	* break-catch-syscall.c (catch_syscall_split_args): Update for
> 	get_syscalls_by_name returning a vector.
> 	* gdbarch.sh (UNKNOWN_SYSCALL): Remove.
> 	* gdbarch.h: Regenerate.
> 	* syscalls/gdb-syscalls.dtd (syscall): Add alias attribute.
> 	* xml-syscall.c [!HAVE_LIBEXPAT] (get_syscalls_by_name): Rename
> 	from get_syscall_by_name.  Now returns a vector of integers.
> 	[HAVE_LIBEXPAT] (struct syscall_desc): Add alias member.
> 	(syscall_create_syscall_desc): Add alias parameter and pass it to
> 	syscall_desc constructor.
> 	(syscall_start_syscall): Handle alias attribute.
> 	(syscall_attr): Add alias attribute.
> 	(xml_get_syscalls_by_name): Rename from xml_get_syscall_number.
> 	Now returns a vector of integers.  Add syscalls whose alias or
> 	name matches the requested name.
> 	(get_syscalls_by_name): Rename from get_syscall_by_name.  Now
> 	returns a vector of integers.
> 	* xml-syscall.h (get_syscalls_by_name): Likewise.
> ---
>  gdb/ChangeLog                 | 21 +++++++++++++++
>  gdb/break-catch-syscall.c     | 11 ++++----
>  gdb/gdbarch.h                 |  3 ---
>  gdb/gdbarch.sh                |  3 ---
>  gdb/syscalls/gdb-syscalls.dtd |  1 +
>  gdb/xml-syscall.c             | 49 +++++++++++++++++++----------------
>  gdb/xml-syscall.h             |  8 +++---
>  7 files changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index e239f201fa..1f679b48ed 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,24 @@
> +2018-10-03  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* break-catch-syscall.c (catch_syscall_split_args): Update for
> +	get_syscalls_by_name returning a vector.
> +	* gdbarch.sh (UNKNOWN_SYSCALL): Remove.
> +	* gdbarch.h: Regenerate.
> +	* syscalls/gdb-syscalls.dtd (syscall): Add alias attribute.
> +	* xml-syscall.c [!HAVE_LIBEXPAT] (get_syscalls_by_name): Rename
> +	from get_syscall_by_name.  Now returns a vector of integers.
> +	[HAVE_LIBEXPAT] (struct syscall_desc): Add alias member.
> +	(syscall_create_syscall_desc): Add alias parameter and pass it to
> +	syscall_desc constructor.
> +	(syscall_start_syscall): Handle alias attribute.
> +	(syscall_attr): Add alias attribute.
> +	(xml_get_syscalls_by_name): Rename from xml_get_syscall_number.
> +	Now returns a vector of integers.  Add syscalls whose alias or
> +	name matches the requested name.
> +	(get_syscalls_by_name): Rename from get_syscall_by_name.  Now
> +	returns a vector of integers.
> +	* xml-syscall.h (get_syscalls_by_name): Likewise.
> +
>  2018-10-02  Tom Tromey  <tom@tromey.com>
>  
>  	* aarch64-linux-tdep.c (aarch64_linux_sigframe_init): Use pulongest.
> diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
> index 93ef74c249..e9e370f4bf 100644
> --- a/gdb/break-catch-syscall.c
> +++ b/gdb/break-catch-syscall.c
> @@ -431,18 +431,19 @@ catch_syscall_split_args (const char *arg)
>  	}
>        else
>  	{
> -	  /* We have a name.  Let's check if it's valid and convert it
> -	     to a number.  */
> -	  get_syscall_by_name (gdbarch, cur_name, &s);
> +	  /* We have a name.  Let's check if it's valid and fetch a
> +	     list of matching numbers.  */
> +	  std::vector<int> numbers = get_syscalls_by_name (gdbarch, cur_name);
>  
> -	  if (s.number == UNKNOWN_SYSCALL)
> +	  if (numbers.empty ())
>  	    /* Here we have to issue an error instead of a warning,
>  	       because GDB cannot do anything useful if there's no
>  	       syscall number to be caught.  */
>  	    error (_("Unknown syscall name '%s'."), cur_name);
>  
>  	  /* Ok, it's valid.  */
> -	  result.push_back (s.number);
> +	  for (int number : numbers)
> +	    result.push_back (number);
>  	}
>      }
>  
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index fc2f1a84a1..6b7afe4a5c 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1569,9 +1569,6 @@ typedef ULONGEST (gdbarch_type_align_ftype) (struct gdbarch *gdbarch, struct typ
>  extern ULONGEST gdbarch_type_align (struct gdbarch *gdbarch, struct type *type);
>  extern void set_gdbarch_type_align (struct gdbarch *gdbarch, gdbarch_type_align_ftype *type_align);
>  
> -/* Definition for an unknown syscall, used basically in error-cases.  */
> -#define UNKNOWN_SYSCALL (-1)
> -
>  extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
>  
>  
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 670ac30c03..ee74acfd2a 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1393,9 +1393,6 @@ done
>  # close it off
>  cat <<EOF
>  
> -/* Definition for an unknown syscall, used basically in error-cases.  */
> -#define UNKNOWN_SYSCALL (-1)
> -
>  extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
>  
>  
> diff --git a/gdb/syscalls/gdb-syscalls.dtd b/gdb/syscalls/gdb-syscalls.dtd
> index c2aa478aa4..6aa73f288a 100644
> --- a/gdb/syscalls/gdb-syscalls.dtd
> +++ b/gdb/syscalls/gdb-syscalls.dtd
> @@ -12,4 +12,5 @@
>  <!ATTLIST syscall
>  	name			CDATA	#REQUIRED
>  	number			CDATA	#REQUIRED
> +	alias			CDATA	#IMPLIED
>  	groups			CDATA	#IMPLIED>
> diff --git a/gdb/xml-syscall.c b/gdb/xml-syscall.c
> index bf17642911..a96d377fac 100644
> --- a/gdb/xml-syscall.c
> +++ b/gdb/xml-syscall.c
> @@ -61,13 +61,11 @@ get_syscall_by_number (struct gdbarch *gdbarch,
>    s->name = NULL;
>  }
>  
> -void
> -get_syscall_by_name (struct gdbarch *gdbarch, const char *syscall_name,
> -		     struct syscall *s)
> +std::vector<int>
> +get_syscalls_by_name (struct gdbarch *gdbarch, const char *syscall_name)
>  {
>    syscall_warn_user ();
> -  s->number = UNKNOWN_SYSCALL;
> -  s->name = syscall_name;
> +  return std::vector<int> ();
>  }
>  
>  const char **
> @@ -96,8 +94,8 @@ get_syscall_group_names (struct gdbarch *gdbarch)
>  /* Structure which describes a syscall.  */
>  struct syscall_desc
>  {
> -  syscall_desc (int number_, std::string name_)
> -  : number (number_), name (name_)
> +  syscall_desc (int number_, std::string name_, std::string alias_)
> +  : number (number_), name (name_), alias(alias_)
                                            ^

Missing whitespace here.

>    {}
>  
>    /* The syscall number.  */
> @@ -107,6 +105,10 @@ struct syscall_desc
>    /* The syscall name.  */
>  
>    std::string name;
> +
> +  /* An optional alias.  */
> +
> +  std::string alias;
>  };
>  
>  typedef std::unique_ptr<syscall_desc> syscall_desc_up;
> @@ -206,10 +208,10 @@ syscall_group_add_syscall (struct syscalls_info *syscalls_info,
>  
>  static void
>  syscall_create_syscall_desc (struct syscalls_info *syscalls_info,
> -			     const char *name, int number,
> +			     const char *name, int number, const char *alias,
>  			     char *groups)
>  {
> -  syscall_desc *sysdesc = new syscall_desc (number, name);
> +  syscall_desc *sysdesc = new syscall_desc (number, name, alias ?: "");

As pointed by Kevin, this use of the ternary operator is not common.

>  
>    syscalls_info->syscalls.emplace_back (sysdesc);
>  
> @@ -234,6 +236,7 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>    /* syscall info.  */
>    char *name = NULL;
>    int number = 0;
> +  char *alias = NULL;
>    char *groups = NULL;
>  
>    for (const gdb_xml_value &attr : attributes)
> @@ -242,6 +245,8 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>          name = (char *) attr.value.get ();
>        else if (strcmp (attr.name, "number") == 0)
>          number = * (ULONGEST *) attr.value.get ();
> +      else if (strcmp (attr.name, "alias") == 0)
> +        alias = (char *) attr.value.get ();
>        else if (strcmp (attr.name, "groups") == 0)
>          groups = (char *) attr.value.get ();
>        else
> @@ -250,7 +255,8 @@ syscall_start_syscall (struct gdb_xml_parser *parser,
>      }
>  
>    gdb_assert (name);
> -  syscall_create_syscall_desc (data->syscalls_info, name, number, groups);
> +  syscall_create_syscall_desc (data->syscalls_info, name, number, alias,
> +			       groups);
>  }
>  
>  
> @@ -258,6 +264,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 },
> +  { "alias", GDB_XML_AF_OPTIONAL, NULL, NULL },
>    { "groups", GDB_XML_AF_OPTIONAL, NULL, NULL },
>    { NULL, GDB_XML_AF_NONE, NULL, NULL }
>  };
> @@ -389,21 +396,21 @@ syscall_group_get_group_by_name (const struct syscalls_info *syscalls_info,
>    return NULL;
>  }
>  
> -static int
> -xml_get_syscall_number (struct gdbarch *gdbarch,
> -                        const char *syscall_name)
> +static std::vector<int>
> +xml_get_syscalls_by_name (struct gdbarch *gdbarch, const char *syscall_name)
>  {
>    struct syscalls_info *syscalls_info = gdbarch_syscalls_info (gdbarch);
> +  std::vector<int> syscalls;
>  
>    if (syscalls_info == NULL
>        || syscall_name == NULL)
> -    return UNKNOWN_SYSCALL;
> +    return syscalls;
>  
>    for (const syscall_desc_up &sysdesc : syscalls_info->syscalls)
> -    if (sysdesc->name == syscall_name)
> -      return sysdesc->number;
> +    if (sysdesc->name == syscall_name || sysdesc->alias == syscall_name)
> +      syscalls.push_back (sysdesc->number);

You can simplify this code by putting the "for" above inside an "if"
like:

  if (syscalls_info != NULL && syscall_name != NULL)
    for (const syscall_desc_up &sysdesc : syscalls_info->syscalls)
      if (sysdesc->name == syscall_name || sysdesc->alias == syscall_name)
	syscalls.push_back (sysdesc->number);

And then you can get rid of the first "if".

>  
> -  return UNKNOWN_SYSCALL;
> +  return syscalls;
>  }
>  
>  static const char *
> @@ -522,14 +529,12 @@ get_syscall_by_number (struct gdbarch *gdbarch,
>    s->name = xml_get_syscall_name (gdbarch, syscall_number);
>  }
>  
> -void
> -get_syscall_by_name (struct gdbarch *gdbarch,
> -		     const char *syscall_name, struct syscall *s)
> +std::vector<int>
> +get_syscalls_by_name (struct gdbarch *gdbarch, const char *syscall_name)

I confess I don't feel very happy with this rename.  This function
expects the full name of the syscall to be passed via SYSCALL_NAME, and
it doesn't do any fuzzy matching, so it can be confusing to the reader
understanding why SYSCALL_NAME can map to more than 1 syscall number.
Of course one can put an explanation in the comment, but I'd rather see
a more explicit interface.  Maybe you can extend "struct syscall" and
include fields for an "alias_name" and "alias_number" there.

>  {
>    init_syscalls_info (gdbarch);
>  
> -  s->number = xml_get_syscall_number (gdbarch, syscall_name);
> -  s->name = syscall_name;
> +  return xml_get_syscalls_by_name (gdbarch, syscall_name);
>  }
>  
>  const char **
> diff --git a/gdb/xml-syscall.h b/gdb/xml-syscall.h
> index 4429d66400..86713097b3 100644
> --- a/gdb/xml-syscall.h
> +++ b/gdb/xml-syscall.h
> @@ -38,11 +38,11 @@ void set_xml_syscall_file_name (struct gdbarch *gdbarch,
>  void get_syscall_by_number (struct gdbarch *gdbarch,
>  			    int syscall_number, struct syscall *s);
>  
> -/* Function that retrieves the syscall number corresponding to the given
> -   name.  It puts the requested information inside 'struct syscall'.  */
> +/* Function that retrieves the syscall numbers corresponding to the given
> +   name.  The list of syscall numbers are returned in a vector.  */
>  
> -void get_syscall_by_name (struct gdbarch *gdbarch,
> -			  const char *syscall_name, struct syscall *s);
> +std::vector<int> get_syscalls_by_name (struct gdbarch *gdbarch,
> +				       const char *syscall_name);
>  
>  /* Function used to retrieve the list of syscalls in the system.  This list
>     is returned as an array of strings.  Returns the list of syscalls in the
> -- 
> 2.18.0

I confess I didn't run your code.  What's the output of "catch syscall"
when an alias is found?  I think it might be interesting to explicitly
mark the extra syscall as an alias.  It might take a bit more tweaking
to the code, but I like the idea of being explicit here.

If you decide to go this route, it's also good to update the "catch
syscall" documentation and testcase.

I'd like to know what you and others think, of course.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]