Re: [PATCH] Add SVR4 psABI specific parser for AUXV entries

[Kamil Rytarowski <n54@gmx.com>]

On 08.04.2020 06:11, Simon Marchi wrote:
> On 2020-04-07 10:14 p.m., Kamil Rytarowski wrote:
>> NetBSD and OpenBSD always use an int to store the type as
>> defined in the SVR4 psABI specifications rather than long
>> as assumed by the default parser.
>>
>> Define svr4_auxv_parse() that shares code with default_auxv_parse().
>>
>> Remove obsd_auxv_parse() and switch OpenBSD to svr4_auxv_parse().
>> Remove not fully accurate comment from obsd-tdep.c.
>>
>> Use svr4_auxv_parse() on NetBSD.
>>
>> gdb/ChangeLog:
>>
>> 	* auxv.h (svr4_auxv_parse): New.
>> 	* auxv.c (default_auxv_parse): Split into default_auxv_parse
>> 	and generic_auxv_parse.
>> 	(svr4_auxv_parse): Add.
>> 	* obsd-tdep.c: Include "auxv.h".
>> 	(obsd_auxv_parse): Remove.
>> 	(obsd_init_abi): Remove comment.
>> 	(obsd_init_abi): Change set_gdbarch_auxv_parse passed argument
>> 	from `obsd_auxv_parse' to `svr4_auxv_parse'.
>> 	* nbsd-tdep.c: Include "auxv.h".
>> 	(nbsd_init_abi): Call set_gdbarch_auxv_parse.
>
> Thanks, just a few minor comments.
>

Please see v2.

>> diff --git a/gdb/auxv.c b/gdb/auxv.c
>> index c13d7a22eb9..240f32236b3 100644
>> --- a/gdb/auxv.c
>> +++ b/gdb/auxv.c
>> @@ -248,34 +248,64 @@ memory_xfer_auxv (struct target_ops *ops,
>>    return procfs_xfer_auxv (readbuf, writebuf, offset, len, xfered_len);
>>  }
>>
>> -/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
>> -   Return 0 if *READPTR is already at the end of the buffer.
>> -   Return -1 if there is insufficient buffer for a whole entry.
>> -   Return 1 if an entry was read into *TYPEP and *VALP.  */
>> -int
>> -default_auxv_parse (struct target_ops *ops, gdb_byte **readptr,
>> -		   gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
>> +/* Generic auxv_parse body shared with default_auxv_parse() and
>> +   svr4_auxv_parse().  */
>
> Instead of saying that it's used by this and that function (which is doomed
> to become obsolete very easily anyway), say what is the particularity of this
> function, compared to other auxv_parse functions: it takes the size of the
> auxv type field as a parameter.
>
>> +
>> +static int
>> +generic_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr,
>> +		    gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp,
>> +		    int sizeof_auxv_type)
>>  {
>> -  const int sizeof_auxv_field = gdbarch_ptr_bit (target_gdbarch ())
>> -				/ TARGET_CHAR_BIT;
>> -  const enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
>> +  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
>> +  const int sizeof_auxv_val = TYPE_LENGTH (ptr_type);
>> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>    gdb_byte *ptr = *readptr;
>>
>>    if (endptr == ptr)
>>      return 0;
>>
>> -  if (endptr - ptr < sizeof_auxv_field * 2)
>> +  if (endptr - ptr < 2 * sizeof_auxv_val)
>>      return -1;
>>
>> -  *typep = extract_unsigned_integer (ptr, sizeof_auxv_field, byte_order);
>> -  ptr += sizeof_auxv_field;
>> -  *valp = extract_unsigned_integer (ptr, sizeof_auxv_field, byte_order);
>> -  ptr += sizeof_auxv_field;
>> +  *typep = extract_unsigned_integer (ptr, sizeof_auxv_type, byte_order);
>> +  ptr += sizeof_auxv_val;      /* Alignment.  */
>
> Instead of just "Alignment", please add a comment explaining that even if the
> auxv type takes less space than an auxv value, there is padding after the type
> such that the value is aligned on a multiple of its size (and this is why we
> advance by `sizeof_auxv_val` and not `sizeof_auxv_type`.  I think that can
> be surprising for somebody who does not know this fact.
>
>> +  *valp = extract_unsigned_integer (ptr, sizeof_auxv_val, byte_order);
>> +  ptr += sizeof_auxv_val;
>>
>>    *readptr = ptr;
>>    return 1;
>>  }
>>
>> +/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
>> +   Return 0 if *READPTR is already at the end of the buffer.
>> +   Return -1 if there is insufficient buffer for a whole entry.
>> +   Return 1 if an entry was read into *TYPEP and *VALP.  */
>
> Since this is the implementation of an exported function, replace the comment
> here with:
>
>   /* See auxv.h.  */
>
> and document in the header file.  I think the documentation you have put in the
> header file for svr4_auxv_parse is good, it explains what the function does and
> how it's different from default_auxv_parse.  I don't think it's particularly
> useful to describe all parameters here, since it's a function that implements
> an interface.  The parameters should be documented where the interface is
> defined.
>
>> +int
>> +default_auxv_parse (struct target_ops *ops, gdb_byte **readptr,
>> +		   gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
>
> This last line is not properly aligned on the parenthesis.
>
>> +{
>> +  struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>> +  const int sizeof_auxv_type = TYPE_LENGTH (ptr_type);
>> +
>> +  return generic_auxv_parse (target_gdbarch (), readptr, endptr, typep, valp,
>> +			     sizeof_auxv_type);
>> +}
>> +
>> +/* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
>> +   Return 0 if *READPTR is already at the end of the buffer.
>> +   Return -1 if there is insufficient buffer for a whole entry.
>> +   Return 1 if an entry was read into *TYPEP and *VALP.  */
>
> Same comment about the comment.
>
>> +int
>> +svr4_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr,
>> +		   gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
>
> This last line is not properly aligned on the parenthesis.
>
> Simon
>