| Message ID | b085e36b-6f10-e23c-8024-307bbe72fbd8@intel.com |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 5725 invoked by alias); 30 Nov 2016 10:29:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 3212 invoked by uid 89); 30 Nov 2016 10:29:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=53, H*r:10.253.24, !doctype, doctype X-HELO: mga04.intel.com Received: from mga04.intel.com (HELO mga04.intel.com) (192.55.52.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 10:29:32 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP; 30 Nov 2016 02:29:30 -0800 X-ExtLoop1: 1 Received: from labpc305.iul.intel.com (HELO [172.28.50.132]) ([172.28.50.132]) by fmsmga002.fm.intel.com with ESMTP; 30 Nov 2016 02:29:29 -0800 Subject: Re: [PATCH 2/2] amd64-linux: expose system register FS_BASE and GS_BASE for Linux. To: Pedro Alves <palves@redhat.com>, eliz@gnu.org, brobecker@adacore.com References: <1478166445-21370-1-git-send-email-walfred.tedeschi@intel.com> <1478166445-21370-3-git-send-email-walfred.tedeschi@intel.com> <4278085b-7272-47b5-047a-fd9f08a843dd@redhat.com> Cc: gdb-patches@sourceware.org From: "Tedeschi, Walfred" <walfred.tedeschi@intel.com> Message-ID: <b085e36b-6f10-e23c-8024-307bbe72fbd8@intel.com> Date: Wed, 30 Nov 2016 11:29:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <4278085b-7272-47b5-047a-fd9f08a843dd@redhat.com> Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Walfred Tedeschi
Nov. 30, 2016, 10:29 a.m. UTC
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: >> 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
Comments
On 11/30/2016 10:29 AM, Tedeschi, Walfred wrote: > 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. Right, that doesn't really exercise cross debugging, because in that case, both gdb and gdbserver are running on the same architecture. > 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. That's another manifestation of the same testing environment issue -- what if you load the x86 core on an ARM host? Then HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE won't be defined, while the data is available. Also what happens if you load a core dump generated on an old kernel, into a gdb running/built on a new kernel. And likewise the other way around (dump on new kernel, load on gdb built on old kernel). This is the sort of host-independence concerns that the core reading code should be having. > > For remote: I tested on loop back scenario, it also show good results. > > Not sure if i have answered your questions. Not exactly. > >> 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: See above. > > 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? I don't know exactly what this change means. Is HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE really about kernel version, or libc version? E.g., what do older kernels put in those gregset offsets? >>> +++ 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. Well, I'm seeing "int" and thinking "32-bit", since that's what it usually is, but then we have bitsize 64-bit. > 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. Funny, that's the only other case like this: $ grep -rn "type=\"int\"" | grep \"64\" i386/64bit-linux.xml:10: <reg name="orig_rax" bitsize="64" type="int" regnum="57"/> Looks like gdb may be being smart about this: (gdb) ptype $orig_rax type = long Is that the type you get back for the new registers too? >>> --- 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. I'm afraid you lost me. What macros? Thanks, Pedro Alves
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