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

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

On Sat, Mar 27, 2010 at 7:54 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Thu, 11 Mar 2010 16:00:05 -0800
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>>
>> >> +
>> >> +#include "i386-xstate.h"
>> >> +
>> >> +#ifndef PTRACE_GETREGSET
>> >> +#define PTRACE_GETREGSET ? ? 0x4204
>> >> +#endif
>> >> +
>> >> +#ifndef PTRACE_SETREGSET
>> >> +#define PTRACE_SETREGSET ? ? 0x4205
>> >> +#endif
>> >> +
>> >> +#endif ? ? ? /* NM_LINUX_XSTATE_H */
>> >
>> > Do we really have to hardcode constants like this in GDB? ?They should
>> > be available in through kernel/libc headers. ?Are Drepper and Torvalds
>> > still fighting over that issue?
>>
>> They are in Linux kernel 2.6.34-rc1. Do we enable gdb support only
>> with the new kernel/glibc headers? I compiled gdb on RHEL4 and it
>> works fine. ?There are:
>>
>> #ifndef PTRACE_GET_THREAD_AREA
>> #define PTRACE_GET_THREAD_AREA 25
>> ?...
>> #ifndef PTRACE_ARCH_PRCTL
>> #define PTRACE_ARCH_PRCTL ? ? ?30
>>
>> in amd64-linux-nat.c.
>
> Yes, we have done that in the past, but I think we should stop adding
> #defines like that.

AVX gdb support only needs PTRACE_GETREGSET/PTRACE_SETREGSET,
which are fixed constants. I don't think we should require new kernel/glibc
header files for AVX support. I can change it to

#ifdef PTRACE_GETREGSET
#if PTRACE_GETREGSET != 0x4204
# error PTRACE_GETREGSET != 0x4204
#endif
#else
#define PTRACE_GETREGSET        0x4204
#endif


>> >> +
>> >> +/* The extended state size in unit of int64. ?We use array of int64 for
>> >> + ? better alignment. ?*/
>> >> +static unsigned int xstate_size_n_of_int64;
>> >
>> > Does alignment really matter? ?I'd rather do without this additional
>> > complication.
>>
>> "xcr0" is a 64bit value. ?It is nice to use array of uint64 to access it.
>
> But there are also 32-bit, 128-bit and 256-bit fields in the xstate.
> Therefore I think that typing it as an array of 64-bit values is
> misleading.

I will change it.

>> >> +static int
>> >> +fetch_xstateregs (struct regcache *regcache, int tid)
>> >> +{
>> >> + ?unsigned long long xstateregs[xstate_size_n_of_int64];
>> >> + ?struct iovec iov;
>> >> +
>> >> + ?if (!have_ptrace_getregset)
>> >> + ? ?return 0;
>> >> +
>> >> + ?iov.iov_base = xstateregs;
>> >> + ?iov.iov_len = xstate_size;
>> >> + ?if (ptrace (PTRACE_GETREGSET, tid, (unsigned int) NT_X86_XSTATE,
>> >> + ? ? ? ? ? (int) &iov) < 0)
>> >
>> > This can't be right!
>>
>> Why? That is the kernel interface in 2.6.34-rc1.
>
> Well, at least your usage of casts here and further on in the code is
> inconsistent. ?But casting a pointer to an int acts as a red flag to
> me. ?Given that the userland prototype for ptrace(2) is:
>
> extern long int ptrace (enum __ptrace_request __request, ...) __THROW;
>
> I believe those casts shouldn't be necessary.

I will change it.

>> >> + ? ?perror_with_name (_("Couldn't read extended state status"));
>> >> +
>> >> + ?i387_supply_xsave (regcache, -1, xstateregs);
>> >> + ?return 1;
>> >> +}
>> >> +
>> >> +/* Store all valid registers in GDB's register array covered by the
>> >> + ? PTRACE_SETREGSET request into the process/thread specified by TID.
>> >> + ? Return non-zero if successful, zero otherwise. ?*/
>> >> +
>> >> +static int
>> >> +store_xstateregs (const struct regcache *regcache, int tid, int regno)
>> >> +{
>> >> + ?unsigned long long xstateregs[xstate_size_n_of_int64];
>> >
>> > I think it is better to use I386_XSTATE_MAX_SIZE here.
>>
>> That is how the kernel interface works. ?Whatever value
>> I386_XSTATE_MAX_SIZE is today won't be the same tomorrow. We will
>> increase it in the coming years. But the same gdb binary will work
>> fine since kernel will only copy number of bytes specified in
>> iov.iov_len, which is all gdb cares/needs.
>
> Yes, you'll need to raise I386_XSTATE_MAX_SIZE whenever the kernel
> gains support for different/larger xstates. ?But I don't see a problem
> with that, since you'll have to make changes to GDB to support those
> variants anyway. ?That reminds me:

I will remove I386_XSTATE_MAX_SIZE since it isn't needed by kernel.

>> >> + ?struct iovec iov;
>> >> +
>> >> + ?if (!have_ptrace_getregset)
>> >> + ? ?return 0;
>> >> +
>> >> + ?iov.iov_base = xstateregs;
>> >> + ?iov.iov_len = xstate_size;
>
> You probably should set iov.iov_len to sizeof(xstateregs) here.

I will make the change.

>> >> ? ? ? ?if (store_fpxregs (regcache, tid, regno))
>> >> @@ -858,7 +943,49 @@ i386_linux_child_post_startup_inferior (ptid_t ptid)
>> >> ?static const struct target_desc *
>> >> ?i386_linux_read_description (struct target_ops *ops)
>> >> ?{
>> >> - ?return tdesc_i386_linux;
>> >> + ?static unsigned long long xcr0;
>> >
>> > Is it really ok, to cache this? ?Will the Linux kernel always return
>> > the same value for every process?
>>
>> xcr0 is a processor value and will be the same for all processes.
>
> ok; but could you change this to uint64_t?
>

I will make the change.

Thanks.


-- 
H.J.