This is the mail archive of the gdb@sourceware.cygnus.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1


Mark Kettenis wrote:
> 
> Sorry, but I don't think this is right:
> 
>  #define save_fpu(tsk) do { \
> -       asm volatile("fxsave %0 ; fwait" \
> -                    : "=m" (tsk->thread.i387.hard)); \
> +       asm volatile("fnstenv %0 ; fxsave %1 ; fwait" \
> +                    : "=m" (tsk->thread.i387.hard), \
> +                      "=m" (tsk->thread.i387.hard.fxsr_space[0])); \
> 
> If I'm not mistaken this means that you've added an additional
> overhead on *every* task switch between FPU-using tasks.  Instead you
> should only convert to the old fsave format when that's actually
> needed:
> 
>  1. When setting up a signal context for invoking a signal handler.
> 
>  2. For the PTRACE_GETFPREGS request.

What happens when a signal occurs in a time slice where the application
hasn't used the FPU?  Having the regular FPU environment around means we
don't have to do the conversion between formats, and don't have to
convert the regular format back into the FXSAVE format when the signal
handler returns.  This ensures current signal handlers that change the
FPU state with the old sigcontext will be guaranteed to work correctly.

Linus has said that he doesn't want to convert between the formats
manually, and this is a nice way to not have to do that.

> I also don't understand why you expose the strange user_xfpregs_struct
> format via the PTRACE_{GET,SET}XFPREGS requests.  Why don't you just
> provide the 512-byte FXSAVE area?

Again, so we don't have to convert between formats.  This structure is
taken from a patch to get SSE working with GDB 5.0, but I guess the raw
FXSAVE data will do just fine.  I didn't really like this either.  I
will change this back.

> All in all, I think that the origional code in 2.4.0test1 is much
> closer to what the endresult should look like.  Just add the
> PTRACE_{GET,SET}XFPREGS requests that return, write the FXSAVE data
> (should be *very* simple), add the necessary bits for setting up the
> signal frame (more or less what's in your patch, but this is the place
> where the FXSAVE -> FSAVE conversion should be done), and add some
> code to the core dumping code to add an extra note with the FXSAVE
> area.

Okay, I tried too hard to please everyone :-)

I'll play around with the conversion into/out of the signal handler and
GETFPREGS request.  Should only take a short while, so don't do anything
with the stuff I sent out yesterday.

-- Gareth

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]