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

Yao Qi
2017-07-13T16:55:17Z

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 (齐尧)