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

[PATCH v2 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           | 41 +++++++++++++++--
 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, 160 insertions(+), 50 deletions(-)
 create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h

-- 
2.34.1

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

[Adhemerval Zanella]

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 v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)

[Adhemerval Zanella]

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           | 41 +++++++++++++++++++++---
 sysdeps/i386/fpu/math-tests-trap-force.h | 29 +++++++++++++++++
 3 files changed, 77 insertions(+), 4 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..2f63e9ba23 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.  */
+  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)
diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
index 18949e982a..6eeb5ab5b0 100644
--- a/sysdeps/i386/fpu/fesetexcept.c
+++ b/sysdeps/i386/fpu/fesetexcept.c
@@ -17,15 +17,48 @@
    <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))
+    {
+      /* And now similarly for SSE.  */
+      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;
+
+      __asm__ ("fnstenv %0" : "=m" (*&temp));
+
+      /* Clear or set relevant flags.  */
+      temp.__status_word |= 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.  */
+        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..20f4ead98d
--- /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.  */
-- 
2.34.1

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

[Adhemerval Zanella]

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.

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..f10bf2d9a3 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.  */
+  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 && !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 v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)

[Bruno Haible]

Looks all good, except for two comments:

Adhemerval Zanella wrote:
> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
> index 18949e982a..6eeb5ab5b0 100644
> --- a/sysdeps/i386/fpu/fesetexcept.c
> +++ b/sysdeps/i386/fpu/fesetexcept.c
> @@ -17,15 +17,48 @@
>     <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))
> +    {
> +      /* And now similarly for SSE.  */
> +      unsigned int mxcsr;
> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));

The comment "And now similarly for SSE." is odd, since there was no similar code
before it. How about
         /* Set the flags in the SSE unit.  */

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

The comment here should be
         /* Set relevant flags.  */
since here, unlike in fesetexceptflag(), we don't need to clear any flags.

Bruno

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

[Adhemerval Zanella Netto]

On 24/10/23 10:43, Bruno Haible wrote:
> Looks all good, except for two comments:
> 
> Adhemerval Zanella wrote:
>> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
>> index 18949e982a..6eeb5ab5b0 100644
>> --- a/sysdeps/i386/fpu/fesetexcept.c
>> +++ b/sysdeps/i386/fpu/fesetexcept.c
>> @@ -17,15 +17,48 @@
>>     <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))
>> +    {
>> +      /* And now similarly for SSE.  */
>> +      unsigned int mxcsr;
>> +      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
> 
> The comment "And now similarly for SSE." is odd, since there was no similar code
> before it. How about
>          /* Set the flags in the SSE unit.  */

Ack.

> 
>> +      /* Set relevant flags.  */
>> +      mxcsr |= excepts;
>> +
>> +      /* Put the new data in effect.  */
>> +      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
>> +    }
>> +  else
>> +    {
>> +      fenv_t temp;
>> +
>> +      __asm__ ("fnstenv %0" : "=m" (*&temp));
>> +
>> +      /* Clear or set relevant flags.  */
>> +      temp.__status_word |= temp.__status_word & excepts;
> 
> The comment here should be
>          /* Set relevant flags.  */
> since here, unlike in fesetexceptflag(), we don't need to clear any flags.

Ack.

I changed both locally.

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

[Bruno Haible]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Hi Adhemerval,

In the error case, the new code mistakenly masks all floating-point
exceptions (i.e. as if someone had called fedisableexcept (FE_ALL_EXCEPT)).
This is because the FNSTENV instruction is documented as
   "Saves the current FPU operating environment at the memory
    location specified with the destination operand, and then
    masks all floating-point exceptions."

The mistake came from my initial proposed fix of BZ 30990. Sorry about that.

Here's a proposed fix, on top of your patch. I've verified that the sequence
of instructions
    __asm__ ("fnstenv %0" : "=m" (*&temp));
    __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
does restore the exceptions trapping bits.

Bruno


[-- Attachment #2: BZ30989-fnstenv-fix.diff --]
[-- Type: text/x-patch, Size: 1249 bytes --]

diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
index 6eeb5ab5b0..8c93264b0c 100644
--- a/sysdeps/i386/fpu/fesetexcept.c
+++ b/sysdeps/i386/fpu/fesetexcept.c
@@ -46,15 +46,20 @@ fesetexcept (int excepts)
       fenv_t temp;
 
       __asm__ ("fnstenv %0" : "=m" (*&temp));
+      /* Note: fnstenv masks all floating-point exceptions until the fldenv
+         or fldcw below.  */
 
       /* Clear or set relevant flags.  */
       temp.__status_word |= 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.  */
-        return -1;
+        {
+          /* 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));

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

[Bruno Haible]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

Hi Adhemerval,

Like in the PATCH v2 2/3, in the error case, the new code mistakenly masks
all floating-point exceptions (i.e. as if someone had called
fedisableexcept (FE_ALL_EXCEPT)).
This is because the FNSTENV instruction is documented as
   "Saves the current FPU operating environment at the memory
    location specified with the destination operand, and then
    masks all floating-point exceptions."

The mistake came from my initial proposed fix. Sorry about that.

Here's a proposed fix, on top of your patch. I've verified that the sequence
of instructions
    __asm__ ("fnstenv %0" : "=m" (*&temp));
    __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
does restore the exceptions trapping bits.

Bruno


[-- Attachment #2: BZ30990-fnstenv-fix.diff --]
[-- Type: text/x-patch, Size: 1478 bytes --]

diff --git a/sysdeps/i386/fpu/fsetexcptflg.c b/sysdeps/i386/fpu/fsetexcptflg.c
index ccbcf35e8e..8bff9026a1 100644
--- a/sysdeps/i386/fpu/fsetexcptflg.c
+++ b/sysdeps/i386/fpu/fsetexcptflg.c
@@ -36,6 +36,8 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
   /* Get the current x87 FPU environment.  We have to do this since we
      cannot separately set the status word.  */
   __asm__ ("fnstenv %0" : "=m" (*&temp));
+  /* Note: fnstenv masks all floating-point exceptions until the fldenv
+     or fldcw below.  */
 
   if (CPU_FEATURE_USABLE (SSE))
     {
@@ -62,10 +64,13 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
       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;
+        {
+          /* 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));

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

[Adhemerval Zanella Netto]

On 30/10/23 12:21, Bruno Haible wrote:
> Hi Adhemerval,
> 
> In the error case, the new code mistakenly masks all floating-point
> exceptions (i.e. as if someone had called fedisableexcept (FE_ALL_EXCEPT)).
> This is because the FNSTENV instruction is documented as
>    "Saves the current FPU operating environment at the memory
>     location specified with the destination operand, and then
>     masks all floating-point exceptions."
> 
> The mistake came from my initial proposed fix of BZ 30990. Sorry about that.
> 
> Here's a proposed fix, on top of your patch. I've verified that the sequence
> of instructions
>     __asm__ ("fnstenv %0" : "=m" (*&temp));
>     __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
> does restore the exceptions trapping bits.
> 
> Bruno

Right, it would be good to have a regression check for this.  I will update
the patch.