[PATCH V2 1/1] amd64-linux: expose system register FS_BASE and GS_BASE for Linux.
Tedeschi, Walfred
walfred.tedeschi@intel.com
Fri Jan 20 14:29:00 GMT 2017
On 01/19/2017 08:44 PM, Pedro Alves wrote:
> 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 was considering newer and older versions gdb/gdbserver respectively.
This came into my mind due to some reviews with Mark. That was the first
commit for MPX.
>
> (I think some punctuation in the first sentence would make it
> much easier to grok.)
I will change the text.
>> 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 [...] ?
and in GDB just before.
(sorry)
>
>> 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?
To let ORIG_RAX at the end in GDB, adding general register before it.
As you guessed bellow.
>> 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.
Agree, it would be easier. I saw the changes in Richard's commit
and liked it a lot.
Will do as you proposed, with a previous preparatory commit.
>>
>> 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.
I was considering a post patch, but pre patch is better; This way we do
not need to create an
additional variable.
>> +
>> + /* 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?
Not really, I will minimize the sample.
>> +}
>> 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.
ok! Thanks!
>> +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
>
Thank you very much for the review!
Regards,
/Fred
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
More information about the Gdb-patches
mailing list