From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15918 invoked by alias); 28 Mar 2010 11:55:46 -0000 Received: (qmail 15909 invoked by uid 22791); 28 Mar 2010 11:55:46 -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; Sun, 28 Mar 2010 11:55:41 +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 o2SBtZNv005874; Sun, 28 Mar 2010 13:55:35 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o2SBtYEk029587; Sun, 28 Mar 2010 13:55:34 +0200 (CEST) Date: Sun, 28 Mar 2010 11:55:00 -0000 Message-Id: <201003281155.o2SBtYEk029587@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <6dc9ffc81003271837l761ac147t7ba88988cb99fcb0@mail.gmail.com> (hjl.tools@gmail.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> <201003271547.o2RFlvNI002277@glazunov.sibelius.xs4all.nl> <6dc9ffc81003271837l761ac147t7ba88988cb99fcb0@mail.gmail.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/msg00940.txt.bz2 > Date: Sat, 27 Mar 2010 18:37:41 -0700 > From: "H.J. Lu" > > >> 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" > >> + > >>  /* 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? > > I need to change size of .reg-xstate section from i386-linux-nat.c. But then, why do you have the i386_linux_update_xstateregs() function if you still need to pass the array itself around? Anyway, how about setting the size of the .reg-xstate to I386_XSTATE_SSE_SIZE unconditionally? Tools will look at xcr0 value encoded in there to determine what information in there is valid, so dumping a little bit more than strictly necessary shouldn't be a problem. It would simplify things a bit. Less code is good! > >>  { > >>    { ".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? > > The size of .reg-xstate section is used only for native gcore and > won't be used for remote targets. Ugh, yes you're right, gcore is a native-only feature. > >> +      /* 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? > > 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. Please don't use casts when they're not absolutely necessary; they tend to hide bugs. > >> 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? > > 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. Sorry; that confuses me even more. Let me try to explain again what puzzles me. The pseudo XMM registers are 128-bit, so why are you building a 256-bit type? Is the problem simply that the comment is wrong and you're talking about pseudo YMM registers here? > >> @@ -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? > > That is set to a value higher enough to support AVX. The actual number > of registers will be set properly later. See: OK, then please adjust the comment to say something like: /* Even though the default ABI only includes general-purpose registers, floating-point registers and the SSE registers, we have to leave a gap for the upper AVX registers.  */ Thanks, Mark