diff mbox

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

Message ID 2907792.3mtlvelY3m@ralph.baldwin.cx
State New
Headers show

Commit Message

John Baldwin July 12, 2017, 8:03 p.m. UTC 
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().

Comments

Yao Qi July 13, 2017, 4:55 p.m. UTC | #1 
John Baldwin <jhb@freebsd.org> writes:

> 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.

The tdesc_reg is created for each target description, so there are
multiple tdesc_reg objects created.  That is, the tdesc_reg of "fs_base"
are different among these tdesc_amd64_*_linux.  However, the tdesc_reg
is a singleton in each target description.  Note that we can't use
singleton for each register globally, because tdesc_reg.target_regnum
can be different in different target descriptions.  For example,
fs_base's regnum is 57 in "bare-metal" amd64 target descriptions, while
it is 58 in "linux" amd64 target descriptions, because of 64bit-linux.xml.

>
> 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

Your patch fixes the crash, but I can't see fs_base and gs_base on my
machine either.

> 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().

because each tdesc_reg is a singleton among the target description.  The
reason Simon observed that we have different "fs_base" tdesc_reg added
and removed from the hash table is that they are from different target
descriptions.  GDB crashes in tdesc_use_registers.  The arguments tdesc
and tdesc_data are not consistent, tdesc is amd64 linux target
description, but tdesc_data was got when tdesc is amd64 target
description, so the tdesc_reg in tdesc_data are from amd64 target
description as well.  So, we push a "fs_base" from one target
description and want to remove a "fs_base" from another target
description.

Does this answer your question?  I think maybe there is some "better"
fix that is to keep tdesc and tdes_data consistent.  However, I don't
think it further.  Since current GDB trunk is unusable on x86_64-linux,
it is better get a fix soon.

>
> 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;
>      }
>  

There is one line, so braces are not needed.

>    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;

Indentation looks wrong.  Did you run regression test on x86_64-linux?
John Baldwin July 13, 2017, 5:04 p.m. UTC | #2 
On Thursday, July 13, 2017 05:55:06 PM Yao Qi wrote:
> John Baldwin <jhb@freebsd.org> writes:
> Your patch fixes the crash, but I can't see fs_base and gs_base on my
> machine either.

Ok.  I assume yours is newer than my (ancient) VM and thus this patch
doesn't work.
 
> > 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().
> 
> because each tdesc_reg is a singleton among the target description.  The
> reason Simon observed that we have different "fs_base" tdesc_reg added
> and removed from the hash table is that they are from different target
> descriptions.  GDB crashes in tdesc_use_registers.  The arguments tdesc
> and tdesc_data are not consistent, tdesc is amd64 linux target
> description, but tdesc_data was got when tdesc is amd64 target
> description, so the tdesc_reg in tdesc_data are from amd64 target
> description as well.  So, we push a "fs_base" from one target
> description and want to remove a "fs_base" from another target
> description.
> 
> Does this answer your question?  I think maybe there is some "better"
> fix that is to keep tdesc and tdes_data consistent.  However, I don't
> think it further.  Since current GDB trunk is unusable on x86_64-linux,
> it is better get a fix soon.

Ok.  I think then that the hash table in tdesc_use_registers shouldn't
be using the 'tdesc_reg' as the key but instead use the register name?
This should be fairly simple to implement via a std::unordered_map<>.
I'll try that in a bit, but if that doesn't resolve it we should revert
the commits until we have a real fix.
diff mbox

Patch

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;