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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> 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