From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26775 invoked by alias); 28 Mar 2010 01:37:49 -0000 Received: (qmail 26767 invoked by uid 22791); 28 Mar 2010 01:37:48 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-vw0-f41.google.com (HELO mail-vw0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 28 Mar 2010 01:37:44 +0000 Received: by vws20 with SMTP id 20so5188136vws.0 for ; Sat, 27 Mar 2010 18:37:42 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.90.201 with HTTP; Sat, 27 Mar 2010 18:37:41 -0700 (PDT) In-Reply-To: <201003271547.o2RFlvNI002277@glazunov.sibelius.xs4all.nl> References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180643.GB10869@intel.com> <20100306222037.GD21133@intel.com> <201003271547.o2RFlvNI002277@glazunov.sibelius.xs4all.nl> Date: Sun, 28 Mar 2010 01:37:00 -0000 Received: by 10.220.62.201 with SMTP id y9mr1761950vch.219.1269740261936; Sat, 27 Mar 2010 18:37:41 -0700 (PDT) Message-ID: <6dc9ffc81003271837l761ac147t7ba88988cb99fcb0@mail.gmail.com> Subject: Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes) From: "H.J. Lu" To: Mark Kettenis Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00937.txt.bz2 On Sat, Mar 27, 2010 at 8:47 AM, Mark Kettenis wr= ote: >> Date: Sat, 6 Mar 2010 14:20:37 -0800 >> From: "H.J. Lu" >> >> Hi, >> >> Here are i386 changes to support AVX. OK to install? > > OK, here's a review of the remainder of this part of the diff. =A0I'll > wait with reviewing the amd64 bits until we've got the i386 part > right, since a lot of what I'll say about i386 will also apply to > amd64. =A0OK? That is fine. >> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c >> index b23c109..66ecf84 100644 >> --- a/gdb/i386-linux-tdep.c >> +++ b/gdb/i386-linux-tdep.c >> +#include "i387-tdep.h" >> +#include "i386-xstate.h" >> + >> =A0/* The syscall's XML filename for i386. =A0*/ >> =A0#define XML_SYSCALL_FILENAME_I386 "syscalls/i386-linux.xml" >> >> @@ -47,13 +50,15 @@ >> =A0#include >> >> =A0#include "features/i386/i386-linux.c" >> +#include "features/i386/i386-avx-linux.c" >> >> =A0/* Supported register note sections. =A0*/ >> -static struct core_regset_section i386_linux_regset_sections[] =3D >> +struct core_regset_section i386_linux_regset_sections[] =3D > > Why do you make this non-static? I need to change size of .reg-xstate section from i386-linux-nat.c. >> =A0{ >> =A0 =A0{ ".reg", 144, "general-purpose" }, >> =A0 =A0{ ".reg2", 108, "floating-point" }, >> =A0 =A0{ ".reg-xfp", 512, "extended floating-point" }, >> + =A0{ ".reg-xstate", 0, "XSAVE extended state" }, >> =A0 =A0{ NULL, 0 } >> =A0}; >> @@ -560,6 +566,66 @@ static int i386_linux_sc_reg_offset[] =3D >> =A0 =A00 * 4 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= /* %gs */ >> =A0}; >> >> +/* Update XSAVE extended state register note section. =A0*/ >> + >> +void >> +i386_linux_update_xstateregset >> + =A0(struct core_regset_section *regset_sections, unsigned int xstate_s= ize) >> +{ >> + =A0int i; >> + >> + =A0/* Update the XSAVE extended state register note section for "gcore= ". >> + =A0 =A0 Disable it if its size is 0. =A0*/ >> + =A0for (i =3D 0; regset_sections[i].sect_name !=3D NULL; i++) >> + =A0 =A0if (strcmp (regset_sections[i].sect_name, ".reg-xstate") =3D=3D= 0) >> + =A0 =A0 =A0{ >> + =A0 =A0 if (xstate_size) >> + =A0 =A0 =A0 regset_sections[i].size =3D xstate_size; >> + =A0 =A0 else >> + =A0 =A0 =A0 regset_sections[i].sect_name =3D NULL; >> + =A0 =A0 break; >> + =A0 =A0 =A0} >> +} > > What will happen if you have a single GDB connected to two different > remote targets, one with AVX support and one without? The size of .reg-xstate section is used only for native gcore and won't be used for remote targets. >> +/* Get XSAVE extended state xcr0 from core dump. =A0*/ >> + >> +unsigned long long >> +i386_linux_core_read_xcr0 (struct gdbarch *gdbarch, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct target_ops *targ= et, bfd *abfd) > > If you follow my advice about using uint64_t for xr0, the return value > will have to be adjusted. I will make the change. >> +{ >> + =A0asection *xstate =3D bfd_get_section_by_name (abfd, ".reg-xstate"); >> + =A0unsigned long long xcr0; >> + >> + =A0if (xstate) >> + =A0 =A0{ >> + =A0 =A0 =A0size_t size =3D bfd_section_size (abfd, xstate); >> + >> + =A0 =A0 =A0gdb_assert (size >=3D I386_XSTATE_SSE_SIZE); > > Isn't a gdb_assert() here a bit harsh? =A0What happens if you simply retu= rn 0? I will remove it. If the size < I386_XSTATE_SSE_SIZE, a warning will be iss= ued and 0 will be returned. >> + =A0 =A0 =A0/* Check extended state size. =A0*/ >> + =A0 =A0 =A0if (size < I386_XSTATE_AVX_SIZE) >> + =A0 =A0 xcr0 =3D I386_XSTATE_SSE_MASK; >> + =A0 =A0 =A0else >> + =A0 =A0 { >> + =A0 =A0 =A0 char contents[8]; >> + >> + =A0 =A0 =A0 if (! bfd_get_section_contents (abfd, xstate, contents, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 (file_ptr) I386_LINUX_XSAVE_XCR0_OFFSET, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 8)) > > Is that cast really necessary? I just follow the tradition. Most of bfd_get_section_contents calls have (file_ptr) cast. It may be used to avoid 32bit vs 64bit VMA warning. >> + =A0Same memory layout will be used for the coredump NT_X86_XSTATE >> + =A0representing the XSAVE extended state registers. >> + >> + =A0The first 8 bytes of the sw_usable_bytes[464..467] is set to OS ena= bled >> + =A0enabled state mask, =A0which is same as the 64bit mask returned by = the >> + =A0xgetbv's XCR0). We can use this mask as well as the mask saved in t= he >> + =A0xstate_hdr bytes to interpret what states the processor/OS supports= and >> + =A0what state is in, used/initialized conditions, for the particular >> + =A0process/thread. =A0*/ > > Can you ask a native english speaker to look at this comment? I will see what I can do. >> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c >> index 05afa56..8ced34a 100644 >> --- a/gdb/i386-tdep.c >> +++ b/gdb/i386-tdep.c >> @@ -2183,6 +2241,59 @@ i387_ext_type (struct gdbarch *gdbarch) >> =A0 =A0return tdep->i387_ext_type; >> =A0} >> >> +/* Construct vector type for pseudo XMM registers. =A0We can't use >> + =A0 tdesc_find_type since XMM isn't described in target description. = =A0*/ > > I'm confused here. =A0If you have a non-AVX target, why do you need a 256= -bit vector type? i386_ymm_type is only called from else if (i386_ymm_regnum_p (gdbarch, regnum)) return i386_ymm_type (gdbarch); It won't be called if you have a non-AVX target. >> +static struct type * >> +i386_ymm_type (struct gdbarch *gdbarch) >> +{ .. >> =A0 =A0if (i386_mmx_regnum_p (gdbarch, regnum)) >> =A0 =A0 =A0return i386_mmx_type (gdbarch); >> + =A0else if (i386_ymm_regnum_p (gdbarch, regnum)) >> + =A0 =A0return i386_ymm_type (gdbarch); >> =A0 =A0else ... >> + =A0 =A0 =A0 /* ... Write lower 16byte. =A0*/ >> + =A0 =A0 =A0 regcache_raw_write (regcache, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0I387_XMM0_REGNUM (t= dep) + regnum, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf); >> + =A0 =A0 =A0 /* ... Write upper 16byte. =A0*/ >> + =A0 =A0 =A0 regcache_raw_write (regcache, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tdep->ymm0h_regnum = + regnum, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0buf + 16); > > Culd you change the comments here to say 128-bit instead of 16byte? I will make the change. >> @@ -5649,7 +5836,8 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct tdesc_arch_data *tdesc= _data) >> =A0{ >> =A0 =A0const struct target_desc *tdesc =3D tdep->tdesc; >> - =A0const struct tdesc_feature *feature_core, *feature_vector; >> + =A0const struct tdesc_feature *feature_core; >> + =A0const struct tdesc_feature *feature_sse, *feature_avx; >> =A0 =A0int i, num_regs, valid_p; >> >> =A0 =A0if (! tdesc_has_registers (tdesc)) >> @@ -5659,13 +5847,37 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, >> =A0 =A0feature_core =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.cor= e"); >> >> =A0 =A0/* Get SSE registers. =A0*/ >> - =A0feature_vector =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse= "); >> + =A0feature_sse =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse"); >> >> - =A0if (feature_core =3D=3D NULL || feature_vector =3D=3D NULL) >> + =A0if (feature_core =3D=3D NULL || feature_sse =3D=3D NULL) >> =A0 =A0 =A0return 0; >> >> + =A0/* Try AVX registers. =A0*/ >> + =A0feature_avx =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.avx"); >> + >> =A0 =A0valid_p =3D 1; >> >> + =A0/* The XCR0 bits. =A0*/ >> + =A0if (feature_avx) >> + =A0 =A0{ >> + =A0 =A0 =A0tdep->xcr0 =3D I386_XSTATE_AVX_MASK; >> + >> + =A0 =A0 =A0/* It may be set by ABI-specific. =A0*/ > > Sorry, but does comment makes no sense to me. I will update it. >> @@ -5854,9 +6071,13 @@ i386_gdbarch_init (struct gdbarch_info info, stru= ct gdbarch_list *arches) >> =A0 =A0set_tdesc_pseudo_register_type (gdbarch, i386_pseudo_register_typ= e); >> =A0 =A0set_tdesc_pseudo_register_name (gdbarch, i386_pseudo_register_nam= e); >> >> - =A0/* The default ABI includes general-purpose registers, >> - =A0 =A0 floating-point registers, and the SSE registers. =A0*/ >> - =A0set_gdbarch_num_regs (gdbarch, I386_SSE_NUM_REGS); >> + =A0/* Override the normal target description method to make the AVX >> + =A0 =A0 upper halves anonymous. =A0*/ >> + =A0set_gdbarch_register_name (gdbarch, i386_register_name); >> + >> + =A0/* The default ABI includes general-purpose registers, floating-poi= nt >> + =A0 =A0 registers, the SSE registers and the upper AVX registers. =A0*/ >> + =A0set_gdbarch_num_regs (gdbarch, I386_AVX_NUM_REGS); > > Isn't it better to leave the AVX registers out of the default target, > and only provide them if we're talking to a target (native or remote) > that indicates it supports them? That is set to a value higher enough to support AVX. The actual number of registers will be set properly later. See: http://sourceware.org/ml/gdb-patches/2010-02/msg00709.html >> @@ -5940,6 +6177,9 @@ i386_gdbarch_init (struct gdbarch_info info, struc= t gdbarch_list *arches) >> =A0 =A0set_gdbarch_fast_tracepoint_valid_at (gdbarch, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 i386_fast_tracepoint_valid_at); >> >> + =A0/* Tell remote stub that we support XML target description. =A0*/ >> + =A0set_gdbarch_qsupported (gdbarch, "x86=3Dxml"); > >> @@ -146,9 +156,24 @@ struct gdbarch_tdep >> =A0 =A0/* Number of SSE registers. =A0*/ >> =A0 =A0int num_xmm_regs; >> >> + =A0/* Bits of the extended control register 0 (the XFEATURE_ENABLED_MA= SK >> + =A0 =A0 register), excluding the x87 bit, which are supported by this = gdb. >> + =A0 */ >> + =A0unsigned long long xcr0; > > GDB should be capitalized. > I will make the change. Thanks. --=20 H.J.