[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