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

[John Baldwin <jhb at FreeBSD dot org>]

On 10/12/18 6:51 PM, Sergio Durigan Junior wrote:
> 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?

I don't see a use case for more than one alias given the existing group
functionality.
 
>> @@ -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.

Fixed.

>>    {}
>>  
>>    /* The syscall number.  */
>> @@ -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.

Yes, I'll expand.
 
>> @@ -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".

Ok.

>>  
>> -  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.

Alias fields wouldn't work as you can have N aliases.  For example in the
updated mapping for FreeBSD 12 there are 3 versions of "fhstatfs"
so that 'catch syscall fhstatfs' matches 3 syscalls:

(gdb) catch syscall fhstatfs
Catchpoint 1 (syscalls 'freebsd4_fhstatfs' [297] 'freebsd11_fhstatfs' [398] 'fhstatfs' [558])

The only caller of this function doesn't actually expect to get full
'struct syscall' objects back, but just integers.  I could instead return
a vector of 'struct syscall' perhaps but only the integers would be used.

That said, get_syscalls_by_group returns an array of 'struct syscall'
objects (it doesn't use a vector, but uses a plain C array).  We could
share some code in the consumer if we made get_syscalls_by_group and this
function follow the same convention.  I would probably want to use a
std::vector rather than a plain C array as the memory management/ownership
is cleaner.

> 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.

I posted some output above.  The aliases are kind of marked already due to
their name in the case of FreeBSD's syscall table.

-- 
John Baldwin