From: Yao Qi <qiyaoltc@gmail.com>
To: John Baldwin <jhb@freebsd.org>
Cc: gdb-patches@sourceware.org,
Simon Marchi <simon.marchi@polymtl.ca>,
Phil Muldoon <pmuldoon@redhat.com>
Subject: Re: [PATCH 1/2] Include the fs_base and gs_base registers in amd64 target descriptions.
Date: Thu, 13 Jul 2017 16:55:00 -0000 [thread overview]
Message-ID: <86y3rsp991.fsf@gmail.com> (raw)
In-Reply-To: <2907792.3mtlvelY3m@ralph.baldwin.cx> (John Baldwin's message of "Wed, 12 Jul 2017 13:03:03 -0700")
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 (齐尧)
next prev parent reply other threads:[~2017-07-13 16:55 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-27 22:50 [PATCH 0/2] Support fs_base and gs_base for native FreeBSD/amd64 John Baldwin
2017-06-27 22:51 ` [PATCH 2/2] Support the fs_base and gs_base registers on FreeBSD/amd64 native processes John Baldwin
2017-07-11 8:09 ` Yao Qi
2017-06-27 22:51 ` [PATCH 1/2] Include the fs_base and gs_base registers in amd64 target descriptions John Baldwin
2017-07-11 8:03 ` Yao Qi
2017-07-11 16:26 ` John Baldwin
2017-07-12 12:16 ` Phil Muldoon
2017-07-12 13:02 ` Yao Qi
2017-07-12 13:51 ` Simon Marchi
2017-07-12 20:03 ` John Baldwin
2017-07-13 16:55 ` Yao Qi [this message]
2017-07-13 17:04 ` John Baldwin
2017-07-13 18:40 ` Pedro Alves
2017-07-13 19:59 ` Pedro Alves
2017-07-12 16:23 ` Keith Seitz
2017-07-11 7:49 ` [PATCH 0/2] Support fs_base and gs_base for native FreeBSD/amd64 Yao Qi
2017-07-11 16:26 ` John Baldwin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86y3rsp991.fsf@gmail.com \
--to=qiyaoltc@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=jhb@freebsd.org \
--cc=pmuldoon@redhat.com \
--cc=simon.marchi@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).