Re: [PATCH v2 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)

[Carlos O'Donell <carlos@redhat.com>]

On 11/6/23 08:27, Adhemerval Zanella wrote:
> From: Bruno Haible <bruno@clisp.org>
> 
> According to ISO C23 (7.6.4.4), fesetexcept is supposed to set
> floating-point exception flags without raising a trap (unlike
> feraiseexcept, which is supposed to raise a trap if feenableexcept
> was called with the appropriate argument).
> 
> The flags can be set in the 387 unit or in the SSE unit.  When we need
> to clear a flag, we need to do so in both units, due to the way
> fetestexcept is implemented.
> 
> When we need to set a flag, it is sufficient to do it in the SSE unit,
> because that is guaranteed to not trap.  However, on i386 CPUs that have
> only a 387 unit, set the flags in the 387, as long as this cannot trap.
> 

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> ---
>  math/test-fexcept-traps.c         | 25 +++++++++++-
>  sysdeps/i386/fpu/fsetexcptflg.c   | 63 ++++++++++++++++++++-----------
>  sysdeps/x86_64/fpu/fsetexcptflg.c | 24 +++++++-----
>  3 files changed, 79 insertions(+), 33 deletions(-)
> 
> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
> index 9b8f583ae6..6bfb5124da 100644
> --- a/math/test-fexcept-traps.c
> +++ b/math/test-fexcept-traps.c
> @@ -19,6 +19,7 @@
>  #include <fenv.h>
>  #include <stdio.h>
>  #include <math-tests.h>
> +#include <math-barriers.h>
>  
>  static int
>  do_test (void)
> @@ -65,8 +66,28 @@ do_test (void)
>  
>    /* The test is that this does not cause exception traps.  For architectures
>       where setting the exception might result in traps the function should
> -     return a nonzero value.  */
> -  ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
> +     return a nonzero value.
> +     Also check if the function does not alter the exception mask.  */
> +  {
> +    int exc_before = fegetexcept ();
> +    ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
> +    int exc_after = fegetexcept ();
> +    if (exc_before != exc_after)
> +      {
> +	puts ("fesetexceptflag (FE_ALL_EXCEPT) changed the exceptions mask");
> +	return 1;
> +      }
> +  }
> +
> +  /* Execute some floating-point operations, since on some CPUs exceptions
> +     triggers a trap only at the next floating-point instruction.  */
> +  volatile double a = 1.0;
> +  volatile double b = a + a;
> +  math_force_eval (b);
> +  volatile long double al = 1.0L;
> +  volatile long double bl = al + al;
> +  math_force_eval (bl);

OK.

> +
>    if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP)
>      {
>        puts ("fesetexceptflag failed");
> diff --git a/sysdeps/i386/fpu/fsetexcptflg.c b/sysdeps/i386/fpu/fsetexcptflg.c
> index e724b7d6fd..480165cff9 100644
> --- a/sysdeps/i386/fpu/fsetexcptflg.c
> +++ b/sysdeps/i386/fpu/fsetexcptflg.c
> @@ -17,42 +17,63 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <fenv.h>
> -#include <math.h>
> -#include <unistd.h>
>  #include <ldsodefs.h>
> -#include <dl-procinfo.h>
>  
>  int
>  __fesetexceptflag (const fexcept_t *flagp, int excepts)
>  {
> -  fenv_t temp;
> +  /* The flags can be set in the 387 unit or in the SSE unit.  When we need to
> +     clear a flag, we need to do so in both units, due to the way fetestexcept
> +     is implemented.
> +     When we need to set a flag, it is sufficient to do it in the SSE unit,
> +     because that is guaranteed to not trap.  However, on i386 CPUs that have
> +     only a 387 unit, set the flags in the 387, as long as this cannot trap.  */
>  
> -  /* Get the current environment.  We have to do this since we cannot
> -     separately set the status word.  */
> -  __asm__ ("fnstenv %0" : "=m" (*&temp));
> +  fenv_t temp;
>  
> -  temp.__status_word &= ~(excepts & FE_ALL_EXCEPT);
> -  temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT;
> +  excepts &= FE_ALL_EXCEPT;
>  
> -  /* Store the new status word (along with the rest of the environment.
> -     Possibly new exceptions are set but they won't get executed unless
> -     the next floating-point instruction.  */
> -  __asm__ ("fldenv %0" : : "m" (*&temp));
> +  /* Get the current x87 FPU environment.  We have to do this since we
> +     cannot separately set the status word.
> +     Note: fnstenv masks all floating-point exceptions until the fldenv
> +     or fldcw below.  */
> +  __asm__ ("fnstenv %0" : "=m" (*&temp));
>  
> -  /* If the CPU supports SSE, we set the MXCSR as well.  */
>    if (CPU_FEATURE_USABLE (SSE))
>      {
> -      unsigned int xnew_exc;
> +      unsigned int mxcsr;
> +
> +      /* Clear relevant flags.  */
> +      temp.__status_word &= ~(excepts & ~ *flagp);
>  
> -      /* Get the current MXCSR.  */
> -      __asm__ ("stmxcsr %0" : "=m" (*&xnew_exc));
> +      /* Store the new status word (along with the rest of the environment).  */
> +      __asm__ ("fldenv %0" : : "m" (*&temp));
>  
> -      /* Set the relevant bits.  */
> -      xnew_exc &= ~(excepts & FE_ALL_EXCEPT);
> -      xnew_exc |= *flagp & excepts & FE_ALL_EXCEPT;
> +      /* And now similarly for SSE.  */
> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
> +
> +      /* Clear or set relevant flags.  */
> +      mxcsr ^= (mxcsr ^ *flagp) & excepts;
>  
>        /* Put the new data in effect.  */
> -      __asm__ ("ldmxcsr %0" : : "m" (*&xnew_exc));
> +      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
> +    }
> +  else
> +    {
> +      /* Clear or set relevant flags.  */
> +      temp.__status_word ^= (temp.__status_word ^ *flagp) & excepts;
> +
> +      if ((~temp.__control_word) & temp.__status_word & excepts)
> +	{
> +	  /* Setting the exception flags may trigger a trap (at the next
> +	     floating-point instruction, but that does not matter).
> +	     ISO C 23 § 7.6.4.5 does not allow it.  */
> +	  __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
> +	  return -1;
> +	}
> +
> +      /* Store the new status word (along with the rest of the environment).  */
> +      __asm__ ("fldenv %0" : : "m" (*&temp));
>      }
>  
>    /* Success.  */
> diff --git a/sysdeps/x86_64/fpu/fsetexcptflg.c b/sysdeps/x86_64/fpu/fsetexcptflg.c
> index a3ac1dea01..2ce2b509f2 100644
> --- a/sysdeps/x86_64/fpu/fsetexcptflg.c
> +++ b/sysdeps/x86_64/fpu/fsetexcptflg.c
> @@ -22,30 +22,34 @@
>  int
>  fesetexceptflag (const fexcept_t *flagp, int excepts)
>  {
> +  /* The flags can be set in the 387 unit or in the SSE unit.
> +     When we need to clear a flag, we need to do so in both units,
> +     due to the way fetestexcept() is implemented.
> +     When we need to set a flag, it is sufficient to do it in the SSE unit,
> +     because that is guaranteed to not trap.  */
> +
>    fenv_t temp;
>    unsigned int mxcsr;
>  
> -  /* XXX: Do we really need to set both the exception in both units?
> -     Shouldn't it be enough to set only the SSE unit?  */
> +  excepts &= FE_ALL_EXCEPT;
>  
>    /* Get the current x87 FPU environment.  We have to do this since we
>       cannot separately set the status word.  */
>    __asm__ ("fnstenv %0" : "=m" (*&temp));
>  
> -  temp.__status_word &= ~(excepts & FE_ALL_EXCEPT);
> -  temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT;
> +  /* Clear relevant flags.  */
> +  temp.__status_word &= ~(excepts & ~ *flagp);
>  
> -  /* Store the new status word (along with the rest of the environment.
> -     Possibly new exceptions are set but they won't get executed unless
> -     the next floating-point instruction.  */
> +  /* Store the new status word (along with the rest of the environment).  */
>    __asm__ ("fldenv %0" : : "m" (*&temp));
>  
> -  /* And now the same for SSE.  */
> +  /* And now similarly for SSE.  */
>    __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
>  
> -  mxcsr &= ~(excepts & FE_ALL_EXCEPT);
> -  mxcsr |= *flagp & excepts & FE_ALL_EXCEPT;
> +  /* Clear or set relevant flags.  */
> +  mxcsr ^= (mxcsr ^ *flagp) & excepts;
>  
> +  /* Put the new data in effect.  */
>    __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
>  
>    /* Success.  */

-- 
Cheers,
Carlos.