Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes)

[Mark Kettenis <mark.kettenis@xs4all.nl>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5887 bytes --]

> Date: Sat, 27 Mar 2010 18:37:41 -0700
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> >> 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 <stdint.h>
> >>
> >>  #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