[v3] Add support for "info auxv" on NetBSD

Message ID 20200320172739.26705-1-n54@gmx.com
State New
Headers show
Series
  • [v3] Add support for "info auxv" on NetBSD
Related show

Commit Message

Kamil Rytarowski March 20, 2020, 5:27 p.m.
Register nbsd_auxv_parse() that overloads the default (Linux-style)
AUXV parsing. On NetBSD the type parameter is defined as int32_t
for all architectures.

Register nbsd_init_abi() that sets set_gdbarch_auxv_parse().

Call nbsd_init_abi() from all CPU-specific NetBSD tdep files.

gdb/ChangeLog:

	* nbsd-tdep.c: Include "gdbarch.h".
	* nbsd-tdep.c (nbsd_auxv_parse, nbsd_init_abi): New.
	* alpha-nbsd-tdep.c (alphanbsd_init_abi): Call nbsd_init_abi().
	* amd64-nbsd-tdep.c (amd64nbsd_init_abi): Likewise.
	* arm-nbsd-tdep.c (arm_netbsd_elf_init_abi): Likewise.
	* hppa-nbsd-tdep.c (hppanbsd_init_abi): Likewise.
	* i386-nbsd-tdep.c (i386nbsd_init_abi): Likewise.
	* mips-nbsd-tdep.c (nbsd_init_abi): Likewise.
	* ppc-nbsd-tdep.c (ppcnbsd_init_abi): Likewise.
	* sh-nbsd-tdep.c (shnbsd_init_abi): Likewise.
	* sparc-nbsd-tdep.c (sparc32nbsd_init_abi): Likewise.
	* sparc64-nbsd-tdep.c (sparc64nbsd_init_abi): Likewise.
	* vax-nbsd-tdep.c (vaxnbsd_elf_init_abi): Likewise.
---
 gdb/ChangeLog           | 16 ++++++++++++++++
 gdb/alpha-nbsd-tdep.c   |  2 ++
 gdb/amd64-nbsd-tdep.c   |  1 +
 gdb/arm-nbsd-tdep.c     |  3 +++
 gdb/hppa-nbsd-tdep.c    |  2 ++
 gdb/i386-nbsd-tdep.c    |  2 ++
 gdb/mips-nbsd-tdep.c    |  2 ++
 gdb/nbsd-tdep.c         | 38 ++++++++++++++++++++++++++++++++++++++
 gdb/nbsd-tdep.h         |  4 ++++
 gdb/ppc-nbsd-tdep.c     |  2 ++
 gdb/sh-nbsd-tdep.c      |  1 +
 gdb/sparc-nbsd-tdep.c   |  2 ++
 gdb/sparc64-nbsd-tdep.c |  2 ++
 gdb/vax-nbsd-tdep.c     |  2 ++
 14 files changed, 79 insertions(+)

--
2.25.0

Comments

Kamil Rytarowski March 26, 2020, 11:26 p.m. | #1
Ping?

On 20.03.2020 18:27, Kamil Rytarowski wrote:
> Register nbsd_auxv_parse() that overloads the default (Linux-style)

> AUXV parsing. On NetBSD the type parameter is defined as int32_t

> for all architectures.

>

> Register nbsd_init_abi() that sets set_gdbarch_auxv_parse().

>

> Call nbsd_init_abi() from all CPU-specific NetBSD tdep files.

>

> gdb/ChangeLog:

>

> 	* nbsd-tdep.c: Include "gdbarch.h".

> 	* nbsd-tdep.c (nbsd_auxv_parse, nbsd_init_abi): New.

> 	* alpha-nbsd-tdep.c (alphanbsd_init_abi): Call nbsd_init_abi().

> 	* amd64-nbsd-tdep.c (amd64nbsd_init_abi): Likewise.

> 	* arm-nbsd-tdep.c (arm_netbsd_elf_init_abi): Likewise.

> 	* hppa-nbsd-tdep.c (hppanbsd_init_abi): Likewise.

> 	* i386-nbsd-tdep.c (i386nbsd_init_abi): Likewise.

> 	* mips-nbsd-tdep.c (nbsd_init_abi): Likewise.

> 	* ppc-nbsd-tdep.c (ppcnbsd_init_abi): Likewise.

> 	* sh-nbsd-tdep.c (shnbsd_init_abi): Likewise.

> 	* sparc-nbsd-tdep.c (sparc32nbsd_init_abi): Likewise.

> 	* sparc64-nbsd-tdep.c (sparc64nbsd_init_abi): Likewise.

> 	* vax-nbsd-tdep.c (vaxnbsd_elf_init_abi): Likewise.

> ---

>  gdb/ChangeLog           | 16 ++++++++++++++++

>  gdb/alpha-nbsd-tdep.c   |  2 ++

>  gdb/amd64-nbsd-tdep.c   |  1 +

>  gdb/arm-nbsd-tdep.c     |  3 +++

>  gdb/hppa-nbsd-tdep.c    |  2 ++

>  gdb/i386-nbsd-tdep.c    |  2 ++

>  gdb/mips-nbsd-tdep.c    |  2 ++

>  gdb/nbsd-tdep.c         | 38 ++++++++++++++++++++++++++++++++++++++

>  gdb/nbsd-tdep.h         |  4 ++++

>  gdb/ppc-nbsd-tdep.c     |  2 ++

>  gdb/sh-nbsd-tdep.c      |  1 +

>  gdb/sparc-nbsd-tdep.c   |  2 ++

>  gdb/sparc64-nbsd-tdep.c |  2 ++

>  gdb/vax-nbsd-tdep.c     |  2 ++

>  14 files changed, 79 insertions(+)

>

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog

> index 650b74bae4a..572403001ce 100644

> --- a/gdb/ChangeLog

> +++ b/gdb/ChangeLog

> @@ -1,3 +1,19 @@

> +2020-03-20  Kamil Rytarowski  <n54@gmx.com>

> +

> +	* nbsd-tdep.c: Include "gdbarch.h".

> +	* nbsd-tdep.c (nbsd_auxv_parse, nbsd_init_abi): New.

> +	* alpha-nbsd-tdep.c (alphanbsd_init_abi): Call nbsd_init_abi().

> +	* amd64-nbsd-tdep.c (amd64nbsd_init_abi): Likewise.

> +	* arm-nbsd-tdep.c (arm_netbsd_elf_init_abi): Likewise.

> +	* hppa-nbsd-tdep.c (hppanbsd_init_abi): Likewise.

> +	* i386-nbsd-tdep.c (i386nbsd_init_abi): Likewise.

> +	* mips-nbsd-tdep.c (nbsd_init_abi): Likewise.

> +	* ppc-nbsd-tdep.c (ppcnbsd_init_abi): Likewise.

> +	* sh-nbsd-tdep.c (shnbsd_init_abi): Likewise.

> +	* sparc-nbsd-tdep.c (sparc32nbsd_init_abi): Likewise.

> +	* sparc64-nbsd-tdep.c (sparc64nbsd_init_abi): Likewise.

> +	* vax-nbsd-tdep.c (vaxnbsd_elf_init_abi): Likewise.

> +

>  2020-03-20  Kamil Rytarowski  <n54@gmx.com>

>

>  	* amd64-bsd-nat.c (gdb_ptrace): Change return type from `int' to

> diff --git a/gdb/alpha-nbsd-tdep.c b/gdb/alpha-nbsd-tdep.c

> index ab9240e35da..58294edb3f6 100644

> --- a/gdb/alpha-nbsd-tdep.c

> +++ b/gdb/alpha-nbsd-tdep.c

> @@ -258,6 +258,8 @@ alphanbsd_init_abi (struct gdbarch_info info,

>    /* Hook into the MDEBUG frame unwinder.  */

>    alpha_mdebug_init_abi (info, gdbarch);

>

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* NetBSD/alpha does not provide single step support via ptrace(2); we

>       must use software single-stepping.  */

>    set_gdbarch_software_single_step (gdbarch, alpha_software_single_step);

> diff --git a/gdb/amd64-nbsd-tdep.c b/gdb/amd64-nbsd-tdep.c

> index 89d07236b85..59071488ed7 100644

> --- a/gdb/amd64-nbsd-tdep.c

> +++ b/gdb/amd64-nbsd-tdep.c

> @@ -106,6 +106,7 @@ amd64nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>

>    amd64_init_abi (info, gdbarch,

>  		  amd64_target_description (X86_XSTATE_SSE_MASK, true));

> +  nbsd_init_abi (info, gdbarch);

>

>    tdep->jb_pc_offset = 7 * 8;

>

> diff --git a/gdb/arm-nbsd-tdep.c b/gdb/arm-nbsd-tdep.c

> index e01df50bc25..a6104f760b3 100644

> --- a/gdb/arm-nbsd-tdep.c

> +++ b/gdb/arm-nbsd-tdep.c

> @@ -150,6 +150,9 @@ arm_netbsd_elf_init_abi (struct gdbarch_info info,

>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

>

>    arm_netbsd_init_abi_common (info, gdbarch);

> +

> +  nbsd_init_abi (info, gdbarch);

> +

>    if (tdep->fp_model == ARM_FLOAT_AUTO)

>      tdep->fp_model = ARM_FLOAT_SOFT_VFP;

>

> diff --git a/gdb/hppa-nbsd-tdep.c b/gdb/hppa-nbsd-tdep.c

> index b532ab1d5cc..d601aa96f3f 100644

> --- a/gdb/hppa-nbsd-tdep.c

> +++ b/gdb/hppa-nbsd-tdep.c

> @@ -201,6 +201,8 @@ hppanbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>    /* Obviously NetBSD is BSD-based.  */

>    hppabsd_init_abi (info, gdbarch);

>

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* Core file support.  */

>    set_gdbarch_iterate_over_regset_sections

>      (gdbarch, hppanbsd_iterate_over_regset_sections);

> diff --git a/gdb/i386-nbsd-tdep.c b/gdb/i386-nbsd-tdep.c

> index 3157451e08f..f350412d9bd 100644

> --- a/gdb/i386-nbsd-tdep.c

> +++ b/gdb/i386-nbsd-tdep.c

> @@ -377,6 +377,8 @@ i386nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>    /* Obviously NetBSD is BSD-based.  */

>    i386bsd_init_abi (info, gdbarch);

>

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* NetBSD has a different `struct reg'.  */

>    tdep->gregset_reg_offset = i386nbsd_r_reg_offset;

>    tdep->gregset_num_regs = ARRAY_SIZE (i386nbsd_r_reg_offset);

> diff --git a/gdb/mips-nbsd-tdep.c b/gdb/mips-nbsd-tdep.c

> index 38bc7ce636b..6f4d22b24fb 100644

> --- a/gdb/mips-nbsd-tdep.c

> +++ b/gdb/mips-nbsd-tdep.c

> @@ -354,6 +354,8 @@ static void

>  mipsnbsd_init_abi (struct gdbarch_info info,

>                     struct gdbarch *gdbarch)

>  {

> +  nbsd_init_abi (info, gdbarch)

> +

>    set_gdbarch_iterate_over_regset_sections

>      (gdbarch, mipsnbsd_iterate_over_regset_sections);

>

> diff --git a/gdb/nbsd-tdep.c b/gdb/nbsd-tdep.c

> index 49bb2b706bd..5d5ea26622e 100644

> --- a/gdb/nbsd-tdep.c

> +++ b/gdb/nbsd-tdep.c

> @@ -22,6 +22,7 @@

>  #include "defs.h"

>  #include "solib-svr4.h"

>  #include "nbsd-tdep.h"

> +#include "gdbarch.h"

>

>  /* FIXME: kettenis/20060115: We should really eliminate the next two

>     functions completely.  */

> @@ -47,3 +48,40 @@ nbsd_pc_in_sigtramp (CORE_ADDR pc, const char *func_name)

>    return (func_name != NULL

>  	  && startswith (func_name, "__sigtramp"));

>  }

> +

> +/* NetBSD-specific parser for AUXV data with. NetBSD follows the ELF

> +   specification, contrary to some other ELF Operating Systems.  */

> +

> +static int

> +nbsd_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr,

> +		 gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)

> +{

> +  struct type *int_type = builtin_type (gdbarch)->builtin_int;

> +  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;

> +  const int sizeof_auxv_type = TYPE_LENGTH (int_type);

> +  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 < 2 * sizeof_auxv_val)

> +    return -1;

> +

> +  *typep = extract_unsigned_integer (ptr, sizeof_auxv_type, byte_order);

> +  ptr += sizeof_auxv_val;	/* Alignment.  */

> +  *valp = extract_unsigned_integer (ptr, sizeof_auxv_val, byte_order);

> +  ptr += sizeof_auxv_val;

> +

> +  *readptr = ptr;

> +  return 1;

> +}

> +

> +/* See nbsd-tdep.h.  */

> +

> +void

> +nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

> +{

> +  set_gdbarch_auxv_parse (gdbarch, nbsd_auxv_parse);

> +}

> diff --git a/gdb/nbsd-tdep.h b/gdb/nbsd-tdep.h

> index c99a8b537b6..4b06c13f87b 100644

> --- a/gdb/nbsd-tdep.h

> +++ b/gdb/nbsd-tdep.h

> @@ -25,4 +25,8 @@ struct link_map_offsets *nbsd_lp64_solib_svr4_fetch_link_map_offsets (void);

>

>  int nbsd_pc_in_sigtramp (CORE_ADDR, const char *);

>

> +/* NetBSD specific set of ABI-related routines.  */

> +

> +void nbsd_init_abi (struct gdbarch_info, struct gdbarch *);

> +

>  #endif /* NBSD_TDEP_H */

> diff --git a/gdb/ppc-nbsd-tdep.c b/gdb/ppc-nbsd-tdep.c

> index d75930c9f8d..81492deaccd 100644

> --- a/gdb/ppc-nbsd-tdep.c

> +++ b/gdb/ppc-nbsd-tdep.c

> @@ -173,6 +173,8 @@ static void

>  ppcnbsd_init_abi (struct gdbarch_info info,

>                    struct gdbarch *gdbarch)

>  {

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* For NetBSD, this is an on again, off again thing.  Some systems

>       do use the broken struct convention, and some don't.  */

>    set_gdbarch_return_value (gdbarch, ppcnbsd_return_value);

> diff --git a/gdb/sh-nbsd-tdep.c b/gdb/sh-nbsd-tdep.c

> index aa319261684..2b2a7e3fd4a 100644

> --- a/gdb/sh-nbsd-tdep.c

> +++ b/gdb/sh-nbsd-tdep.c

> @@ -63,6 +63,7 @@ shnbsd_init_abi (struct gdbarch_info info,

>                    struct gdbarch *gdbarch)

>  {

>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

> +  nbsd_init_abi (info, gdbarch);

>

>    tdep->core_gregmap = (struct sh_corefile_regmap *)regmap;

>    tdep->sizeof_gregset = 84;

> diff --git a/gdb/sparc-nbsd-tdep.c b/gdb/sparc-nbsd-tdep.c

> index 7aba6020d27..ab1b557c57c 100644

> --- a/gdb/sparc-nbsd-tdep.c

> +++ b/gdb/sparc-nbsd-tdep.c

> @@ -296,6 +296,8 @@ sparc32nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>  {

>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

>

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* NetBSD doesn't support the 128-bit `long double' from the psABI.  */

>    set_gdbarch_long_double_bit (gdbarch, 64);

>    set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);

> diff --git a/gdb/sparc64-nbsd-tdep.c b/gdb/sparc64-nbsd-tdep.c

> index cd5bfe89410..dac7fa78b9b 100644

> --- a/gdb/sparc64-nbsd-tdep.c

> +++ b/gdb/sparc64-nbsd-tdep.c

> @@ -249,6 +249,8 @@ sparc64nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>  {

>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

>

> +  nbsd_init_abi (info, gdbarch);

> +

>    tdep->gregset = &sparc64nbsd_gregset;

>    tdep->sizeof_gregset = 160;

>

> diff --git a/gdb/vax-nbsd-tdep.c b/gdb/vax-nbsd-tdep.c

> index c2c08cc1603..7630ac5ab94 100644

> --- a/gdb/vax-nbsd-tdep.c

> +++ b/gdb/vax-nbsd-tdep.c

> @@ -29,6 +29,8 @@

>  static void

>  vaxnbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

>  {

> +  nbsd_init_abi (info, gdbarch);

> +

>    /* NetBSD ELF uses SVR4-style shared libraries.  */

>    set_solib_svr4_fetch_link_map_offsets

>      (gdbarch, svr4_ilp32_fetch_link_map_offsets);

> --

> 2.25.0

>
John Baldwin March 27, 2020, 4:31 p.m. | #2
On 3/26/20 4:26 PM, Kamil Rytarowski wrote:
> Ping?

> 

> On 20.03.2020 18:27, Kamil Rytarowski wrote:

>> Register nbsd_auxv_parse() that overloads the default (Linux-style)

>> AUXV parsing. On NetBSD the type parameter is defined as int32_t

>> for all architectures.


I would tone down some of the rhetoric.  FreeBSD uses the default AUXV
parsing, and I think Solaris does as well, so describing it as Linux-only
isn't very accurate.

(Similarly, I think the earlier reviews I saw around ptrace() claimed that
NetBSD was the only OS to use LWPs with ptrace() in the log messages in
effect which isn't really true as both Solaris and FreeBSD use LWPs
happily with ptrace(), just using a different convention.)

>>  }

>> +

>> +/* NetBSD-specific parser for AUXV data with. NetBSD follows the ELF

>> +   specification, contrary to some other ELF Operating Systems.  */


I would also tone this down a bit, and at least reference the correct
specification.  The ELF spec doesn't define the layout of auxv_t.  The
per-architecture psABI documents do.  Also, just saying that you follow
the spec doesn't help.  I would suggest something like:

/* NetBSD-specific parser for AUXV entries.  NetBSD always uses an int
   to store the type as defined in the SVR4 psABI specifications rather
   than long as assumed by the default parser.  */

-- 
John Baldwin
Kamil Rytarowski March 27, 2020, 5:04 p.m. | #3
On 27.03.2020 17:31, John Baldwin wrote:
> On 3/26/20 4:26 PM, Kamil Rytarowski wrote:

>> Ping?

>>

>> On 20.03.2020 18:27, Kamil Rytarowski wrote:

>>> Register nbsd_auxv_parse() that overloads the default (Linux-style)

>>> AUXV parsing. On NetBSD the type parameter is defined as int32_t

>>> for all architectures.

>

> I would tone down some of the rhetoric.  FreeBSD uses the default AUXV

> parsing, and I think Solaris does as well, so describing it as Linux-only

> isn't very accurate.

>


It is phrases as Linux-style, not Linux-only.

Linux-style is considered as the default one and other OSs have to tune it.

> (Similarly, I think the earlier reviews I saw around ptrace() claimed that

> NetBSD was the only OS to use LWPs with ptrace() in the log messages in

> effect which isn't really true as both Solaris and FreeBSD use LWPs

> happily with ptrace(), just using a different convention.)

>


The pair of (PID+LWP) for getters and setters of registers (among
certain other ptrace operations) is NetBSD style ptrace(2) API design.
This leads to the point that NetBSD is currently the only OS where
get_ptrace_pid() is not compatible (at leat in the current form).

>>>  }

>>> +

>>> +/* NetBSD-specific parser for AUXV data with. NetBSD follows the ELF

>>> +   specification, contrary to some other ELF Operating Systems.  */

>

> I would also tone this down a bit, and at least reference the correct

> specification.  The ELF spec doesn't define the layout of auxv_t.  The

> per-architecture psABI documents do.  Also, just saying that you follow

> the spec doesn't help.  I would suggest something like:

>

> /* NetBSD-specific parser for AUXV entries.  NetBSD always uses an int

>    to store the type as defined in the SVR4 psABI specifications rather

>    than long as assumed by the default parser.  */

>


This is toned down compared to obsd-tdet.c, that says:

/* Unlike Linux, OpenBSD actually follows the ELF standard.  */

OK, is this patch fine after rephrasing the above texts?
John Baldwin March 27, 2020, 7:22 p.m. | #4
On 3/27/20 10:04 AM, Kamil Rytarowski wrote:
> On 27.03.2020 17:31, John Baldwin wrote:

>> On 3/26/20 4:26 PM, Kamil Rytarowski wrote:

>>> Ping?

>>>

>>> On 20.03.2020 18:27, Kamil Rytarowski wrote:

>>>> Register nbsd_auxv_parse() that overloads the default (Linux-style)

>>>> AUXV parsing. On NetBSD the type parameter is defined as int32_t

>>>> for all architectures.

>>

>> I would tone down some of the rhetoric.  FreeBSD uses the default AUXV

>> parsing, and I think Solaris does as well, so describing it as Linux-only

>> isn't very accurate.

>>

> 

> It is phrases as Linux-style, not Linux-only.


I would still perhaps drop the paranthetical part as it reads that way
I think even with -style vs -only.

>> (Similarly, I think the earlier reviews I saw around ptrace() claimed that

>> NetBSD was the only OS to use LWPs with ptrace() in the log messages in

>> effect which isn't really true as both Solaris and FreeBSD use LWPs

>> happily with ptrace(), just using a different convention.)

>>

> 

> The pair of (PID+LWP) for getters and setters of registers (among

> certain other ptrace operations) is NetBSD style ptrace(2) API design.

> This leads to the point that NetBSD is currently the only OS where

> get_ptrace_pid() is not compatible (at leat in the current form).


It might be a bit of how it was said, the actual text:

<quote>
    Unlike most other Operating Systems, NetBSD tracks both pid and lwp.
    The process id on NetBSD is stored always in the pid field of ptid.
</quote>

If by "track" you mean "the kernel keeps track of LWPs as distinct
from processes" (which is how I read "track"), then I think it is
inaccurate.


>> I would also tone this down a bit, and at least reference the correct

>> specification.  The ELF spec doesn't define the layout of auxv_t.  The

>> per-architecture psABI documents do.  Also, just saying that you follow

>> the spec doesn't help.  I would suggest something like:

>>

>> /* NetBSD-specific parser for AUXV entries.  NetBSD always uses an int

>>    to store the type as defined in the SVR4 psABI specifications rather

>>    than long as assumed by the default parser.  */

>>

> 

> This is toned down compared to obsd-tdet.c, that says:

> 

> /* Unlike Linux, OpenBSD actually follows the ELF standard.  */


That may be, but that probably isn't who I would choose as a model to
follow.

> OK, is this patch fine after rephrasing the above texts?


It looks fine to me, but it probably needs an approver such as Tom to ok it.
(I can approve FreeBSD-related things, but not really other bits.)

-- 
John Baldwin
Kamil Rytarowski March 29, 2020, 8:35 p.m. | #5
On 27.03.2020 20:22, John Baldwin wrote:
> On 3/27/20 10:04 AM, Kamil Rytarowski wrote:

>> On 27.03.2020 17:31, John Baldwin wrote:

>>> On 3/26/20 4:26 PM, Kamil Rytarowski wrote:

>>>> Ping?

>>>>

>>>> On 20.03.2020 18:27, Kamil Rytarowski wrote:

>>>>> Register nbsd_auxv_parse() that overloads the default (Linux-style)

>>>>> AUXV parsing. On NetBSD the type parameter is defined as int32_t

>>>>> for all architectures.

>>>

>>> I would tone down some of the rhetoric.  FreeBSD uses the default AUXV

>>> parsing, and I think Solaris does as well, so describing it as Linux-only

>>> isn't very accurate.

>>>

>>

>> It is phrases as Linux-style, not Linux-only.

>

> I would still perhaps drop the paranthetical part as it reads that way

> I think even with -style vs -only.

>

>>> (Similarly, I think the earlier reviews I saw around ptrace() claimed that

>>> NetBSD was the only OS to use LWPs with ptrace() in the log messages in

>>> effect which isn't really true as both Solaris and FreeBSD use LWPs

>>> happily with ptrace(), just using a different convention.)

>>>

>>

>> The pair of (PID+LWP) for getters and setters of registers (among

>> certain other ptrace operations) is NetBSD style ptrace(2) API design.

>> This leads to the point that NetBSD is currently the only OS where

>> get_ptrace_pid() is not compatible (at leat in the current form).

>

> It might be a bit of how it was said, the actual text:

>

> <quote>

>     Unlike most other Operating Systems, NetBSD tracks both pid and lwp.

>     The process id on NetBSD is stored always in the pid field of ptid.

> </quote>

>

> If by "track" you mean "the kernel keeps track of LWPs as distinct

> from processes" (which is how I read "track"), then I think it is

> inaccurate.

>

>

>>> I would also tone this down a bit, and at least reference the correct

>>> specification.  The ELF spec doesn't define the layout of auxv_t.  The

>>> per-architecture psABI documents do.  Also, just saying that you follow

>>> the spec doesn't help.  I would suggest something like:

>>>

>>> /* NetBSD-specific parser for AUXV entries.  NetBSD always uses an int

>>>    to store the type as defined in the SVR4 psABI specifications rather

>>>    than long as assumed by the default parser.  */

>>>

>>

>> This is toned down compared to obsd-tdet.c, that says:

>>

>> /* Unlike Linux, OpenBSD actually follows the ELF standard.  */

>

> That may be, but that probably isn't who I would choose as a model to

> follow.

>

>> OK, is this patch fine after rephrasing the above texts?

>

> It looks fine to me, but it probably needs an approver such as Tom to ok it.

> (I can approve FreeBSD-related things, but not really other bits.)

>


OK. I will rephrase the texts as I understand that the wording is sensitive.
Simon Marchi March 29, 2020, 10:10 p.m. | #6
On 2020-03-20 1:27 p.m., Kamil Rytarowski wrote:
> @@ -47,3 +48,40 @@ nbsd_pc_in_sigtramp (CORE_ADDR pc, const char *func_name)

>    return (func_name != NULL

>  	  && startswith (func_name, "__sigtramp"));

>  }

> +

> +/* NetBSD-specific parser for AUXV data with. NetBSD follows the ELF

> +   specification, contrary to some other ELF Operating Systems.  */

> +

> +static int

> +nbsd_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr,

> +		 gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)

> +{

> +  struct type *int_type = builtin_type (gdbarch)->builtin_int;

> +  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;

> +  const int sizeof_auxv_type = TYPE_LENGTH (int_type);

> +  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 < 2 * sizeof_auxv_val)

> +    return -1;

> +

> +  *typep = extract_unsigned_integer (ptr, sizeof_auxv_type, byte_order);

> +  ptr += sizeof_auxv_val;	/* Alignment.  */

> +  *valp = extract_unsigned_integer (ptr, sizeof_auxv_val, byte_order);

> +  ptr += sizeof_auxv_val;


From this code, I understand that on AMD64/NetBSD, an auxv entry looks like
(each character is a byte):

T T T T P P P P V V V V V V V V

Where T is the type value, P is padding and V is value.  Is that right?

> +

> +  *readptr = ptr;

> +  return 1;

> +}


Instead of defining this function that is very similar to default_auxv_parse, I would
strongly suggest making a parametrized version of default_auxv_parse (and make
default_auxv_parse use it).

Simon

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 650b74bae4a..572403001ce 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@ 
+2020-03-20  Kamil Rytarowski  <n54@gmx.com>
+
+	* nbsd-tdep.c: Include "gdbarch.h".
+	* nbsd-tdep.c (nbsd_auxv_parse, nbsd_init_abi): New.
+	* alpha-nbsd-tdep.c (alphanbsd_init_abi): Call nbsd_init_abi().
+	* amd64-nbsd-tdep.c (amd64nbsd_init_abi): Likewise.
+	* arm-nbsd-tdep.c (arm_netbsd_elf_init_abi): Likewise.
+	* hppa-nbsd-tdep.c (hppanbsd_init_abi): Likewise.
+	* i386-nbsd-tdep.c (i386nbsd_init_abi): Likewise.
+	* mips-nbsd-tdep.c (nbsd_init_abi): Likewise.
+	* ppc-nbsd-tdep.c (ppcnbsd_init_abi): Likewise.
+	* sh-nbsd-tdep.c (shnbsd_init_abi): Likewise.
+	* sparc-nbsd-tdep.c (sparc32nbsd_init_abi): Likewise.
+	* sparc64-nbsd-tdep.c (sparc64nbsd_init_abi): Likewise.
+	* vax-nbsd-tdep.c (vaxnbsd_elf_init_abi): Likewise.
+
 2020-03-20  Kamil Rytarowski  <n54@gmx.com>

 	* amd64-bsd-nat.c (gdb_ptrace): Change return type from `int' to
diff --git a/gdb/alpha-nbsd-tdep.c b/gdb/alpha-nbsd-tdep.c
index ab9240e35da..58294edb3f6 100644
--- a/gdb/alpha-nbsd-tdep.c
+++ b/gdb/alpha-nbsd-tdep.c
@@ -258,6 +258,8 @@  alphanbsd_init_abi (struct gdbarch_info info,
   /* Hook into the MDEBUG frame unwinder.  */
   alpha_mdebug_init_abi (info, gdbarch);

+  nbsd_init_abi (info, gdbarch);
+
   /* NetBSD/alpha does not provide single step support via ptrace(2); we
      must use software single-stepping.  */
   set_gdbarch_software_single_step (gdbarch, alpha_software_single_step);
diff --git a/gdb/amd64-nbsd-tdep.c b/gdb/amd64-nbsd-tdep.c
index 89d07236b85..59071488ed7 100644
--- a/gdb/amd64-nbsd-tdep.c
+++ b/gdb/amd64-nbsd-tdep.c
@@ -106,6 +106,7 @@  amd64nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)

   amd64_init_abi (info, gdbarch,
 		  amd64_target_description (X86_XSTATE_SSE_MASK, true));
+  nbsd_init_abi (info, gdbarch);

   tdep->jb_pc_offset = 7 * 8;

diff --git a/gdb/arm-nbsd-tdep.c b/gdb/arm-nbsd-tdep.c
index e01df50bc25..a6104f760b3 100644
--- a/gdb/arm-nbsd-tdep.c
+++ b/gdb/arm-nbsd-tdep.c
@@ -150,6 +150,9 @@  arm_netbsd_elf_init_abi (struct gdbarch_info info,
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

   arm_netbsd_init_abi_common (info, gdbarch);
+
+  nbsd_init_abi (info, gdbarch);
+
   if (tdep->fp_model == ARM_FLOAT_AUTO)
     tdep->fp_model = ARM_FLOAT_SOFT_VFP;

diff --git a/gdb/hppa-nbsd-tdep.c b/gdb/hppa-nbsd-tdep.c
index b532ab1d5cc..d601aa96f3f 100644
--- a/gdb/hppa-nbsd-tdep.c
+++ b/gdb/hppa-nbsd-tdep.c
@@ -201,6 +201,8 @@  hppanbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* Obviously NetBSD is BSD-based.  */
   hppabsd_init_abi (info, gdbarch);

+  nbsd_init_abi (info, gdbarch);
+
   /* Core file support.  */
   set_gdbarch_iterate_over_regset_sections
     (gdbarch, hppanbsd_iterate_over_regset_sections);
diff --git a/gdb/i386-nbsd-tdep.c b/gdb/i386-nbsd-tdep.c
index 3157451e08f..f350412d9bd 100644
--- a/gdb/i386-nbsd-tdep.c
+++ b/gdb/i386-nbsd-tdep.c
@@ -377,6 +377,8 @@  i386nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* Obviously NetBSD is BSD-based.  */
   i386bsd_init_abi (info, gdbarch);

+  nbsd_init_abi (info, gdbarch);
+
   /* NetBSD has a different `struct reg'.  */
   tdep->gregset_reg_offset = i386nbsd_r_reg_offset;
   tdep->gregset_num_regs = ARRAY_SIZE (i386nbsd_r_reg_offset);
diff --git a/gdb/mips-nbsd-tdep.c b/gdb/mips-nbsd-tdep.c
index 38bc7ce636b..6f4d22b24fb 100644
--- a/gdb/mips-nbsd-tdep.c
+++ b/gdb/mips-nbsd-tdep.c
@@ -354,6 +354,8 @@  static void
 mipsnbsd_init_abi (struct gdbarch_info info,
                    struct gdbarch *gdbarch)
 {
+  nbsd_init_abi (info, gdbarch)
+
   set_gdbarch_iterate_over_regset_sections
     (gdbarch, mipsnbsd_iterate_over_regset_sections);

diff --git a/gdb/nbsd-tdep.c b/gdb/nbsd-tdep.c
index 49bb2b706bd..5d5ea26622e 100644
--- a/gdb/nbsd-tdep.c
+++ b/gdb/nbsd-tdep.c
@@ -22,6 +22,7 @@ 
 #include "defs.h"
 #include "solib-svr4.h"
 #include "nbsd-tdep.h"
+#include "gdbarch.h"

 /* FIXME: kettenis/20060115: We should really eliminate the next two
    functions completely.  */
@@ -47,3 +48,40 @@  nbsd_pc_in_sigtramp (CORE_ADDR pc, const char *func_name)
   return (func_name != NULL
 	  && startswith (func_name, "__sigtramp"));
 }
+
+/* NetBSD-specific parser for AUXV data with. NetBSD follows the ELF
+   specification, contrary to some other ELF Operating Systems.  */
+
+static int
+nbsd_auxv_parse (struct gdbarch *gdbarch, gdb_byte **readptr,
+		 gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
+{
+  struct type *int_type = builtin_type (gdbarch)->builtin_int;
+  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
+  const int sizeof_auxv_type = TYPE_LENGTH (int_type);
+  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 < 2 * sizeof_auxv_val)
+    return -1;
+
+  *typep = extract_unsigned_integer (ptr, sizeof_auxv_type, byte_order);
+  ptr += sizeof_auxv_val;	/* Alignment.  */
+  *valp = extract_unsigned_integer (ptr, sizeof_auxv_val, byte_order);
+  ptr += sizeof_auxv_val;
+
+  *readptr = ptr;
+  return 1;
+}
+
+/* See nbsd-tdep.h.  */
+
+void
+nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
+{
+  set_gdbarch_auxv_parse (gdbarch, nbsd_auxv_parse);
+}
diff --git a/gdb/nbsd-tdep.h b/gdb/nbsd-tdep.h
index c99a8b537b6..4b06c13f87b 100644
--- a/gdb/nbsd-tdep.h
+++ b/gdb/nbsd-tdep.h
@@ -25,4 +25,8 @@  struct link_map_offsets *nbsd_lp64_solib_svr4_fetch_link_map_offsets (void);

 int nbsd_pc_in_sigtramp (CORE_ADDR, const char *);

+/* NetBSD specific set of ABI-related routines.  */
+
+void nbsd_init_abi (struct gdbarch_info, struct gdbarch *);
+
 #endif /* NBSD_TDEP_H */
diff --git a/gdb/ppc-nbsd-tdep.c b/gdb/ppc-nbsd-tdep.c
index d75930c9f8d..81492deaccd 100644
--- a/gdb/ppc-nbsd-tdep.c
+++ b/gdb/ppc-nbsd-tdep.c
@@ -173,6 +173,8 @@  static void
 ppcnbsd_init_abi (struct gdbarch_info info,
                   struct gdbarch *gdbarch)
 {
+  nbsd_init_abi (info, gdbarch);
+
   /* For NetBSD, this is an on again, off again thing.  Some systems
      do use the broken struct convention, and some don't.  */
   set_gdbarch_return_value (gdbarch, ppcnbsd_return_value);
diff --git a/gdb/sh-nbsd-tdep.c b/gdb/sh-nbsd-tdep.c
index aa319261684..2b2a7e3fd4a 100644
--- a/gdb/sh-nbsd-tdep.c
+++ b/gdb/sh-nbsd-tdep.c
@@ -63,6 +63,7 @@  shnbsd_init_abi (struct gdbarch_info info,
                   struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  nbsd_init_abi (info, gdbarch);

   tdep->core_gregmap = (struct sh_corefile_regmap *)regmap;
   tdep->sizeof_gregset = 84;
diff --git a/gdb/sparc-nbsd-tdep.c b/gdb/sparc-nbsd-tdep.c
index 7aba6020d27..ab1b557c57c 100644
--- a/gdb/sparc-nbsd-tdep.c
+++ b/gdb/sparc-nbsd-tdep.c
@@ -296,6 +296,8 @@  sparc32nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

+  nbsd_init_abi (info, gdbarch);
+
   /* NetBSD doesn't support the 128-bit `long double' from the psABI.  */
   set_gdbarch_long_double_bit (gdbarch, 64);
   set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);
diff --git a/gdb/sparc64-nbsd-tdep.c b/gdb/sparc64-nbsd-tdep.c
index cd5bfe89410..dac7fa78b9b 100644
--- a/gdb/sparc64-nbsd-tdep.c
+++ b/gdb/sparc64-nbsd-tdep.c
@@ -249,6 +249,8 @@  sparc64nbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

+  nbsd_init_abi (info, gdbarch);
+
   tdep->gregset = &sparc64nbsd_gregset;
   tdep->sizeof_gregset = 160;

diff --git a/gdb/vax-nbsd-tdep.c b/gdb/vax-nbsd-tdep.c
index c2c08cc1603..7630ac5ab94 100644
--- a/gdb/vax-nbsd-tdep.c
+++ b/gdb/vax-nbsd-tdep.c
@@ -29,6 +29,8 @@ 
 static void
 vaxnbsd_elf_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
+  nbsd_init_abi (info, gdbarch);
+
   /* NetBSD ELF uses SVR4-style shared libraries.  */
   set_solib_svr4_fetch_link_map_offsets
     (gdbarch, svr4_ilp32_fetch_link_map_offsets);