From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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 , 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" Message-ID: Date: Wed, 30 Nov 2016 10:29:00 -0000 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 X-SW-Source: 2016-11/txt/msg00968.txt.bz2 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 >> Richard Henderson > What changed compared to Richard's original version? I have added a test, gdbserver support was also added by me. >> =0C >> 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[] =3D >> -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=20 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=20 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=20 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=20 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=20 *gdbarch, int regnum) if (num_regs > gdbarch_num_regs (gdbarch)) num_regs =3D gdbarch_num_regs (gdbarch); +#ifndef HAVE_STRUCT_USER_REGS_STRUCT_FS_BASE + if (regnum =3D=3D AMD64_FSBASE_REGNUM || regnum =3D=3D AMD64_GSBASE_REGN= UM) + 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 i= 386/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-linu= x.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 @@ >> + >> + >> + >> + >> + >> + >> + > #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=20 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[] =3D >> -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=20 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[] =3D > -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 =3D 10; >> + a =3D func (a); > Is any of this relevant for the test? Not anymore. I did a previous version doing a step to force update of=20 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/gd= b.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 . >> + >> +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 }] !=3D "" } { >> + 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