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

[John Baldwin <jhb@FreeBSD.org>]

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.

-- 
John Baldwin