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 2/2] amd64-linux: expose system register FS_BASE and GS_BASE for Linux.


On 11/23/2016 01:01 PM, Pedro Alves wrote:
On 11/03/2016 09:47 AM, Walfred Tedeschi wrote:
This patch allows examination of the registers FS_BASE and GS_BASE
for Linux Systems running on 64bit. Tests for simple read and write
of the new registers is also added with this patch.

Tests were performed on:
x86_64 RHEL 5.3     - For the older ptrace calls.
x86_64 Ubuntu 16.10 - For the new ptrace calls.

2016-04-26  Walfred Tedeschi  <walfred.tedeschi@intel.com>
	    Richard Henderson  <rth@redhat.com>
What changed compared to Richard's original version?
I have added a test, gdbserver support was also added by me.
  
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 3f2a92b..a8a0b79 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -103,7 +103,14 @@ 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,
+  /* System register added at the end.  */
+#ifdef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
+  21 * 8,  22 * 8,		/* fs_base and gs_base.  */
+#else
+  -1, -1,			/* fs_base and gs_base.  */
+#endif
    15 * 8			      /* "orig_rax" */
+
Spurious new line?
Yes, fixed for the next version.

How's this meant to work for cross debugging, and core debugging?
I have used the loopback board to test the remote scenario, but it can be that this is not enough.
You have just pointed out one problem with this setup.

Core debugging was working automatically, as far as i could see. Core testes passed and have reported the FS_base register and GS_base registers. gcore tests report fs_base and gs_base registers while reading the registers from the core file.

For remote: I tested on loop back scenario, it also show good results.

Not sure if i have answered your questions.

I don't think it makes sense to put a host-specific
"#ifdef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE" check in a tdep file.
Agreed! I am using values that take into account the newest kernel.
On the other hand for older kernels there is the need to return -1 on the native file, like the snippet below:

diff --git a/gdb/amd64-nat.c b/gdb/amd64-nat.c
index ad5df57..e929735 100644
--- a/gdb/amd64-nat.c
+++ b/gdb/amd64-nat.c
@@ -65,6 +65,11 @@ amd64_native_gregset_reg_offset (struct gdbarch *gdbarch, int regnum)
   if (num_regs > gdbarch_num_regs (gdbarch))
     num_regs = gdbarch_num_regs (gdbarch);

+#ifndef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
+  if (regnum == AMD64_FSBASE_REGNUM || regnum == AMD64_GSBASE_REGNUM)
+    return -1;
+#endif

would this be ok?
diff --git a/gdb/features/Makefile b/gdb/features/Makefile
index 30eed5d..c87f29f 100644
--- a/gdb/features/Makefile
+++ b/gdb/features/Makefile
@@ -259,7 +259,7 @@ $(outdir)/i386/i386-linux.dat: i386/32bit-core.xml i386/32bit-sse.xml \
  			       i386/32bit-linux.xml
  $(outdir)/i386/amd64.dat: i386/64bit-core.xml i386/64bit-sse.xml
  $(outdir)/i386/amd64-linux.dat: i386/64bit-core.xml i386/64bit-sse.xml \
-			        i386/64bit-linux.xml
+			       i386/64bit-linux.xml i386/64bit-segments.xml
  $(outdir)/i386/i386-avx.dat: i386/32bit-core.xml i386/32bit-avx.xml
  $(outdir)/i386/i386-avx-linux.dat: i386/32bit-core.xml i386/32bit-avx.xml \
  			       i386/32bit-linux.xml
@@ -279,11 +279,11 @@ $(outdir)/i386/i386-mmx.dat: i386/32bit-core.xml
  $(outdir)/i386/i386-mmx-linux.dat: i386/32bit-core.xml i386/32bit-linux.xml
  $(outdir)/i386/amd64-avx.dat: i386/64bit-core.xml i386/64bit-avx.xml
  $(outdir)/i386/amd64-avx-linux.dat: i386/64bit-core.xml i386/64bit-avx.xml \
-				    i386/64bit-linux.xml
+			       i386/64bit-linux.xml i386/64bit-segments.xml
Is indentation on these two changes above correct?  Can't tell from
the mail client.
Yes, now all lines have same indentation.
+++ b/gdb/features/i386/64bit-segments.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2016 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.i386.segments">
+  <reg name="fs_base" bitsize="64" type="int" regnum="58"/>
+  <reg name="gs_base" bitsize="64" type="int" regnum="59"/>
#1 - Why is "regnum" hard coded?
Removed that, thanks.

#2 - Is bitsize 64 and type "int" really correct?
I suppose you saw something wrong here, that i missed.
My reaoning was the following: I used for gs_base and fs_base the same type used for orig_rax as they also have the same type the user_reg_struct.
from sys/user.h:
struct user_regs_struct
{
  __extension__ unsigned long long int r15;
...
  __extension__ unsigned long long int orig_rax;
...
  __extension__ unsigned long long int fs_base;
  __extension__ unsigned long long int gs_base;

...


--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -133,6 +133,11 @@ static const int x86_64_regmap[] =
    -1,
    -1, -1, -1, -1, -1, -1, -1, -1,
    ORIG_RAX * 8,
+#ifdef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
+  21 * 8,  22 * 8,
+#else
+  -1, -1,
+#endif
    -1, -1, -1, -1,			/* MPX registers BND0 ... BND3.  */
It's curious that above this was put after orig_rax, while
here:

This is due to the fact that we usually let orig_rax at last.
In the gdb side we fix it by means of the macros.
However, we are adding new system register we could change that a bit now.
Would you consider that it would be better to keep the block of the system registers at last in the order they were introduced?

I.e. now we let the fs_base and gs_base at last instead of the orig_rax.

--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -103,7 +103,14 @@ 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,
+  /* System register added at the end.  */
+#ifdef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE
+  21 * 8,  22 * 8,		/* fs_base and gs_base.  */
+#else
+  -1, -1,			/* fs_base and gs_base.  */
+#endif
    15 * 8			      /* "orig_rax" */
+

It was put before.

+++ b/gdb/testsuite/gdb.arch/amd64-gs_base.c
@@ -0,0 +1,33 @@
+/* Unwinder test program for fs_base and gs_base.
What aspect of the unwinder is being tested?
Sorry, wrong message.

+
+int
+func (int a)
+{
+  return a * a;
+}
+
+int
+main (void)
+{
+  volatile int a;
+  a = 10;
+  a = func (a);
Is any of this relevant for the test?
Not anymore. I did a previous version doing a step to force update of registers
in order to verify if the register written would survive.

This version was abandoned but code remained.
+  return a;
+}
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..ccd6b87
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-gs_base.exp
@@ -0,0 +1,57 @@
+# Copyright 2016 Free Software Foundation, Inc.
+
+# This file is part of the GDB testsuite.
+
+# 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/>.
+
+if { ![istarget "x86_64-*linux*"] } then {
+    verbose "Skipping x86_64 fs_base and gs_base tests."
+    return
+}
+
+standard_testfile
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	  executable { debug }] != "" } {
+    untested ${testfile}.exp
+    return -1
+}
+
+
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto func
+
Use prepare_for_testing and handle runto failure.
fixed for the next version.

+gdb_test "info register sys" $info_reg_out\
+   "info registers fs_base and gs_base with value "
Spurious space after "value".
yes, fixed for the next version.

Thanks,
Pedro Alves



Thanks a lot for your review!

Best 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]