Re: Include SSE state in i386 fenv_t (bug 16064)

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

On Thu, May 8, 2014 at 4:01 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> This patch fixes bug 16064, i386 fenv_t not including SSE state, using
> the technique suggested there of storing the state in the existing
> __eip field of fenv_t to avoid needing to increase the size of fenv_t
> and add new symbol versions.  The included testcase, which previously
> failed for i386 (but passed for x86_64), illustrates how the previous
> state was buggy.
>
> This patch causes the SSE state to be included *to the extent it is on
> x86_64*.  Where some state should logically be included but isn't for
> x86_64 (see bug 16068), this patch does not cause it to be included
> for i386 either.  The idea is that any patch fixing that bug should
> fix it for both x86_64 and i386 at once.
>
> Tested i386 and x86_64.  (I haven't tested the case of a CPU without
> SSE2 disabling the test.)
>
> 2014-05-08  Joseph Myers  <joseph@codesourcery.com>
>
>         [BZ #16064]
>         * sysdeps/i386/fpu/fegetenv.c: Include <unistd.h>, <ldsodefs.h>
>         and <dl-procinfo.h>.
>         (__fegetenv): Save SSE state in envp->__eip if supported.
>         * sysdeps/i386/fpu/feholdexcpt.c (feholdexcept): Save SSE state in
>         envp->__eip if supported.
>         * sysdeps/i386/fpu/fesetenv.c: Include <unistd.h>, <ldsodefs.h>
>         and <dl-procinfo.h>.
>         (__fesetenv): Do not use envp->__eip to set eip in floating-point
>         environment.  Set SSE state if supported.
>         * sysdeps/x86/fpu/Makefile [$(subdir) = math] (tests): Add
>         test-fenv-sse.
>         [$(subdir) = math] (CFLAGS-test-fenv-sse.c): Add -msse2
>         -mfpmath=sse.
>         * sysdeps/x86/fpu/test-fenv-sse.c: New file.
>
> diff --git a/sysdeps/i386/fpu/fegetenv.c b/sysdeps/i386/fpu/fegetenv.c
> index 8dbdb57..8c45b6b 100644
> --- a/sysdeps/i386/fpu/fegetenv.c
> +++ b/sysdeps/i386/fpu/fegetenv.c
> @@ -18,6 +18,9 @@
>     <http://www.gnu.org/licenses/>.  */
>
>  #include <fenv.h>
> +#include <unistd.h>
> +#include <ldsodefs.h>
> +#include <dl-procinfo.h>
>
>  int
>  __fegetenv (fenv_t *envp)
> @@ -28,6 +31,9 @@ __fegetenv (fenv_t *envp)
>       would block all exceptions.  */
>    __asm__ ("fldenv %0" : : "m" (*envp));
>
> +  if ((GLRO(dl_hwcap) & HWCAP_I386_XMM) != 0)
> +    __asm__ ("stmxcsr %0" : "=m" (envp->__eip));
> +
>    /* Success.  */
>    return 0;
>  }
> diff --git a/sysdeps/i386/fpu/feholdexcpt.c b/sysdeps/i386/fpu/feholdexcpt.c
> index d475ca8..dc9d703 100644
> --- a/sysdeps/i386/fpu/feholdexcpt.c
> +++ b/sysdeps/i386/fpu/feholdexcpt.c
> @@ -35,10 +35,10 @@ feholdexcept (fenv_t *envp)
>        unsigned int xwork;
>
>        /* Get the current control word.  */
> -      __asm__ ("stmxcsr %0" : "=m" (*&xwork));
> +      __asm__ ("stmxcsr %0" : "=m" (envp->__eip));
>
>        /* Set all exceptions to non-stop and clear them.  */
> -      xwork = (xwork | 0x1f80) & ~0x3f;
> +      xwork = (envp->__eip | 0x1f80) & ~0x3f;
>
>        __asm__ ("ldmxcsr %0" : : "m" (*&xwork));
>      }
> diff --git a/sysdeps/i386/fpu/fesetenv.c b/sysdeps/i386/fpu/fesetenv.c
> index 95b2f0a..3eb0de0 100644
> --- a/sysdeps/i386/fpu/fesetenv.c
> +++ b/sysdeps/i386/fpu/fesetenv.c
> @@ -19,6 +19,9 @@
>
>  #include <fenv.h>
>  #include <assert.h>
> +#include <unistd.h>
> +#include <ldsodefs.h>
> +#include <dl-procinfo.h>
>
>
>  int
> @@ -63,7 +66,7 @@ __fesetenv (const fenv_t *envp)
>                               & (FE_ALL_EXCEPT | FE_TOWARDZERO));
>        temp.__status_word &= ~FE_ALL_EXCEPT;
>        temp.__status_word |= envp->__status_word & FE_ALL_EXCEPT;
> -      temp.__eip = envp->__eip;
> +      temp.__eip = 0;
>        temp.__cs_selector = envp->__cs_selector;
>        temp.__opcode = envp->__opcode;

Should we also set __cs_selector/__opcode/__data_offset/__data_selector
to 0?

-- 
H.J.