| Message ID | 20181106175431.59832-3-jhb@FreeBSD.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 6 Nov 2018 09:54:30 -0800 John Baldwin <jhb@FreeBSD.org> wrote: > When setting a syscall catchpoint by name, catch syscalls whose name > or alias matches the requested string. > > When the ABI of a system call is changed in the FreeBSD kernel, this > is implemented by leaving a compatibility 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). > > 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. Okay. Kevin
On 2018-11-06 12:54, John Baldwin wrote: > When setting a syscall catchpoint by name, catch syscalls whose name > or alias matches the requested string. > > When the ABI of a system call is changed in the FreeBSD kernel, this > is implemented by leaving a compatibility 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). Will we ever need to provide more than one alias to a syscall? Simon
On 2018-11-09 10:28, Simon Marchi wrote: > On 2018-11-06 12:54, John Baldwin wrote: >> When setting a syscall catchpoint by name, catch syscalls whose name >> or alias matches the requested string. >> >> When the ABI of a system call is changed in the FreeBSD kernel, this >> is implemented by leaving a compatibility 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). > > Will we ever need to provide more than one alias to a syscall? Sorry, I just saw you have already answered this question, here: https://sourceware.org/ml/gdb-patches/2018-10/msg00385.html Simon
Hi, On 11/06/2018 05:54 PM, John Baldwin wrote: > --- 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> I think there should be a NEWS entry for this. I'd think there should be a docs update as well, but it looks like this dtd isn't documented in the manual. :-/ > + /* 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); Nit: this return-vector interface seems like leads to a bit of unnecessary work, and double the number of necessary vector/heap allocations -- if get_syscalls_by_name were passed RESULT instead of returning a new vector, it could add to RESULT directly? The code above would like something like this instead with get_syscalls_by_name returning true/false to indicate whether something was added: /* We have a name. Let's check if it's valid and fetch a list of matching numbers. */ if (!get_syscalls_by_name (gdbarch, cur_name, result)) /* 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); That means there's no need to copy numbers between the vectors (since there's only one vector). Same for get_syscalls_by_group. Note that instead of: > + for (int number : numbers) > + result.push_back (number); You could write: result.insert (result.end (), numbers.begin (), numbers.end ()); which may be more efficient assuming insert is smart enough to call std::vector::reserve to allocate space in one go. Thanks, Pedro Alves
On 11/9/18 8:31 AM, Pedro Alves wrote: > Hi, > > On 11/06/2018 05:54 PM, John Baldwin wrote: > >> --- 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> > > I think there should be a NEWS entry for this. Ok. > I'd think there should be a docs update as well, but it > looks like this dtd isn't documented in the manual. :-/ > > >> + /* 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); > > Nit: this return-vector interface seems like leads to a bit of unnecessary > work, and double the number of necessary vector/heap allocations -- if > get_syscalls_by_name were passed RESULT instead of returning a new vector, > it could add to RESULT directly? The code above would like something like this > instead with get_syscalls_by_name returning true/false to indicate > whether something was added: > > /* We have a name. Let's check if it's valid and fetch a > list of matching numbers. */ > if (!get_syscalls_by_name (gdbarch, cur_name, result)) > /* 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); > > That means there's no need to copy numbers between the vectors > (since there's only one vector). > > Same for get_syscalls_by_group. Oh, yes, that is a much better idea. I will adopt that change in both patches. > Note that instead of: > >> + for (int number : numbers) >> + result.push_back (number); > > You could write: > > result.insert (result.end (), numbers.begin (), numbers.end ()); > > which may be more efficient assuming insert is smart enough > to call std::vector::reserve to allocate space in one go. Ok. I wasn't quite sure if there was a more efficient idiom to use. It's a bit of a shame std::vector<> doesn't have an explicit append/join method similar to list.append() in Python.
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 9f4c450e4e..487c97a957 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,24 @@ +2018-11-06 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-11-06 John Baldwin <jhb@FreeBSD.org> * break-catch-syscall.c (catch_syscall_split_args): Update for diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c index 8e1de10fa7..b6f0978b5d 100644 --- a/gdb/break-catch-syscall.c +++ b/gdb/break-catch-syscall.c @@ -425,18 +425,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 2cb6961083..8e354d2278 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 bbfa8d2205..ff0e751f48 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 c3cd425186..ba9df0475b 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_) {} /* 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,11 @@ 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 != NULL ? alias : ""); syscalls_info->syscalls.emplace_back (sysdesc); @@ -234,6 +237,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 +246,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 +256,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 +265,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 +397,18 @@ 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; + 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); - for (const syscall_desc_up &sysdesc : syscalls_info->syscalls) - if (sysdesc->name == syscall_name) - return sysdesc->number; - - return UNKNOWN_SYSCALL; + return syscalls; } static const char * @@ -506,14 +511,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) { 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 95c968c9c7..9bb97d0922 100644 --- a/gdb/xml-syscall.h +++ b/gdb/xml-syscall.h @@ -38,11 +38,12 @@ 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 numbers of all syscalls with either a name or + alias equal to SYSCALL_NAME 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