From patchwork Wed Jul 12 20:03:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Baldwin X-Patchwork-Id: 21565 Received: (qmail 87699 invoked by alias); 12 Jul 2017 20:03:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 87384 invoked by uid 89); 12 Jul 2017 20:03:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=1986, whim, H*F:D*freebsd.org, lying X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Jul 2017 20:03:08 +0000 Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 181EB10AB01; Wed, 12 Jul 2017 16:03:06 -0400 (EDT) From: John Baldwin To: gdb-patches@sourceware.org, Simon Marchi Cc: Yao Qi , Phil Muldoon Subject: Re: [PATCH 1/2] Include the fs_base and gs_base registers in amd64 target descriptions. Date: Wed, 12 Jul 2017 13:03:03 -0700 Message-ID: <2907792.3mtlvelY3m@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-STABLE; KDE/4.14.10; amd64; ; ) In-Reply-To: References: <20170627224948.99138-1-jhb@FreeBSD.org> MIME-Version: 1.0 X-IsSubscribed: yes 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 > > 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;