Re: [PATCH 2/9] Support fs_base and gs_base on FreeBSD/i386.

[John Baldwin <jhb@FreeBSD.org>]

On 2/2/19 7:26 AM, Simon Marchi wrote:
> On 2019-01-22 13:42, John Baldwin wrote:
>> The i386 BSD native target uses the same ptrace operations
>> (PT_[GS]ET[FG]SBASE) as the amd64 BSD native target to fetch and store
>> the registers.
>>
>> The amd64 BSD native now uses 'tdep->fsbase_regnum' instead of
>> hardcoding AMD64_FSBASE_REGNUM and AMD64_GSBASE_REGNUM to support
>> 32-bit targets.  In addition, the store operations explicitly zero the
>> new register value before fetching it from the register cache to
>> ensure 32-bit values are zero-extended.
> 
> To be clear, this happens when debugging a 32-bits process on a 64-bits 
> OS?  When debugging a 32-bits process on a 32-bits OS, the code in 
> i386-bsd-nat.c would be used?

Correct.

>> gdb/ChangeLog:
>>
>> 	* amd64-bsd-nat.c (amd64bsd_fetch_inferior_registers): Use
>> 	tdep->fsbase_regnum instead of constants for fs_base and gs_base.
>> 	(amd64bsd_store_inferior_registers): Likewise.
>> 	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
>> 	Enable segment base registers.
>> 	* i386-bsd-nat.c (i386bsd_fetch_inferior_registers): Use
>> 	PT_GETFSBASE and PT_GETGSBASE.
>> 	(i386bsd_store_inferior_registers): Use PT_SETFSBASE and PT_SETGSBASE.
>> 	* i386-fbsd-nat.c (i386_fbsd_nat_target::read_description): Enable
>> 	segment base registers.
>> 	* i386-fbsd-tdep.c (i386fbsd_core_read_description): Likewise.
>> ---
>>  gdb/ChangeLog        | 14 ++++++++++++
>>  gdb/amd64-bsd-nat.c  | 26 ++++++++++++++-------
>>  gdb/amd64-fbsd-nat.c |  4 ++--
>>  gdb/i386-bsd-nat.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++
>>  gdb/i386-fbsd-nat.c  |  2 +-
>>  gdb/i386-fbsd-tdep.c |  2 +-
>>  6 files changed, 90 insertions(+), 12 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 4afd5b664e..056a60fa23 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,17 @@
>> +2019-01-22  John Baldwin  <jhb@FreeBSD.org>
>> +
>> +	* amd64-bsd-nat.c (amd64bsd_fetch_inferior_registers): Use
>> +	tdep->fsbase_regnum instead of constants for fs_base and gs_base.
>> +	(amd64bsd_store_inferior_registers): Likewise.
>> +	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
>> +	Enable segment base registers.
>> +	* i386-bsd-nat.c (i386bsd_fetch_inferior_registers): Use
>> +	PT_GETFSBASE and PT_GETGSBASE.
>> +	(i386bsd_store_inferior_registers): Use PT_SETFSBASE and 
>> PT_SETGSBASE.
>> +	* i386-fbsd-nat.c (i386_fbsd_nat_target::read_description): Enable
>> +	segment base registers.
>> +	* i386-fbsd-tdep.c (i386fbsd_core_read_description): Likewise.
>> +
>>  2019-01-22  John Baldwin  <jhb@FreeBSD.org>
>>
>>  	* amd64-fbsd-nat.c (amd64_fbsd_nat_target::read_description):
>> diff --git a/gdb/amd64-bsd-nat.c b/gdb/amd64-bsd-nat.c
>> index a2a91abb91..0f47ff6c61 100644
>> --- a/gdb/amd64-bsd-nat.c
>> +++ b/gdb/amd64-bsd-nat.c
>> @@ -43,6 +43,9 @@ amd64bsd_fetch_inferior_registers (struct regcache
>> *regcache, int regnum)
>>  {
>>    struct gdbarch *gdbarch = regcache->arch ();
>>    pid_t pid = get_ptrace_pid (regcache->ptid ());
>> +#ifdef PT_GETFSBASE
>> +  const struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +#endif
> 
> TDEP is used in both #ifdef PT_GETFSBASE and #ifdef PT_GETGSBASE, but is 
> only declared here in an #ifdef PT_GETFSBASE.  I suppose it's not 
> actually an issue because they will always be present together.  But we 
> might as well use "#if defined(PT_GETFSBASE) || defined(PT_GETGSBASE)" 
> for the declaration.

Ok.  I could also just perhaps collapse the other two #ifdef's instead of
using separate ones.  FreeBSD always provides both if it provides one.  None
of the other BSD's provide these currently.  I suspect if they did they
would always provide both.

>> @@ -134,11 +140,13 @@ amd64bsd_store_inferior_registers (struct
>> regcache *regcache, int regnum)
>>      }
>>
>>  #ifdef PT_SETFSBASE
>> -  if (regnum == -1 || regnum == AMD64_FSBASE_REGNUM)
>> +  if (regnum == -1 || regnum == tdep->fsbase_regnum)
>>      {
>>        register_t base;
>>
>> -      regcache->raw_collect (AMD64_FSBASE_REGNUM, &base);
>> +      /* Clear the full base value to support 32-bit targets.  */
>> +      base = 0;
>> +      regcache->raw_collect (tdep->fsbase_regnum, &base);
> 
> It's probably safer to clear the value to 0 as you did, so that's fine.  
> But I would have thought that when debugging 32-bits processes, the high 
> bits would be ignored at some point.  The kernel would know that this is 
> a 32 bits register, so it would just take the 32 low bits of what it 
> receives, and it magically works whatever the original data type is 
> because it's little endian.

I had originally just done this out of paranoia, but I checked and FreeBSD's
amd64 kernel will actually fail attempts to set the base address higher
than 4G with EINVAL rather than silently truncating.

-- 
John Baldwin