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

["H.J. Lu" <hjl.tools@gmail.com>]

On Sun, Mar 28, 2010 at 4:55 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> 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?

i386-linux-nat.c calls i386_linux_update_xstateregs with
 i386_linux_regset_sections. Also amd64-linux-nat.c calls
i386_linux_update_xstateregset with amd64_linux_regset_sections.
If I don't make amd64_linux_regset_sections and
i386_linux_regset_sections global, I have to write
i386_linux_update_xstateregset and amd64_linux_update_xstateregset.
The only difference of 2 functions will be amd64_linux_regset_sections
vs. i386_linux_regset_sections.

> 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.

That will make the code more complex since the generic gcore
implementation will have to adjust section size based on XCR0.
But if it is what is required, I will make the change.

> It would simplify things a bit.  Less code is good!
>
>
>> >> +      /* 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.

I will make the change.

>> >> 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?

Ooops. I meant "pseudo YMM registers". I will update comments.

>> >> @@ -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.  */
>

I will make the change.

Thanks.

-- 
H.J.