Re: [PATCH 1/2] Include the fs_base and gs_base registers in amd64 target descriptions.

[John Baldwin <jhb at freebsd dot org>]

On Wednesday, July 12, 2017 03:50:58 PM Simon Marchi wrote:
> On 2017-07-12 15:02, Yao Qi wrote:
> > On Wed, Jul 12, 2017 at 1:16 PM, Phil Muldoon <pmuldoon@redhat.com> 
> > wrote:
> >> 
> >> Does anyone else see this?
> >> 
> > 
> > I don't see the issue you posted here, but there are
> > some regressions shown in buildbot.
> > https://sourceware.org/ml/gdb-testers/2017-q3/msg00370.html
> > 
> > John, could you take a look?
> 
> I looked around, I don't know exactly what's wrong but I might have some 
> pointers.
> 
>  From what I understand, tdesc_use_registers adds the registers listed in 
> the target description to a hash table, and then removes some of those 
> same registers that are also specified by the architecture, so it's left 
> with the registers for which we need to assign a number.  The hash table 
> hash the pointer itself, it does not look at what it points to.
> 
> I added some prints where we add and remove the registers from the htab. 
>   For rax, it looks good:
> 
> ADDING 0x237a510 rax
> REMOVING 0x237a510 rax
> 
> We add and remov the same pointer.  For fs_base, the pointers are 
> different:
> 
> ADDING 0x237d840 fs_base
> REMOVING 0x23a5d70 fs_base
> 
> It suggests that something is wrong here, I would expect those two 
> pointers to be equal.  I don't know enough about how the reg objects are 
> created to know why it this happens.

I'm not sure why these should actually be equal at all.  In theory we
are resolving two different target descriptions which should have each
called tdesc_create_reg().  I don't see anything that follows a
singleton-like pattern so that if two tdesc's create the same register
with the same name they point to the same 'struct tdesc_reg *' though
that seems to be what is happening for other registers other than
fs_base and gs_base.

I noticed that amd64_init_abi() in amd64-tdep.c invokes
tdesc_numbered_register earlier than is done for other registers on x86.
In particular, all the other registers are "added" via
tdesc_numbered_register in i386_validate_tdesc() which is called after
gdbarch_init_abi() (and thus after any OS-dependent hooks have had a
chance to complete).  On a whim I tried deferring adding fs_base and
gs_base until i386_validate_tdesc().  This does seem to fix the crash
for me on a CentOS 7 VM I have lying around.  The fs_base and gs_base
registers still work for me on FreeBSD.  They do not show up in
info registers on the CentOS VM, but perhaps it is too old to have
the relevant ptrace ops (I know it's too old to have the updated
struct user_reg).  What I don't really understand though is why this
works.  I also don't fully understand why 'data->arch_regs' is supposed
to always hold the same pointer values as in 'target_desc' in
tdesc_use_registers().

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9ff7dfc513..3196ef75a1 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3061,15 +3061,7 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments") != NULL)
     {
-      const struct tdesc_feature *feature =
-	  tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments");
-      struct tdesc_arch_data *tdesc_data_segments =
-	  (struct tdesc_arch_data *) info.tdep_info;
-
-      tdesc_numbered_register (feature, tdesc_data_segments,
-		       AMD64_FSBASE_REGNUM, "fs_base");
-      tdesc_numbered_register (feature, tdesc_data_segments,
-		       AMD64_GSBASE_REGNUM, "gs_base");
+      tdep->fsbase_regnum = AMD64_FSBASE_REGNUM;
     }
 
   if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys") != NULL)
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index bd728f03dc..1c8263cc87 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8199,7 +8199,7 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep,
   const struct tdesc_feature *feature_core;
 
   const struct tdesc_feature *feature_sse, *feature_avx, *feature_mpx,
-			     *feature_avx512, *feature_pkeys;
+    *feature_avx512, *feature_pkeys, *feature_segments;
   int i, num_regs, valid_p;
 
   if (! tdesc_has_registers (tdesc))
@@ -8225,6 +8225,9 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep,
   /* Try PKEYS  */
   feature_pkeys = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.pkeys");
 
+  /* Try segment base registers.  */
+  feature_segments = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.segments");
+
   valid_p = 1;
 
   /* The XCR0 bits.  */
@@ -8347,6 +8350,14 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep,
 					    tdep->pkeys_register_names[i]);
     }
 
+  if (feature_segments && tdep->fsbase_regnum >= 0)
+    {
+      valid_p &= tdesc_numbered_register (feature_segments, tdesc_data,
+					  tdep->fsbase_regnum, "fs_base");
+      valid_p &= tdesc_numbered_register (feature_segments, tdesc_data,
+					  tdep->fsbase_regnum + 1, "gs_base");
+    }
+
   return valid_p;
 }
 
@@ -8591,6 +8602,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   tdep->pkru_regnum = -1;
   tdep->num_pkeys_regs = 0;
 
+  /* No segment base registers. */
+  tdep->fsbase_regnum = -1;
+
   tdesc_data = tdesc_data_alloc ();
 
   set_gdbarch_relocate_instruction (gdbarch, i386_relocate_instruction);
diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
index 1ce89fcf65..a887a47752 100644
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -198,6 +198,10 @@ struct gdbarch_tdep
   /* PKEYS register names.  */
   const char **pkeys_register_names;
 
+  /* Register number for %fs_base.  Set this to -1 indicate the absence of
+     segment base registers.  */
+  int fsbase_regnum;
+
   /* Target description.  */
   const struct target_desc *tdesc;
 

-- 
John Baldwin