Re: [PATCH v2 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989)

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

On 11/6/23 08:27, Adhemerval Zanella wrote:
> 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.  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.
> 
> Checked on i686-linux-gnu.

LGTM.

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

> ---
>  math/test-fesetexcept-traps.c            | 28 ++++++++++++---
>  sysdeps/i386/fpu/fesetexcept.c           | 46 +++++++++++++++++++++---
>  sysdeps/i386/fpu/math-tests-trap-force.h | 29 +++++++++++++++
>  sysdeps/x86/fpu/test-fenv-sse-2.c        | 23 +++---------
>  4 files changed, 100 insertions(+), 26 deletions(-)
>  create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h
> 
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 96f6c4752f..8a5c0bca80 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-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)
> @@ -41,8 +42,28 @@ do_test (void)
>  
>    /* Verify fesetexcept does not cause exception traps.  For architectures
>       where setting the exception might result in traps the function should
> -     return a nonzero value.  */
> -  ret = fesetexcept (FE_ALL_EXCEPT);
> +     return a nonzero value.
> +     Also check if the function does not alter the exception mask.  */
> +  {
> +    int exc_before = fegetexcept ();
> +    ret = fesetexcept (FE_ALL_EXCEPT);
> +    int exc_after = fegetexcept ();
> +    if (exc_before != exc_after)
> +      {
> +	puts ("fesetexcept (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.  */

Correct, this is called delayed floating point exception handling and hppa has this too.

> +  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);
> +
>    if (ret == 0)
>      puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>    else if (!EXCEPTION_SET_FORCES_TRAP)
> @@ -61,5 +82,4 @@ do_test (void)
>    return result;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
> index 18949e982a..58f577d93d 100644
> --- a/sysdeps/i386/fpu/fesetexcept.c
> +++ b/sysdeps/i386/fpu/fesetexcept.c
> @@ -17,15 +17,53 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <fenv.h>
> +#include <ldsodefs.h>
>  
>  int
>  fesetexcept (int excepts)
>  {
> -  fenv_t temp;
> +  /* The flags can be set in the 387 unit or in the SSE unit.  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.  */
>  
> -  __asm__ ("fnstenv %0" : "=m" (*&temp));
> -  temp.__status_word |= excepts & FE_ALL_EXCEPT;
> -  __asm__ ("fldenv %0" : : "m" (*&temp));
> +  excepts &= FE_ALL_EXCEPT;
> +
> +  if (CPU_FEATURE_USABLE (SSE))
> +    {
> +      /* Get the control word of the SSE unit.  */
> +      unsigned int mxcsr;
> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
> +
> +      /* Set relevant flags.  */
> +      mxcsr |= excepts;
> +
> +      /* Put the new data in effect.  */
> +      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
> +    }
> +  else
> +    {
> +      fenv_t temp;
> +
> +      /* Note: fnstenv masks all floating-point exceptions until the fldenv
> +	 or fldcw below.  */
> +      __asm__ ("fnstenv %0" : "=m" (*&temp));
> +
> +      /* Set relevant flags.  */
> +      temp.__status_word |= excepts;
> +
> +      if ((~temp.__control_word) & excepts)
> +	{
> +	  /* Setting the exception flags may trigger a trap (at the next
> +	     floating-point instruction, but that does not matter).
> +	     ISO C23 (7.6.4.4) 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));
> +    }
>  
>    return 0;
>  }
> diff --git a/sysdeps/i386/fpu/math-tests-trap-force.h b/sysdeps/i386/fpu/math-tests-trap-force.h
> new file mode 100644
> index 0000000000..f41e1ffc2d
> --- /dev/null
> +++ b/sysdeps/i386/fpu/math-tests-trap-force.h
> @@ -0,0 +1,29 @@
> +/* Configuration for math tests: support for setting exception flags
> +   without causing enabled traps.  i686 version.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef I386_FPU_MATH_TESTS_TRAP_FORCE_H
> +#define I386_FPU_MATH_TESTS_TRAP_FORCE_H 1
> +
> +#include <cpu-features.h>
> +
> +/* Setting exception flags in FPU Status Register results in enabled traps for
> +   those exceptions being taken.  */
> +#define EXCEPTION_SET_FORCES_TRAP !CPU_FEATURE_USABLE (SSE)
> +
> +#endif /* math-tests-trap-force.h.  */
> diff --git a/sysdeps/x86/fpu/test-fenv-sse-2.c b/sysdeps/x86/fpu/test-fenv-sse-2.c
> index f3e820b6ed..7a0503790f 100644
> --- a/sysdeps/x86/fpu/test-fenv-sse-2.c
> +++ b/sysdeps/x86/fpu/test-fenv-sse-2.c
> @@ -22,17 +22,8 @@
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdio.h>
> -
> -static bool
> -have_sse2 (void)
> -{
> -  unsigned int eax, ebx, ecx, edx;
> -
> -  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> -    return false;
> -
> -  return (edx & bit_SSE2) != 0;
> -}
> +#include <cpu-features.h>
> +#include <support/check.h>
>  
>  static uint32_t
>  get_sse_mxcsr (void)
> @@ -164,13 +155,9 @@ sse_tests (void)
>  static int
>  do_test (void)
>  {
> -  if (!have_sse2 ())
> -    {
> -      puts ("CPU does not support SSE2, cannot test");
> -      return 0;
> -    }
> +  if (!CPU_FEATURE_USABLE (SSE2))
> +    FAIL_UNSUPPORTED ("CPU does not support SSE2");
>    return sse_tests ();
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include <test-skeleton.c>
> +#include <support/test-driver.c>

-- 
Cheers,
Carlos.