diff mbox series

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

Message ID 20231106132713.953501-3-adhemerval.zanella@linaro.org
State New
Headers show
Series Multiple floating-point environment fixes | expand

Commit Message

Adhemerval Zanella Netto Nov. 6, 2023, 1:27 p.m. UTC
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.
---
 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

Comments

Carlos O'Donell Nov. 6, 2023, 4:14 p.m. UTC | #1
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>
diff mbox series

Patch

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.  */
+  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>