This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]