[PATCH 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86

[PATCH 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86

[Adhemerval Zanella]

Bruno has found fesetexcept/fesetexceptflag wrongly raise floating-point
exception flags on x86 and powerpc.

Adhemerval Zanella (2):
  powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag
    (BZ 30988)
  i686: Do not raise exception traps on fesetexcept (BZ 30989)

Bruno Haible (1):
  x86: Do not raises floating-point exception traps on fesetexceptflag
    (BZ 30990)

 math/test-fesetexcept-traps.c            | 22 ++++++---
 math/test-fexcept-traps.c                | 22 ++++++---
 sysdeps/i386/fpu/fesetexcept.c           | 43 +++++++++++++++++-
 sysdeps/i386/fpu/fsetexcptflg.c          | 58 +++++++++++++++---------
 sysdeps/i386/fpu/math-tests-trap-force.h | 29 ++++++++++++
 sysdeps/powerpc/fpu/fesetexcept.c        |  5 ++
 sysdeps/powerpc/fpu/fsetexcptflg.c       |  9 +++-
 sysdeps/x86_64/fpu/fsetexcptflg.c        | 24 ++++++----
 8 files changed, 164 insertions(+), 48 deletions(-)
 create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h

-- 
2.34.1

[PATCH 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)

[Adhemerval Zanella]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 4108 bytes --]

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).

This is a side-effect of how we implement the GNU extension
feenableexcept, where feenableexcept/fesetenv/fesetmode/feupdateenv
might issue prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE) depending of the
argument.  And on PR_FP_EXC_PRECISE, setting a floating-point exception
flag triggers a trap.

To make the both functions follow the C23, fesetexcept and
fesetexceptflag now fail if the argument may trigger a trap.

The math tests now check for an value different than 0, instead
of bail out as unsupported for EXCEPTION_SET_FORCES_TRAP.

Checked on powerpc64le-linux-gnu.
---
 math/test-fesetexcept-traps.c      | 11 ++++-------
 math/test-fexcept-traps.c          | 11 ++++-------
 sysdeps/powerpc/fpu/fesetexcept.c  |  5 +++++
 sysdeps/powerpc/fpu/fsetexcptflg.c |  9 ++++++++-
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
index 71b6e45b33..96f6c4752f 100644
--- a/math/test-fesetexcept-traps.c
+++ b/math/test-fesetexcept-traps.c
@@ -39,16 +39,13 @@ do_test (void)
       return result;
     }
 
-  if (EXCEPTION_SET_FORCES_TRAP)
-    {
-      puts ("setting exceptions traps, cannot test on this architecture");
-      return 77;
-    }
-  /* Verify fesetexcept does not cause exception traps.  */
+  /* 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);
   if (ret == 0)
     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
-  else
+  else if (!EXCEPTION_SET_FORCES_TRAP)
     {
       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
       if (EXCEPTION_TESTS (float))
diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
index 9701c3c320..9b8f583ae6 100644
--- a/math/test-fexcept-traps.c
+++ b/math/test-fexcept-traps.c
@@ -63,14 +63,11 @@ do_test (void)
       result = 1;
     }
 
-  if (EXCEPTION_SET_FORCES_TRAP)
-    {
-      puts ("setting exceptions traps, cannot test on this architecture");
-      return 77;
-    }
-  /* The test is that this does not cause exception traps.  */
+  /* 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);
-  if (ret != 0)
+  if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP)
     {
       puts ("fesetexceptflag failed");
       result = 1;
diff --git a/sysdeps/powerpc/fpu/fesetexcept.c b/sysdeps/powerpc/fpu/fesetexcept.c
index 609a148a95..2850156d3a 100644
--- a/sysdeps/powerpc/fpu/fesetexcept.c
+++ b/sysdeps/powerpc/fpu/fesetexcept.c
@@ -31,6 +31,11 @@ fesetexcept (int excepts)
 	    & FE_INVALID_SOFTWARE));
   if (n.l != u.l)
     {
+      if (n.l & fenv_exceptions_to_reg (excepts))
+	/* Setting the exception flags may trigger a trap.  ISO C 23 § 7.6.4.4
+	    does not allow it.   */
+	return -1;
+
       fesetenv_register (n.fenv);
 
       /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips.  */
diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c
index 2b22f913c0..6517e8ea03 100644
--- a/sysdeps/powerpc/fpu/fsetexcptflg.c
+++ b/sysdeps/powerpc/fpu/fsetexcptflg.c
@@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
      This may cause floating-point exceptions if the restored state
      requests it.  */
   if (n.l != u.l)
-    fesetenv_register (n.fenv);
+    {
+      if (n.l & fenv_exceptions_to_reg (excepts))
+	/* Setting the exception flags may trigger a trap.  ISO C 23 § 7.6.4.4
+	    does not allow it.   */
+	return -1;
+
+      fesetenv_register (n.fenv);
+    }
 
   /* Deal with FE_INVALID_SOFTWARE not being implemented on some chips.  */
   if (flag & FE_INVALID)
-- 
2.34.1

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

[Adhemerval Zanella]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 5126 bytes --]

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            | 11 ++++++
 sysdeps/i386/fpu/fesetexcept.c           | 43 ++++++++++++++++++++++--
 sysdeps/i386/fpu/math-tests-trap-force.h | 29 ++++++++++++++++
 3 files changed, 81 insertions(+), 2 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..122c23eb7e 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)
@@ -43,6 +44,16 @@ do_test (void)
      where setting the exception might result in traps the function should
      return a nonzero value.  */
   ret = fesetexcept (FE_ALL_EXCEPT);
+
+  /* Execute some floating-point operations, since on some CPUs exceptions
+     triggers a trap only at the next floating-point instruction.  */
+  double a = 1.0;
+  double b = a + a;
+  math_force_eval (b);
+  long double al = 1.0L;
+  long double bl = al + al;
+  math_force_eval (bl);
+
   if (ret == 0)
     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
   else if (!EXCEPTION_SET_FORCES_TRAP)
diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
index 18949e982a..a4c70cd1d1 100644
--- a/sysdeps/i386/fpu/fesetexcept.c
+++ b/sysdeps/i386/fpu/fesetexcept.c
@@ -17,15 +17,54 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <fenv.h>
+#include <ldsodefs.h>
 
 int
 fesetexcept (int excepts)
 {
+  /* 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.  */
+
   fenv_t temp;
 
+  excepts &= FE_ALL_EXCEPT;
+
   __asm__ ("fnstenv %0" : "=m" (*&temp));
-  temp.__status_word |= excepts & FE_ALL_EXCEPT;
-  __asm__ ("fldenv %0" : : "m" (*&temp));
+
+  if (CPU_FEATURE_USABLE (SSE))
+    {
+      /* Clear relevant flags.  */
+      temp.__status_word &= ~excepts;
+
+      /* Store the new status word (along with the rest of the environment).  */
+      __asm__ ("fldenv %0" : : "m" (*&temp));
+
+      /* And now similarly for SSE.  */
+      unsigned int mxcsr;
+      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
+
+      /* Set relevant flags.  */
+      mxcsr |= excepts & FE_ALL_EXCEPT;
+
+      /* Put the new data in effect.  */
+      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
+    }
+  else
+    {
+      /* Clear or set relevant flags.  */
+      temp.__status_word ^= temp.__status_word & 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.  */
+        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..d88229c271
--- /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 FPSCR results in enabled traps for those
+   exceptions being taken.  */
+#define EXCEPTION_SET_FORCES_TRAP (CPU_FEATURE_USABLE (SSE))
+
+#endif /* math-tests-trap-force.h.  */
-- 
2.34.1

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

[Adhemerval Zanella]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6695 bytes --]

From: Bruno Haible <bruno@clisp.org>

According to ISO C23 (7.6.4.5), fesetexceptflag is supposed to set
floating-point exception flags without raising a trap.

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.

Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
---
 math/test-fexcept-traps.c         | 11 ++++++
 sysdeps/i386/fpu/fsetexcptflg.c   | 58 ++++++++++++++++++++-----------
 sysdeps/x86_64/fpu/fsetexcptflg.c | 24 +++++++------
 3 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
index 9b8f583ae6..a486d17951 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)
@@ -67,6 +68,16 @@ do_test (void)
      where setting the exception might result in traps the function should
      return a nonzero value.  */
   ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
+
+  /* Execute some floating-point operations, since on some CPUs exceptions
+     triggers a trap only at the next floating-point instruction.  */
+  double a = 1.0;
+  double b = a + a;
+  math_force_eval (b);
+  long double al = 1.0L;
+  long double bl = al + al;
+  math_force_eval (bl);
+
   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..ccbcf35e8e 100644
--- a/sysdeps/i386/fpu/fsetexcptflg.c
+++ b/sysdeps/i386/fpu/fsetexcptflg.c
@@ -17,42 +17,58 @@
    <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.  */
+  __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.  */
+        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.  */
-- 
2.34.1

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

[Joseph Myers]

On Mon, 23 Oct 2023, Adhemerval Zanella wrote:

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

Don't you need to check (~temp.__control_word), as in patch 3, since the 
bits in the control word are set for masked exceptions, clear for 
trapping?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)

[Bruno Haible]

Adhemerval Zanella wrote:
> diff --git a/sysdeps/powerpc/fpu/fsetexcptflg.c b/sysdeps/powerpc/fpu/fsetexcptflg.c
> index 2b22f913c0..6517e8ea03 100644
> --- a/sysdeps/powerpc/fpu/fsetexcptflg.c
> +++ b/sysdeps/powerpc/fpu/fsetexcptflg.c
> @@ -44,7 +44,14 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
>       This may cause floating-point exceptions if the restored state
>       requests it.  */
>    if (n.l != u.l)
> -    fesetenv_register (n.fenv);
> +    {
> +      if (n.l & fenv_exceptions_to_reg (excepts))
> +	/* Setting the exception flags may trigger a trap.  ISO C 23 § 7.6.4.4
> +	    does not allow it.   */

Here, in fsetexcptflg.c, the comment should reference § 7.6.4.5, not § 7.6.4.4.

Bruno

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

[Bruno Haible]

Adhemerval Zanella wrote:
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 96f6c4752f..122c23eb7e 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-traps.c
> @@ -43,6 +44,16 @@ do_test (void)
>       where setting the exception might result in traps the function should
>       return a nonzero value.  */
>    ret = fesetexcept (FE_ALL_EXCEPT);
> +
> +  /* Execute some floating-point operations, since on some CPUs exceptions
> +     triggers a trap only at the next floating-point instruction.  */
> +  double a = 1.0;
> +  double b = a + a;
> +  math_force_eval (b);
> +  long double al = 1.0L;
> +  long double bl = al + al;

I would mark the variables a, b, al, bl as 'volatile', otherwise it's too
easy for GCC to do constant-folding and thus eliminate the floating-point
operations.

> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
> index 18949e982a..a4c70cd1d1 100644
> --- a/sysdeps/i386/fpu/fesetexcept.c
> +++ b/sysdeps/i386/fpu/fesetexcept.c
> @@ -17,15 +17,54 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <fenv.h>
> +#include <ldsodefs.h>
>  
>  int
>  fesetexcept (int excepts)
>  {
> +  /* 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.  */
> +
>    fenv_t temp;
>  
> +  excepts &= FE_ALL_EXCEPT;
> +
>    __asm__ ("fnstenv %0" : "=m" (*&temp));

The variable 'temp' and this __asm__ statement can be moved to the 'else'
branch, since in the case that an SSE unit is present, we don't need to
touch the 387 unit at all. (Since the job here is to _set_ some exception
flag bits.)

> +  if (CPU_FEATURE_USABLE (SSE))
> +    {
> +      /* Clear relevant flags.  */
> +      temp.__status_word &= ~excepts;
> +
> +      /* Store the new status word (along with the rest of the environment).  */
> +      __asm__ ("fldenv %0" : : "m" (*&temp));
> +

These last two statements can be removed, since in this case, we don't need to
touch the 387 unit at all.

> +      /* And now similarly for SSE.  */
> +      unsigned int mxcsr;
> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
> +
> +      /* Set relevant flags.  */
> +      mxcsr |= excepts & FE_ALL_EXCEPT;

No need for the ' & FE_ALL_EXCEPT' here, since it was already done at function
entry.

> +      /* Put the new data in effect.  */
> +      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
> +    }
> +  else
> +    {
> +      /* Clear or set relevant flags.  */
> +      temp.__status_word ^= temp.__status_word & excepts;

This last statement is not right: It clears bits from temp.__status_word,
but should *set* these bits instead. Change this to:

         /* Set relevant flags.  */
         temp.__status_word |= excepts;

> +      if (temp.__control_word & temp.__status_word & excepts)

The temp.__status_word does not need to be tested here, since we just
set all EXCEPTS bit in it just before. With Joseph's remark, this line
becomes

         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 C 23 § 7.6.4.5 does not allow it.  */

In this function, we need to reference § 7.6.4.4.

> 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..d88229c271
> --- /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 FPSCR results in enabled traps for those
> +   exceptions being taken.  */

The i386 does not have an FPSCR register. The exception flags are stored
in the register that gdb calls 'fstat' instead. I would use the same name.
(The Intel reference does not have a short name for this register; it
calls it "FPU Status Register".)

Bruno

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

[Bruno Haible]

Adhemerval Zanella wrote:
> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
> index 9b8f583ae6..a486d17951 100644
> --- a/math/test-fexcept-traps.c
> +++ b/math/test-fexcept-traps.c
> @@ -67,6 +68,16 @@ do_test (void)
>       where setting the exception might result in traps the function should
>       return a nonzero value.  */
>    ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
> +
> +  /* Execute some floating-point operations, since on some CPUs exceptions
> +     triggers a trap only at the next floating-point instruction.  */
> +  double a = 1.0;
> +  double b = a + a;
> +  math_force_eval (b);
> +  long double al = 1.0L;
> +  long double bl = al + al;

Like in [2/3], I would mark the variables a, b, al, bl as 'volatile',
otherwise it's too easy for GCC to do constant-folding and thus eliminate
the floating-point operations.

Bruno

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

[Adhemerval Zanella Netto]

On 23/10/23 21:17, Bruno Haible wrote:
> Adhemerval Zanella wrote:
>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
>> index 96f6c4752f..122c23eb7e 100644
>> --- a/math/test-fesetexcept-traps.c
>> +++ b/math/test-fesetexcept-traps.c
>> @@ -43,6 +44,16 @@ do_test (void)
>>       where setting the exception might result in traps the function should
>>       return a nonzero value.  */
>>    ret = fesetexcept (FE_ALL_EXCEPT);
>> +
>> +  /* Execute some floating-point operations, since on some CPUs exceptions
>> +     triggers a trap only at the next floating-point instruction.  */
>> +  double a = 1.0;
>> +  double b = a + a;
>> +  math_force_eval (b);
>> +  long double al = 1.0L;
>> +  long double bl = al + al;
> 
> I would mark the variables a, b, al, bl as 'volatile', otherwise it's too
> easy for GCC to do constant-folding and thus eliminate the floating-point
> operations.

It should not matter for i386, since it still generates a floating-point
loading, but I agree that forcing the fp operation seems better.

> 
>> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
>> index 18949e982a..a4c70cd1d1 100644
>> --- a/sysdeps/i386/fpu/fesetexcept.c
>> +++ b/sysdeps/i386/fpu/fesetexcept.c
>> @@ -17,15 +17,54 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #include <fenv.h>
>> +#include <ldsodefs.h>
>>  
>>  int
>>  fesetexcept (int excepts)
>>  {
>> +  /* 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.  */
>> +
>>    fenv_t temp;
>>  
>> +  excepts &= FE_ALL_EXCEPT;
>> +
>>    __asm__ ("fnstenv %0" : "=m" (*&temp));
> 
> The variable 'temp' and this __asm__ statement can be moved to the 'else'
> branch, since in the case that an SSE unit is present, we don't need to
> touch the 387 unit at all. (Since the job here is to _set_ some exception
> flag bits.)

Indeed, I will change it.

> 
>> +  if (CPU_FEATURE_USABLE (SSE))
>> +    {
>> +      /* Clear relevant flags.  */
>> +      temp.__status_word &= ~excepts;
>> +
>> +      /* Store the new status word (along with the rest of the environment).  */
>> +      __asm__ ("fldenv %0" : : "m" (*&temp));
>> +
> 
> These last two statements can be removed, since in this case, we don't need to
> touch the 387 unit at all.

Ack.

> 
>> +      /* And now similarly for SSE.  */
>> +      unsigned int mxcsr;
>> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
>> +
>> +      /* Set relevant flags.  */
>> +      mxcsr |= excepts & FE_ALL_EXCEPT;
> 
> No need for the ' & FE_ALL_EXCEPT' here, since it was already done at function
> entry.

Ack.

> 
>> +      /* Put the new data in effect.  */
>> +      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
>> +    }
>> +  else
>> +    {
>> +      /* Clear or set relevant flags.  */
>> +      temp.__status_word ^= temp.__status_word & excepts;
> 
> This last statement is not right: It clears bits from temp.__status_word,
> but should *set* these bits instead. Change this to:
> 
>          /* Set relevant flags.  */
>          temp.__status_word |= excepts;

Indeed, I think I missed it because I don't have some old chip that actually
stress it.  I will check if qemu-user can emulated one.

> 
>> +      if (temp.__control_word & temp.__status_word & excepts)
> 
> The temp.__status_word does not need to be tested here, since we just
> set all EXCEPTS bit in it just before. With Joseph's remark, this line
> becomes
> 
>          if ((~temp.__control_word) & excepts)

Ack.

> 
>> +        /* 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.  */
> 
> In this function, we need to reference § 7.6.4.4.

Ack (and I will remove § to avoid non ascii characteres).

> 
>> 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..d88229c271
>> --- /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 FPSCR results in enabled traps for those
>> +   exceptions being taken.  */
> 
> The i386 does not have an FPSCR register. The exception flags are stored
> in the register that gdb calls 'fstat' instead. I would use the same name.
> (The Intel reference does not have a short name for this register; it
> calls it "FPU Status Register".)
> 

Ack.

> Bruno
> 
> 
> 

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

[Adhemerval Zanella Netto]

On 23/10/23 21:19, Bruno Haible wrote:
> Adhemerval Zanella wrote:
>> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
>> index 9b8f583ae6..a486d17951 100644
>> --- a/math/test-fexcept-traps.c
>> +++ b/math/test-fexcept-traps.c
>> @@ -67,6 +68,16 @@ do_test (void)
>>       where setting the exception might result in traps the function should
>>       return a nonzero value.  */
>>    ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
>> +
>> +  /* Execute some floating-point operations, since on some CPUs exceptions
>> +     triggers a trap only at the next floating-point instruction.  */
>> +  double a = 1.0;
>> +  double b = a + a;
>> +  math_force_eval (b);
>> +  long double al = 1.0L;
>> +  long double bl = al + al;
> 
> Like in [2/3], I would mark the variables a, b, al, bl as 'volatile',
> otherwise it's too easy for GCC to do constant-folding and thus eliminate
> the floating-point operations.

Ack.

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

[Bruno Haible]

Adhemerval Zanella Netto wrote:
> > In this function, we need to reference § 7.6.4.4.
> 
> Ack (and I will remove § to avoid non ascii characteres).

We can have UTF-8 encoded non-ASCII characters in the source code comments
nowadays. Such as in glibc/include/idx.h, glibc/string/strverscmp.c, etc.

Bruno