From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19624 invoked by alias); 27 Mar 2010 15:48:10 -0000 Received: (qmail 19604 invoked by uid 22791); 27 Mar 2010 15:48:08 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 27 Mar 2010 15:48:02 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id o2RFlwrj020367; Sat, 27 Mar 2010 16:47:58 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o2RFlvNI002277; Sat, 27 Mar 2010 16:47:57 +0100 (CET) Date: Sat, 27 Mar 2010 15:48:00 -0000 Message-Id: <201003271547.o2RFlvNI002277@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <20100306222037.GD21133@intel.com> (hongjiu.lu@intel.com) Subject: Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes) References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180643.GB10869@intel.com> <20100306222037.GD21133@intel.com> 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/msg00929.txt.bz2 > 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. I'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. OK? > 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 > @@ -23,6 +23,7 @@ > #include "frame.h" > #include "value.h" > #include "regcache.h" > +#include "regset.h" > #include "inferior.h" > #include "osabi.h" > #include "reggroups.h" > @@ -36,9 +37,11 @@ > #include "solib-svr4.h" > #include "symtab.h" > #include "arch-utils.h" > -#include "regset.h" > #include "xml-syscall.h" > > +#include "i387-tdep.h" > +#include "i386-xstate.h" > + > /* The syscall's XML filename for i386. */ > #define XML_SYSCALL_FILENAME_I386 "syscalls/i386-linux.xml" > > @@ -47,13 +50,15 @@ > #include > > #include "features/i386/i386-linux.c" > +#include "features/i386/i386-avx-linux.c" > > /* Supported register note sections. */ > -static struct core_regset_section i386_linux_regset_sections[] = > +struct core_regset_section i386_linux_regset_sections[] = Why do you make this non-static? > { > { ".reg", 144, "general-purpose" }, > { ".reg2", 108, "floating-point" }, > { ".reg-xfp", 512, "extended floating-point" }, > + { ".reg-xstate", 0, "XSAVE extended state" }, > { NULL, 0 } > }; > @@ -560,6 +566,66 @@ static int i386_linux_sc_reg_offset[] = > 0 * 4 /* %gs */ > }; > > +/* Update XSAVE extended state register note section. */ > + > +void > +i386_linux_update_xstateregset > + (struct core_regset_section *regset_sections, unsigned int xstate_size) > +{ > + int i; > + > + /* Update the XSAVE extended state register note section for "gcore". > + Disable it if its size is 0. */ > + for (i = 0; regset_sections[i].sect_name != NULL; i++) > + if (strcmp (regset_sections[i].sect_name, ".reg-xstate") == 0) > + { > + if (xstate_size) > + regset_sections[i].size = xstate_size; > + else > + regset_sections[i].sect_name = NULL; > + break; > + } > +} What will happen if you have a single GDB connected to two different remote targets, one with AVX support and one without? > +/* Get XSAVE extended state xcr0 from core dump. */ > + > +unsigned long long > +i386_linux_core_read_xcr0 (struct gdbarch *gdbarch, > + struct target_ops *target, bfd *abfd) If you follow my advice about using uint64_t for xr0, the return value will have to be adjusted. > +{ > + asection *xstate = bfd_get_section_by_name (abfd, ".reg-xstate"); > + unsigned long long xcr0; > + > + if (xstate) > + { > + size_t size = bfd_section_size (abfd, xstate); > + > + gdb_assert (size >= I386_XSTATE_SSE_SIZE); Isn't a gdb_assert() here a bit harsh? What happens if you simply return 0? > + /* Check extended state size. */ > + if (size < I386_XSTATE_AVX_SIZE) > + xcr0 = I386_XSTATE_SSE_MASK; > + else > + { > + char contents[8]; > + > + if (! bfd_get_section_contents (abfd, xstate, contents, > + (file_ptr) I386_LINUX_XSAVE_XCR0_OFFSET, > + 8)) Is that cast really necessary? > /* Get Linux/x86 target description from core dump. */ > > static const struct target_desc * > @@ -568,12 +634,17 @@ i386_linux_core_read_description (struct gdbarch *gdbarch, > bfd *abfd) > { > asection *section = bfd_get_section_by_name (abfd, ".reg2"); > + unsigned long long xcr0; > > if (section == NULL) > return NULL; > > /* Linux/i386. */ > - return tdesc_i386_linux; > + xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd); > + if ((xcr0 & I386_XSTATE_AVX_MASK) == I386_XSTATE_AVX_MASK) > + return tdesc_i386_avx_linux; > + else > + return tdesc_i386_linux; > } > > static void > @@ -623,6 +694,8 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > tdep->sc_reg_offset = i386_linux_sc_reg_offset; > tdep->sc_num_regs = ARRAY_SIZE (i386_linux_sc_reg_offset); > > + tdep->xsave_xcr0_offset = I386_LINUX_XSAVE_XCR0_OFFSET; > + > set_gdbarch_process_record (gdbarch, i386_process_record); > set_gdbarch_process_record_signal (gdbarch, i386_linux_record_signal); > > @@ -840,4 +913,5 @@ _initialize_i386_linux_tdep (void) > > /* Initialize the Linux target description */ > initialize_tdesc_i386_linux (); > + initialize_tdesc_i386_avx_linux (); > } > diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h > index 11f7295..8881fea 100644 > --- a/gdb/i386-linux-tdep.h > +++ b/gdb/i386-linux-tdep.h > @@ -30,12 +30,45 @@ > /* Register number for the "orig_eax" pseudo-register. If this > pseudo-register contains a value >= 0 it is interpreted as the > system call number that the kernel is supposed to restart. */ > -#define I386_LINUX_ORIG_EAX_REGNUM I386_SSE_NUM_REGS > +#define I386_LINUX_ORIG_EAX_REGNUM I386_AVX_NUM_REGS > > /* Total number of registers for GNU/Linux. */ > #define I386_LINUX_NUM_REGS (I386_LINUX_ORIG_EAX_REGNUM + 1) > > +/* Get XSAVE extended state xcr0 from core dump. */ > +extern unsigned long long i386_linux_core_read_xcr0 > + (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd); > + > /* Linux target description. */ > extern struct target_desc *tdesc_i386_linux; > +extern struct target_desc *tdesc_i386_avx_linux; > + > +/* Supported register note sections. */ > +extern struct core_regset_section i386_linux_regset_sections[]; > + > +/* Update XSAVE extended state register note section. */ > +extern void i386_linux_update_xstateregset > + (struct core_regset_section *regset_sections, unsigned int xstate_size); > + > +/* Format of XSAVE extended state is: > + struct > + { > + fxsave_bytes[0..463] > + sw_usable_bytes[464..511] > + xstate_hdr_bytes[512..575] > + avx_bytes[576..831] > + future_state etc > + }; > + > + Same memory layout will be used for the coredump NT_X86_XSTATE > + representing the XSAVE extended state registers. > + > + The first 8 bytes of the sw_usable_bytes[464..467] is set to OS enabled > + enabled state mask, which is same as the 64bit mask returned by the > + xgetbv's XCR0). We can use this mask as well as the mask saved in the > + xstate_hdr bytes to interpret what states the processor/OS supports and > + what state is in, used/initialized conditions, for the particular > + process/thread. */ Can you ask a native english speaker to look at this comment? > 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) > return tdep->i387_ext_type; > } > > +/* Construct vector type for pseudo XMM registers. We can't use > + tdesc_find_type since XMM isn't described in target description. */ I'm confused here. If you have a non-AVX target, why do you need a 256-bit vector type? > +static struct type * > +i386_ymm_type (struct gdbarch *gdbarch) > +{ > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > + if (!tdep->i386_ymm_type) > + { > + const struct builtin_type *bt = builtin_type (gdbarch); > + > + /* The type we're building is this: */ > +#if 0 > + union __gdb_builtin_type_vec256i > + { > + int128_t uint128[2]; > + int64_t v2_int64[4]; > + int32_t v4_int32[8]; > + int16_t v8_int16[16]; > + int8_t v16_int8[32]; > + double v2_double[4]; > + float v4_float[8]; > + }; > +#endif > + > + struct type *t; > + > + t = arch_composite_type (gdbarch, > + "__gdb_builtin_type_vec256i", TYPE_CODE_UNION); > + append_composite_type_field (t, "v8_float", > + init_vector_type (bt->builtin_float, 8)); > + append_composite_type_field (t, "v4_double", > + init_vector_type (bt->builtin_double, 4)); > + append_composite_type_field (t, "v32_int8", > + init_vector_type (bt->builtin_int8, 32)); > + append_composite_type_field (t, "v16_int16", > + init_vector_type (bt->builtin_int16, 16)); > + append_composite_type_field (t, "v8_int32", > + init_vector_type (bt->builtin_int32, 8)); > + append_composite_type_field (t, "v4_int64", > + init_vector_type (bt->builtin_int64, 4)); > + append_composite_type_field (t, "v2_int128", > + init_vector_type (bt->builtin_int128, 2)); > + > + TYPE_VECTOR (t) = 1; > + TYPE_NAME (t) = "builtin_type_vec128i"; > + tdep->i386_ymm_type = t; > + } > + > + return tdep->i386_ymm_type; > +} > + > /* Construct vector type for MMX registers. */ > static struct type * > i386_mmx_type (struct gdbarch *gdbarch) > @@ -2233,6 +2344,8 @@ i386_pseudo_register_type (struct gdbarch *gdbarch, int regnum) > { > if (i386_mmx_regnum_p (gdbarch, regnum)) > return i386_mmx_type (gdbarch); > + else if (i386_ymm_regnum_p (gdbarch, regnum)) > + return i386_ymm_type (gdbarch); > else > { > const struct builtin_type *bt = builtin_type (gdbarch); > @@ -2284,7 +2397,22 @@ i386_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache, > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > - if (i386_word_regnum_p (gdbarch, regnum)) > + if (i386_ymm_regnum_p (gdbarch, regnum)) > + { > + regnum -= tdep->ymm0_regnum; > + > + /* Extract (always little endian). Read lower 16byte. */ > + regcache_raw_read (regcache, > + I387_XMM0_REGNUM (tdep) + regnum, > + raw_buf); > + memcpy (buf, raw_buf, 16); > + /* Read upper 16byte. */ > + regcache_raw_read (regcache, > + tdep->ymm0h_regnum + regnum, > + raw_buf); > + memcpy (buf + 16, raw_buf, 16); > + } > + else if (i386_word_regnum_p (gdbarch, regnum)) > { > int gpnum = regnum - tdep->ax_regnum; > > @@ -2333,7 +2461,20 @@ i386_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache, > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > - if (i386_word_regnum_p (gdbarch, regnum)) > + if (i386_ymm_regnum_p (gdbarch, regnum)) > + { > + regnum -= tdep->ymm0_regnum; > + > + /* ... Write lower 16byte. */ > + regcache_raw_write (regcache, > + I387_XMM0_REGNUM (tdep) + regnum, > + buf); > + /* ... Write upper 16byte. */ > + regcache_raw_write (regcache, > + tdep->ymm0h_regnum + regnum, > + buf + 16); Culd you change the comments here to say 128-bit instead of 16byte? > @@ -5649,7 +5836,8 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, > struct tdesc_arch_data *tdesc_data) > { > const struct target_desc *tdesc = tdep->tdesc; > - const struct tdesc_feature *feature_core, *feature_vector; > + const struct tdesc_feature *feature_core; > + const struct tdesc_feature *feature_sse, *feature_avx; > int i, num_regs, valid_p; > > if (! tdesc_has_registers (tdesc)) > @@ -5659,13 +5847,37 @@ i386_validate_tdesc_p (struct gdbarch_tdep *tdep, > feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core"); > > /* Get SSE registers. */ > - feature_vector = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse"); > + feature_sse = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse"); > > - if (feature_core == NULL || feature_vector == NULL) > + if (feature_core == NULL || feature_sse == NULL) > return 0; > > + /* Try AVX registers. */ > + feature_avx = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.avx"); > + > valid_p = 1; > > + /* The XCR0 bits. */ > + if (feature_avx) > + { > + tdep->xcr0 = I386_XSTATE_AVX_MASK; > + > + /* It may be set by ABI-specific. */ Sorry, but does comment makes no sense to me. > @@ -5854,9 +6071,13 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > set_tdesc_pseudo_register_type (gdbarch, i386_pseudo_register_type); > set_tdesc_pseudo_register_name (gdbarch, i386_pseudo_register_name); > > - /* The default ABI includes general-purpose registers, > - floating-point registers, and the SSE registers. */ > - set_gdbarch_num_regs (gdbarch, I386_SSE_NUM_REGS); > + /* Override the normal target description method to make the AVX > + upper halves anonymous. */ > + set_gdbarch_register_name (gdbarch, i386_register_name); > + > + /* The default ABI includes general-purpose registers, floating-point > + registers, the SSE registers and the upper AVX registers. */ > + set_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? > @@ -5940,6 +6177,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > set_gdbarch_fast_tracepoint_valid_at (gdbarch, > i386_fast_tracepoint_valid_at); > > + /* Tell remote stub that we support XML target description. */ > + set_gdbarch_qsupported (gdbarch, "x86=xml"); > @@ -146,9 +156,24 @@ struct gdbarch_tdep > /* Number of SSE registers. */ > int num_xmm_regs; > > + /* Bits of the extended control register 0 (the XFEATURE_ENABLED_MASK > + register), excluding the x87 bit, which are supported by this gdb. > + */ > + unsigned long long xcr0; GDB should be capitalized.