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

[Yao Qi <qiyaoltc at gmail dot com>]

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?

-- 
Yao (齐尧)