[PATCH V2 1/1] amd64-linux: expose system register FS_BASE and GS_BASE for Linux.

Pedro Alves palves@redhat.com
Thu Jan 19 19:45:00 GMT 2017


Hi Walfred,

On 01/05/2017 03:53 PM, Walfred Tedeschi wrote:

> orig_rax is the last register added there is a special macro that defines
> it in this way. On gdbserver side it is defined in the middle of
> the registers and stays there always for compatibility.

For compatibility with what?

(I think some punctuation in the first sentence would make it
much easier to grok.)

> I have added the two new registers just after the orig_rax.  This causes
> that they will be in the gdbserver side just after orig_rax and in gdb

in gdb [...] ?

> 
> I still can add then where I would prefer, the definition is
> done by the macros defining AMD64_FSBASE_REGNUM. 

I don't understand what you mean.  What would you prefer?

> In this sense they have
> been added just before the orig_rax.
> 
> This decision was taken based on the fact that we would like to add
> hardware registers as usual before the orig_rax, and with this addition
> also before the fs_base and gs_base.

But the gdb register numbers have no bearing on what the user
sees?  It's the order in the target description that
matters, isn't it?

Maybe what you're saying is this:

> --- a/gdb/amd64-linux-tdep.h
> +++ b/gdb/amd64-linux-tdep.h
> @@ -26,7 +26,7 @@
>  /* Register number for the "orig_rax" register.  If this register
>     contains a value >= 0 it is interpreted as the system call number
>     that the kernel is supposed to restart.  */
> -#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_ZMM31H_REGNUM + 1)
> +#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_NUM_REGS)

... AFAICS, AMD64_LINUX_ORIG_RAX_REGNUM is defined as the
register that follows the last register defined in 
amd64-tdep.h, which is always AMD64_NUM_REGS.  I.e., first
we define the "common" registers, and then the
Linux-specific "virtual" registers.

So this this change above makes it simpler to add new
amd64-generic registers without having to remember to
change amd64-linux-tdep.h.  Right?  If that was split into
a small preparatory patch with a rationale like that, it'd
have been much clearer, I think.

> 
> 
> 2017-01-05  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 	    Richard Henderson  <rth@redhat.com>
> 
> gdb/ChangeLog:
> 
> 	* amd64-linux-nat.c (PTRACE_ARCH_PRCTL): New define.
> 	(amd64_linux_fetch_inferior_registers): Add case to fetch FS_BASE
> 	GS_BASE for older kernels.
> 	(amd64_linux_store_inferior_registers): Add case to store FS_BASE
> 	GS_BASE for older kernels.
> 	* amd64-linux-tdep.c (amd64_linux_gregset_reg_offset): Add FS_BASE
> 	and GS_BASE to the offset table.
> 	(amd64_linux_register_reggroup_p): Add FS_BASE and GS_BASE to the
> 	system register group.
> 	* amd64-linux-tdep.h (AMD64_LINUX_ORIG_RAX_REGNUM): Set the value
> 	to AMD64_NUM_REGS.
> 	* amd64-nat.c (amd64_native_gregset_reg_offset): Adds valid_regnum
> 	to test validit of the register.  Implements case for older
> 	kernels.
> 	* amd64-tdep.c (amd64_init_abi): Add segment registers for the
> 	amd64 ABI.
> 	* amd64-tdep.h (amd64_regnum): Add AMD64_FSBASE_REGNUM and
> 	AMD64_GSBASE_REGNUM.
> 	(AMD64_NUM_REGS): Set to AMD64_GSBASE_REGNUM + 1.
> 	* features/Makefile (amd64-linux.dat, amd64-avx-linux.dat)
> 	(amd64-mpx-linux.dat, amd64-avx512-linux.dat, x32-linux.dat)
> 	(x32-avx-linux.dat, x32-avx512-linux.dat): Add
> 	i386/64bit-segments.xml in those rules.
> 	* features/i386/64bit-segments.xml: New file.
> 	* features/i386/amd64-avx-mpx-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/amd64-avx-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/amd64-avx512-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/amd64-mpx-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/x32-avx512-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/x32-avx-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/amd64-linux.xml: Add 64bit-segments.xml.
> 	* features/i386/amd64-avx-linux.c: Regenerated.
> 	* features/i386/amd64-avx-mpx-linux.c: Regenerated.
> 	* features/i386/amd64-avx-mpx.c: Regenerated.
> 	* features/i386/amd64-avx512-linux.c: Regenerated.
> 	* features/i386/amd64-linux.c: Regenerated.
> 	* features/i386/amd64-mpx-linux.c: Regenerated.
> 	* features/i386/i386-avx-mpx-linux.c: Regenerated.
> 	* features/i386/i386-avx-mpx.c: Regenerated.
> 	* features/i386/x32-avx-linux.c: Regenerated.
> 	* features/i386/x32-avx512-linux.c: Regenerated.
> 	* regformats/i386/amd64-avx-linux.dat: Regenerated.
> 	* regformats/i386/amd64-avx-mpx-linux.dat: Regenerated.
> 	* regformats/i386/amd64-avx512-linux.dat: Regenerated.
> 	* regformats/i386/amd64-linux.dat: Regenerated.
> 	* regformats/i386/amd64-mpx-linux.dat: Regenerated.
> 	* regformats/i386/x32-avx-linux.dat: Regenerated.
> 	* regformats/i386/x32-avx512-linux.dat: Regenerated.
> 	* regformats/i386/x32-linux.dat: Regenerated.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (i386 Features): Add system segment registers
> 	as feature.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* linux-x86-low.c (x86_64_regmap): Add fs_base and gs_base
> 	to the register table.
> 	(x86_fill_gregset): Add support for old kernels for the
> 	fs_base and gs_base system registers.
> 	(x86_store_gregset): Likewise.
> 	* configure.srv (srv_i386_64bit_xmlfiles): Add 64bit-segments.xml.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.arch/amd64-gs_base.c: New file.
> 	* gdb.arch/amd64-gs_base.exp: New file.
> 
> ---
>  gdb/amd64-linux-nat.c                       |  53 ++++++++
>  gdb/amd64-linux-tdep.c                      |   7 +-
>  gdb/amd64-linux-tdep.h                      |   2 +-
>  gdb/amd64-nat.c                             |  19 ++-
>  gdb/amd64-tdep.c                            |  13 ++
>  gdb/amd64-tdep.h                            |   6 +-
>  gdb/doc/gdb.texinfo                         |   3 +
>  gdb/features/Makefile                       |  17 +--
>  gdb/features/i386/64bit-segments.xml        |  12 ++
>  gdb/features/i386/amd64-avx-linux.c         |  36 +++---
>  gdb/features/i386/amd64-avx-linux.xml       |   1 +
>  gdb/features/i386/amd64-avx-mpx-linux.c     |  48 +++----
>  gdb/features/i386/amd64-avx-mpx-linux.xml   |   1 +
>  gdb/features/i386/amd64-avx512-linux.c      | 192 ++++++++++++++--------------
>  gdb/features/i386/amd64-avx512-linux.xml    |   1 +
>  gdb/features/i386/amd64-linux.c             |   4 +
>  gdb/features/i386/amd64-linux.xml           |   1 +
>  gdb/features/i386/amd64-mpx-linux.c         |  16 ++-
>  gdb/features/i386/amd64-mpx-linux.xml       |   1 +
>  gdb/features/i386/x32-avx-linux.c           |  36 +++---
>  gdb/features/i386/x32-avx-linux.xml         |   1 +
>  gdb/features/i386/x32-avx512-linux.c        | 192 ++++++++++++++--------------
>  gdb/features/i386/x32-avx512-linux.xml      |   1 +
>  gdb/features/i386/x32-linux.c               |   4 +
>  gdb/features/i386/x32-linux.xml             |   1 +
>  gdb/gdbserver/configure.srv                 |   2 +-
>  gdb/gdbserver/linux-x86-low.c               |  32 +++++
>  gdb/regformats/i386/amd64-avx-linux.dat     |   2 +
>  gdb/regformats/i386/amd64-avx-mpx-linux.dat |   2 +
>  gdb/regformats/i386/amd64-avx512-linux.dat  |   2 +
>  gdb/regformats/i386/amd64-linux.dat         |   2 +
>  gdb/regformats/i386/amd64-mpx-linux.dat     |   2 +
>  gdb/regformats/i386/x32-avx-linux.dat       |   2 +
>  gdb/regformats/i386/x32-avx512-linux.dat    |   2 +
>  gdb/regformats/i386/x32-linux.dat           |   2 +
>  gdb/testsuite/gdb.arch/amd64-gs_base.c      |  26 ++++
>  gdb/testsuite/gdb.arch/amd64-gs_base.exp    |  51 ++++++++
>  37 files changed, 531 insertions(+), 264 deletions(-)
>  create mode 100644 gdb/features/i386/64bit-segments.xml
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-gs_base.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-gs_base.exp
> 
> diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
> index 4a2036b..0ec22cb 100644
> --- a/gdb/amd64-linux-nat.c
> +++ b/gdb/amd64-linux-nat.c
> @@ -40,6 +40,11 @@
>  #include "nat/linux-ptrace.h"
>  #include "nat/amd64-linux-siginfo.h"
>  
> +/* This definition comes from prctl.h, but some kernels may not have it.  */
> +#ifndef PTRACE_ARCH_PRCTL
> +#define PTRACE_ARCH_PRCTL      30
> +#endif
> +
>  /* Mapping between the general-purpose registers in GNU/Linux x86-64
>     `struct user' format and GDB's register cache layout for GNU/Linux
>     i386.
> @@ -171,6 +176,30 @@ amd64_linux_fetch_inferior_registers (struct target_ops *ops,
>  
>  	  amd64_supply_fxsave (regcache, -1, &fpregs);
>  	}
> +#ifndef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
> +      {
> +	/* PTRACE_ARCH_PRCTL is obsolete since 2.6.25, where the
> +	   fs_base and gs_base fields of user_regs_struct can be
> +	   used directly.  */
> +	unsigned long base;
> +
> +	if (regnum == -1 || regnum == AMD64_FSBASE_REGNUM)
> +	  {
> +	    if (ptrace (PTRACE_ARCH_PRCTL, tid, &base, ARCH_GET_FS) < 0)
> +	      perror_with_name (_("Couldn't get segment register fs_base"));
> +
> +	    regcache_raw_supply (regcache, AMD64_FSBASE_REGNUM, &base);
> +	  }
> +
> +	if (regnum == -1 || regnum == AMD64_GSBASE_REGNUM)
> +	  {
> +	    if (ptrace (PTRACE_ARCH_PRCTL, tid, &base, ARCH_GET_GS) < 0)
> +	      perror_with_name (_("Couldn't get segment register gs_base"));
> +
> +	    regcache_raw_supply (regcache, AMD64_GSBASE_REGNUM, &base);
> +	  }
> +      }
> +#endif
>      }
>  }
>  
> @@ -237,6 +266,30 @@ amd64_linux_store_inferior_registers (struct target_ops *ops,
>  	  if (ptrace (PTRACE_SETFPREGS, tid, 0, (long) &fpregs) < 0)
>  	    perror_with_name (_("Couldn't write floating point status"));
>  	}
> +
> +#ifndef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
> +      {
> +	/* PTRACE_ARCH_PRCTL is obsolete since 2.6.25, where the
> +	   fs_base and gs_base fields of user_regs_struct can be
> +	   used directly.  */
> +	void *base;
> +
> +	if (regnum == -1 || regnum == AMD64_FSBASE_REGNUM)
> +	  {
> +	    regcache_raw_collect (regcache, AMD64_FSBASE_REGNUM, &base);
> +
> +	    if (ptrace (PTRACE_ARCH_PRCTL, tid, base, ARCH_SET_FS) < 0)
> +	      perror_with_name (_("Couldn't write fs_base"));
> +	  }
> +	if (regnum == -1 || regnum == AMD64_GSBASE_REGNUM)
> +	  {
> +
> +	    regcache_raw_collect (regcache, AMD64_GSBASE_REGNUM, &base);
> +	    if (ptrace (PTRACE_ARCH_PRCTL, tid, base, ARCH_SET_GS) < 0)
> +	      perror_with_name (_("Couldn't write gs_base"));
> +	  }
> +      }
> +#endif
>      }
>  }
>  

> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 1e9f75f..b9d7176 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -103,6 +103,9 @@ int amd64_linux_gregset_reg_offset[] =
>    -1, -1, -1, -1, -1, -1, -1, -1,
>    -1, -1, -1, -1, -1, -1, -1, -1,
>    -1, -1, -1, -1, -1, -1, -1, -1,
> +
> +  /* End of hardware registers */
> +  21 * 8, 22 * 8,		      /* fs_base and gs_base.  */
>    15 * 8			      /* "orig_rax" */
>  };
>  

> @@ -284,7 +287,9 @@ static int
>  amd64_linux_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
>  				 struct reggroup *group)
>  { 
> -  if (regnum == AMD64_LINUX_ORIG_RAX_REGNUM)
> +  if (regnum == AMD64_LINUX_ORIG_RAX_REGNUM
> +      || regnum == AMD64_FSBASE_REGNUM
> +      || regnum == AMD64_GSBASE_REGNUM)
>      return (group == system_reggroup
>              || group == save_reggroup
>              || group == restore_reggroup);
> diff --git a/gdb/amd64-linux-tdep.h b/gdb/amd64-linux-tdep.h
> index 5d032a6..3704caa 100644
> --- a/gdb/amd64-linux-tdep.h
> +++ b/gdb/amd64-linux-tdep.h
> @@ -26,7 +26,7 @@
>  /* Register number for the "orig_rax" register.  If this register
>     contains a value >= 0 it is interpreted as the system call number
>     that the kernel is supposed to restart.  */
> -#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_ZMM31H_REGNUM + 1)
> +#define AMD64_LINUX_ORIG_RAX_REGNUM (AMD64_NUM_REGS)
>  
>  /* Total number of registers for GNU/Linux.  */
>  #define AMD64_LINUX_NUM_REGS (AMD64_LINUX_ORIG_RAX_REGNUM + 1)
> diff --git a/gdb/amd64-nat.c b/gdb/amd64-nat.c
> index 18c8a99..0143632 100644
> --- a/gdb/amd64-nat.c
> +++ b/gdb/amd64-nat.c
> @@ -53,6 +53,7 @@ amd64_native_gregset_reg_offset (struct gdbarch *gdbarch, int regnum)
>  {
>    int *reg_offset = amd64_native_gregset64_reg_offset;
>    int num_regs = amd64_native_gregset64_num_regs;
> +  bool valid_regnum = false;
>  
>    gdb_assert (regnum >= 0);
>  
> @@ -65,10 +66,22 @@ amd64_native_gregset_reg_offset (struct gdbarch *gdbarch, int regnum)
>    if (num_regs > gdbarch_num_regs (gdbarch))
>      num_regs = gdbarch_num_regs (gdbarch);
>  
> -  if (regnum < num_regs && regnum < gdbarch_num_regs (gdbarch))
> -    return reg_offset[regnum];
> +  /* Looks like we do not need the second part of the &&.
> +     Register validity is already guaranteed in the first part due to the
> +     comparison above.  */

Agreed.  The best would be to send a small preparatory patch that
fixed that.

> +  valid_regnum = regnum < num_regs && regnum < gdbarch_num_regs (gdbarch);
>  
> -  return -1;
> +  if (valid_regnum == false)
> +    return -1;

Write:

  if (!valid_regnum)

But I fail to see the point of the new variable?  It's not used
after that check.

> +
> +  /* Definition of the offset foresees new kernel support, it has to be
> +     corrected here in case the kernel is older.  */

This comment reads to me like we'll need to change the code as soon
as the kernel is updated/changed for something.  Which is course
not what you want to say.

  /* Kernels that predate Linux 2.6.25 don't provide access to
     these segment registers in user_regs_struct.   */
#ifndef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
  if (regnum == AMD64_FSBASE_REGNUM || regnum == AMD64_GSBASE_REGNUM)
    return -1;
#endif

> index 0000000..d0a2a8f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-gs_base.c

> +
> +int
> +main (void)
> +{
> +  volatile int a;
> +  a = 10;
> +  return a;

Does the .exp file make use of this?

> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-gs_base.exp b/gdb/testsuite/gdb.arch/amd64-gs_base.exp
> new file mode 100644
> index 0000000..f2dc887
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-gs_base.exp
> @@ -0,0 +1,51 @@
> +# Copyright 2016 Free Software Foundation, Inc.
> +
> +# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +standard_testfile
> +
> +if { ![istarget "x86_64-*linux*"] } then {
> +    verbose "Skipping x86_64 fs_base and gs_base tests."
> +    return
> +}
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> +     [list debug nowarnings]] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +gdb_test "print /x \$fs_base" "= $hex" "print fs_base"
> +gdb_test "print /x \$gs_base" "= $hex" "print gs_base"
> +
> +gdb_test "print \$fs_base = 2" "= 2" "set fs_base"
> +gdb_test "print \$gs_base = 3" "= 3" "set gs_base"
> +
> +# Test the presence of fs_base and gs_base on the system
> +# register group and values.
> +#
> +set ws "\[\t \]*"

Better to use '+' instead of '*' to enforce at least
one space, and avoid false positives like 0x33.

> +set info_reg_out [multi_line "info register sys" \
> +          "fs_base${ws}0x2${ws}2"\
> +          "gs_base${ws}0x3${ws}3"\
> +          "orig_rax${ws}$hex${ws}\[-\]$decimal" ]
> +
> +gdb_test "info register sys" $info_reg_out\
> +   "info registers fs_base and gs_base with value"
> 

Thanks,
Pedro Alves



More information about the Gdb-patches mailing list