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

["H.J. Lu" <hjl dot tools at gmail dot 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.