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

[Pedro Alves <palves at redhat dot com>]

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