Re: [PATCH 2/9] Support fs_base and gs_base on FreeBSD/i386.

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2019-01-22 13:42, John Baldwin wrote:
> The i386 BSD native target uses the same ptrace operations
> (PT_[GS]ET[FG]SBASE) as the amd64 BSD native target to fetch and store
> the registers.
>
> The amd64 BSD native now uses 'tdep->fsbase_regnum' instead of
> hardcoding AMD64_FSBASE_REGNUM and AMD64_GSBASE_REGNUM to support
> 32-bit targets.  In addition, the store operations explicitly zero the
> new register value before fetching it from the register cache to
> ensure 32-bit values are zero-extended.

To be clear, this happens when debugging a 32-bits process on a 64-bits 
OS?  When debugging a 32-bits process on a 32-bits OS, the code in 
i386-bsd-nat.c would be used?

> gdb/ChangeLog:
>
> 	* amd64-bsd-nat.c (amd64bsd_fetch_inferior_registers): Use
> 	tdep->fsbase_regnum instead of constants for fs_base and gs_base.
> 	(amd64bsd_store_inferior_registers): Likewise.
> 	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
> 	Enable segment base registers.
> 	* i386-bsd-nat.c (i386bsd_fetch_inferior_registers): Use
> 	PT_GETFSBASE and PT_GETGSBASE.
> 	(i386bsd_store_inferior_registers): Use PT_SETFSBASE and PT_SETGSBASE.
> 	* i386-fbsd-nat.c (i386_fbsd_nat_target::read_description): Enable
> 	segment base registers.
> 	* i386-fbsd-tdep.c (i386fbsd_core_read_description): Likewise.
> ---
>  gdb/ChangeLog        | 14 ++++++++++++
>  gdb/amd64-bsd-nat.c  | 26 ++++++++++++++-------
>  gdb/amd64-fbsd-nat.c |  4 ++--
>  gdb/i386-bsd-nat.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/i386-fbsd-nat.c  |  2 +-
>  gdb/i386-fbsd-tdep.c |  2 +-
>  6 files changed, 90 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4afd5b664e..056a60fa23 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,17 @@
> +2019-01-22  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* amd64-bsd-nat.c (amd64bsd_fetch_inferior_registers): Use
> +	tdep->fsbase_regnum instead of constants for fs_base and gs_base.
> +	(amd64bsd_store_inferior_registers): Likewise.
> +	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
> +	Enable segment base registers.
> +	* i386-bsd-nat.c (i386bsd_fetch_inferior_registers): Use
> +	PT_GETFSBASE and PT_GETGSBASE.
> +	(i386bsd_store_inferior_registers): Use PT_SETFSBASE and 
> PT_SETGSBASE.
> +	* i386-fbsd-nat.c (i386_fbsd_nat_target::read_description): Enable
> +	segment base registers.
> +	* i386-fbsd-tdep.c (i386fbsd_core_read_description): Likewise.
> +
>  2019-01-22  John Baldwin  <jhb@FreeBSD.org>
>
>  	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
> diff --git a/gdb/amd64-bsd-nat.c b/gdb/amd64-bsd-nat.c
> index a2a91abb91..0f47ff6c61 100644
> --- a/gdb/amd64-bsd-nat.c
> +++ b/gdb/amd64-bsd-nat.c
> @@ -43,6 +43,9 @@ amd64bsd_fetch_inferior_registers (struct regcache
> *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    pid_t pid = get_ptrace_pid (regcache->ptid ());
> +#ifdef PT_GETFSBASE
> +  const struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +#endif

TDEP is used in both #ifdef PT_GETFSBASE and #ifdef PT_GETGSBASE, but is 
only declared here in an #ifdef PT_GETFSBASE.  I suppose it's not 
actually an issue because they will always be present together.  But we 
might as well use "#if defined(PT_GETFSBASE) || defined(PT_GETGSBASE)" 
for the declaration.

>    if (regnum == -1 || amd64_native_gregset_supplies_p (gdbarch, 
> regnum))
>      {
> @@ -57,27 +60,27 @@ amd64bsd_fetch_inferior_registers (struct regcache
> *regcache, int regnum)
>      }
>
>  #ifdef PT_GETFSBASE
> -  if (regnum == -1 || regnum == AMD64_FSBASE_REGNUM)
> +  if (regnum == -1 || regnum == tdep->fsbase_regnum)
>      {
>        register_t base;
>
>        if (ptrace (PT_GETFSBASE, pid, (PTRACE_TYPE_ARG3) &base, 0) == 
> -1)
>  	perror_with_name (_("Couldn't get segment register fs_base"));
>
> -      regcache->raw_supply (AMD64_FSBASE_REGNUM, &base);
> +      regcache->raw_supply (tdep->fsbase_regnum, &base);
>        if (regnum != -1)
>  	return;
>      }
>  #endif
>  #ifdef PT_GETGSBASE
> -  if (regnum == -1 || regnum == AMD64_GSBASE_REGNUM)
> +  if (regnum == -1 || regnum == tdep->fsbase_regnum + 1)
>      {
>        register_t base;
>
>        if (ptrace (PT_GETGSBASE, pid, (PTRACE_TYPE_ARG3) &base, 0) == 
> -1)
>  	perror_with_name (_("Couldn't get segment register gs_base"));
>
> -      regcache->raw_supply (AMD64_GSBASE_REGNUM, &base);
> +      regcache->raw_supply (tdep->fsbase_regnum + 1, &base);
>        if (regnum != -1)
>  	return;
>      }
> @@ -116,6 +119,9 @@ amd64bsd_store_inferior_registers (struct regcache
> *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    pid_t pid = get_ptrace_pid (regcache->ptid ());
> +#ifdef PT_GETFSBASE
> +  const struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +#endif

Same here.

>    if (regnum == -1 || amd64_native_gregset_supplies_p (gdbarch, 
> regnum))
>      {
> @@ -134,11 +140,13 @@ amd64bsd_store_inferior_registers (struct
> regcache *regcache, int regnum)
>      }
>
>  #ifdef PT_SETFSBASE
> -  if (regnum == -1 || regnum == AMD64_FSBASE_REGNUM)
> +  if (regnum == -1 || regnum == tdep->fsbase_regnum)
>      {
>        register_t base;
>
> -      regcache->raw_collect (AMD64_FSBASE_REGNUM, &base);
> +      /* Clear the full base value to support 32-bit targets.  */
> +      base = 0;
> +      regcache->raw_collect (tdep->fsbase_regnum, &base);

It's probably safer to clear the value to 0 as you did, so that's fine.  
But I would have thought that when debugging 32-bits processes, the high 
bits would be ignored at some point.  The kernel would know that this is 
a 32 bits register, so it would just take the 32 low bits of what it 
receives, and it magically works whatever the original data type is 
because it's little endian.

Otherwise, this LGTM.

Simon