[PATCH v2 0/7] Multiple floating-point environment fixes

[PATCH v2 0/7] Multiple floating-point environment fixes

[Adhemerval Zanella]

Bruno Haible has uncovered multiple issues with floating point
environment functions on multiple platforms.  He already added
gnulib modules to overrides some functions, so some of theses fixes
arealready being used on some projects.

There are still some issues where I am not confortable to fix or
install a patch without proper hardware testing (BZ# 31023 for hppa
and BZ# 30993 for alpha), and there also one that would require a
lot of working since it requires fixing the compiler (BZ# 30973
for sh4).

Adhemerval Zanella (3):
  powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag
    (BZ 30988)
  i686: Do not raise exception traps on fesetexcept (BZ 30989)
  riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)

Bruno Haible (4):
  x86: Do not raises floating-point exception traps on fesetexceptflag
    (BZ 30990)
  manual: Clarify undefined behavior of feenableexcept (BZ 31019)
  alpha: Fix fesetexceptflag (BZ 30998)
  hppa: Fix undefined behaviour in feclearexcept (BZ 30983)

 manual/arith.texi                        |   6 ++
 math/test-fenv.c                         | 131 +++++++++++++++++++++--
 math/test-fesetexcept-traps.c            |  37 +++++--
 math/test-fexcept-traps.c                |  34 ++++--
 sysdeps/alpha/fpu/fsetexcptflg.c         |   2 +-
 sysdeps/hppa/fpu/fclrexcpt.c             |   2 +-
 sysdeps/i386/fpu/fesetexcept.c           |  46 +++++++-
 sysdeps/i386/fpu/fsetexcptflg.c          |  63 +++++++----
 sysdeps/i386/fpu/math-tests-trap-force.h |  29 +++++
 sysdeps/powerpc/fpu/fesetexcept.c        |   5 +
 sysdeps/powerpc/fpu/fsetexcptflg.c       |   9 +-
 sysdeps/riscv/rvf/fenv_private.h         |   8 +-
 sysdeps/x86/fpu/test-fenv-sse-2.c        |  23 +---
 sysdeps/x86_64/fpu/fsetexcptflg.c        |  24 +++--
 14 files changed, 330 insertions(+), 89 deletions(-)
 create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h

-- 
2.34.1

[PATCH v2 1/7] 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=yes, 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 v2 2/7] 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            | 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.  */
+  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>
-- 
2.34.1

[PATCH v2 3/7] 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=yes, Size: 7455 bytes --]

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

[PATCH v2 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)

[Adhemerval Zanella]

From: Bruno Haible <bruno@clisp.org>

Explain undefined behavior of feenableexcept in a special case.
---
 manual/arith.texi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/manual/arith.texi b/manual/arith.texi
index fa7110e992..be24c20493 100644
--- a/manual/arith.texi
+++ b/manual/arith.texi
@@ -1176,6 +1176,12 @@ enabled, the status of the other exceptions is not changed.
 
 The function returns the previous enabled exceptions in case the
 operation was successful, @code{-1} otherwise.
+
+Note: Enabling traps for an exception for which the exception flag is
+currently already set (@pxref{Status bit operations}) has unspecified
+consequences: it may or may not trigger a trap immediately.
+@c It triggers a trap immediately on powerpc*, at the next floating-
+@c instruction on i386, and not at all on the other CPUs.
 @end deftypefun
 
 @deftypefun int fedisableexcept (int @var{excepts})
-- 
2.34.1

[PATCH v2 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)

[Adhemerval Zanella]

libc_feupdateenv_riscv should check for FE_DFL_ENV, similar to
libc_fesetenv_riscv.

Also extend the test-fenv.c to test fenvupdate.

Checked on riscv under qemu-system.
---
 math/test-fenv.c                 | 131 ++++++++++++++++++++++++++++---
 sysdeps/riscv/rvf/fenv_private.h |   8 +-
 2 files changed, 124 insertions(+), 15 deletions(-)

diff --git a/math/test-fenv.c b/math/test-fenv.c
index 0af7141ba7..63dceddb10 100644
--- a/math/test-fenv.c
+++ b/math/test-fenv.c
@@ -196,6 +196,30 @@ set_single_exc (const char *test_name, int fe_exc, fexcept_t exception)
   feclearexcept (exception);
   test_exceptions (str, ALL_EXC ^ fe_exc, 0);
 }
+
+static void
+update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
+		   int fe_exc_clear, int exception)
+{
+  char str[200];
+  /* The standard allows the inexact exception to be set together with the
+     underflow and overflow exceptions.  So ignore the inexact flag if the
+     others are raised.  */
+  int ignore_inexact = (fe_exc & (UNDERFLOW_EXC | OVERFLOW_EXC)) != 0;
+
+  strcpy (str, test_name);
+  strcat (str, ": set flag, with rest not set");
+  feclearexcept (FE_ALL_EXCEPT);
+  feraiseexcept (exception);
+  feupdateenv (envp);
+  test_exceptions (str, fe_exc, ignore_inexact);
+
+  strcpy (str, test_name);
+  strcat (str, ": clear flag, rest also unset");
+  feclearexcept (exception);
+  feupdateenv (envp);
+  test_exceptions (str, fe_exc_clear, ignore_inexact);
+}
 #endif
 
 static void
@@ -233,22 +257,32 @@ fe_tests (void)
 }
 
 #if FE_ALL_EXCEPT
+static const char *
+funcname (int (*func)(const fenv_t *))
+{
+  if (func == fesetenv)
+    return "fesetenv";
+  else if (func == feupdateenv)
+    return "feupdateenv";
+  __builtin_unreachable ();
+}
+
 /* Test that program aborts with no masked interrupts */
 static void
-feenv_nomask_test (const char *flag_name, int fe_exc)
+feenv_nomask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
 {
 # if defined FE_NOMASK_ENV
   int status;
   pid_t pid;
 
   if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT)
-      && fesetenv (FE_NOMASK_ENV) != 0)
+      && func (FE_NOMASK_ENV) != 0)
     {
       printf ("Test: not testing FE_NOMASK_ENV, it isn't implemented.\n");
       return;
     }
 
-  printf ("Test: after fesetenv (FE_NOMASK_ENV) processes will abort\n");
+  printf ("Test: after %s (FE_NOMASK_ENV) processes will abort\n", funcname (func));
   printf ("      when feraiseexcept (%s) is called.\n", flag_name);
   pid = fork ();
   if (pid == 0)
@@ -295,12 +329,12 @@ feenv_nomask_test (const char *flag_name, int fe_exc)
 
 /* Test that program doesn't abort with default environment */
 static void
-feenv_mask_test (const char *flag_name, int fe_exc)
+feenv_mask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
 {
   int status;
   pid_t pid;
 
-  printf ("Test: after fesetenv (FE_DFL_ENV) processes will not abort\n");
+  printf ("Test: after %s (FE_DFL_ENV) processes will not abort\n", funcname (func));
   printf ("      when feraiseexcept (%s) is called.\n", flag_name);
   pid = fork ();
   if (pid == 0)
@@ -313,7 +347,7 @@ feenv_mask_test (const char *flag_name, int fe_exc)
       setrlimit (RLIMIT_CORE, &core_limit);
 #endif
 
-      fesetenv (FE_DFL_ENV);
+      func (FE_DFL_ENV);
       feraiseexcept (fe_exc);
       exit (2);
     }
@@ -615,10 +649,18 @@ feenable_test (const char *flag_name, int fe_exc)
 static void
 fe_single_test (const char *flag_name, int fe_exc)
 {
-  feenv_nomask_test (flag_name, fe_exc);
-  feenv_mask_test (flag_name, fe_exc);
+  feenv_nomask_test (flag_name, fe_exc, fesetenv);
+  feenv_mask_test (flag_name, fe_exc, fesetenv);
   feenable_test (flag_name, fe_exc);
 }
+
+
+static void
+feupdate_single_test (const char *flag_name, int fe_exc)
+{
+  feenv_nomask_test (flag_name, fe_exc, feupdateenv);
+  feenv_mask_test (flag_name, fe_exc, feupdateenv);
+}
 #endif
 
 
@@ -646,6 +688,72 @@ feenv_tests (void)
   fesetenv (FE_DFL_ENV);
 }
 
+#if FE_ALL_EXCEPT
+static void
+feupdateenv_single_test (const char *test_name, int fe_exc, int exception)
+{
+  char str[100];
+  fenv_t env;
+  int res;
+
+  snprintf (str, sizeof str, "feupdateenv %s and FL_DFL_ENV", test_name);
+  update_single_exc (str, FE_DFL_ENV, fe_exc, NO_EXC, exception);
+
+  feraiseexcept (FE_ALL_EXCEPT);
+  res = fegetenv (&env);
+  if (res != 0)
+    {
+      printf ("fegetenv failed: %d\n", res);
+      ++count_errors;
+      return;
+    }
+
+  snprintf (str, sizeof str, "feupdateenv %s and FE_ALL_EXCEPT", test_name);
+  update_single_exc (str, &env, ALL_EXC, ALL_EXC, exception);
+}
+#endif
+
+static void
+feupdateenv_tests (void)
+{
+  /* We might have some exceptions still set.  */
+  feclearexcept (FE_ALL_EXCEPT);
+
+#ifdef FE_DIVBYZERO
+  feupdate_single_test ("FE_DIVBYZERO", FE_DIVBYZERO);
+#endif
+#ifdef FE_INVALID
+  feupdate_single_test ("FE_INVALID", FE_INVALID);
+#endif
+#ifdef FE_INEXACT
+  feupdate_single_test ("FE_INEXACT", FE_INEXACT);
+#endif
+#ifdef FE_UNDERFLOW
+  feupdate_single_test ("FE_UNDERFLOW", FE_UNDERFLOW);
+#endif
+#ifdef FE_OVERFLOW
+  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
+#endif
+
+#ifdef FE_DIVBYZERO
+  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
+#endif
+#ifdef FE_INVALID
+  feupdateenv_single_test ("INVALID", INVALID_EXC, FE_INVALID);
+#endif
+#ifdef FE_INEXACT
+  feupdateenv_single_test ("INEXACT", INEXACT_EXC, FE_INEXACT);
+#endif
+#ifdef FE_UNDERFLOW
+  feupdateenv_single_test ("UNDERFLOW", UNDERFLOW_EXC, FE_UNDERFLOW);
+#endif
+#ifdef FE_OVERFLOW
+  feupdateenv_single_test ("OVERFLOW", OVERFLOW_EXC, FE_OVERFLOW);
+#endif
+
+  feupdateenv (FE_DFL_ENV);
+}
+
 
 static void
 feholdexcept_tests (void)
@@ -766,13 +874,14 @@ initial_tests (void)
 #endif
 }
 
-int
-main (void)
+static int
+do_test (void)
 {
   initial_tests ();
   fe_tests ();
   feenv_tests ();
   feholdexcept_tests ();
+  feupdateenv_tests ();
 
   if (count_errors)
     {
@@ -782,3 +891,5 @@ main (void)
   printf ("\n All tests passed successfully.\n");
   return 0;
 }
+
+#include <support/test-driver.c>
diff --git a/sysdeps/riscv/rvf/fenv_private.h b/sysdeps/riscv/rvf/fenv_private.h
index 40e23661b7..d8d65458b2 100644
--- a/sysdeps/riscv/rvf/fenv_private.h
+++ b/sysdeps/riscv/rvf/fenv_private.h
@@ -93,10 +93,7 @@ libc_fetestexcept_riscv (int ex)
 static __always_inline void
 libc_fesetenv_riscv (const fenv_t *envp)
 {
-  long int env = (long int) envp - (long int) FE_DFL_ENV;
-  if (env != 0)
-    env = *envp;
-
+  long int env = (envp != FE_DFL_ENV ? *envp : 0);
   _FPU_SETCW (env);
 }
 
@@ -123,7 +120,8 @@ libc_feupdateenv_test_riscv (const fenv_t *envp, int ex)
 static __always_inline void
 libc_feupdateenv_riscv (const fenv_t *envp)
 {
-  _FPU_SETCW (*envp | riscv_getflags ());
+  long int env = (envp != FE_DFL_ENV ? *envp : 0);
+  _FPU_SETCW (env | riscv_getflags ());
 }
 
 #define libc_feupdateenv  libc_feupdateenv_riscv
-- 
2.34.1

[PATCH v2 6/7] alpha: Fix fesetexceptflag (BZ 30998)

[Adhemerval Zanella]

From: Bruno Haible <bruno@clisp.org>

It clears some exception flags that are outside the EXCEPTS argument.

It fixes math/test-fexcept on qemu-user.
---
 sysdeps/alpha/fpu/fsetexcptflg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/alpha/fpu/fsetexcptflg.c b/sysdeps/alpha/fpu/fsetexcptflg.c
index 70f3666a6e..63eb06845d 100644
--- a/sysdeps/alpha/fpu/fsetexcptflg.c
+++ b/sysdeps/alpha/fpu/fsetexcptflg.c
@@ -27,7 +27,7 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
   tmp = __ieee_get_fp_control ();
 
   /* Set all the bits that were called for.  */
-  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
+  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;
 
   /* And store it back.  */
   __ieee_set_fp_control (tmp);
-- 
2.34.1

[PATCH v2 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983)

[Adhemerval Zanella]

From: Bruno Haible <bruno@clisp.org>

The expression

  (excepts & FE_ALL_EXCEPT) << 27

produces a signed integer overflow when 'excepts' is specified as
FE_INVALID (= 0x10), because
  - excepts is of type 'int',
  - FE_ALL_EXCEPT is of type 'int',
  - thus (excepts & FE_ALL_EXCEPT) is (int) 0x10,
  - 'int' is 32 bits wide.

The patched code produces the same instruction sequence as
previosuly.
---
 sysdeps/hppa/fpu/fclrexcpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/hppa/fpu/fclrexcpt.c b/sysdeps/hppa/fpu/fclrexcpt.c
index 055fb04ccc..46caf39ec1 100644
--- a/sysdeps/hppa/fpu/fclrexcpt.c
+++ b/sysdeps/hppa/fpu/fclrexcpt.c
@@ -26,7 +26,7 @@ feclearexcept (int excepts)
   /* Get the current status word. */
   __asm__ ("fstd %%fr0,0(%1)" : "=m" (s.l) : "r" (&s.l) : "%r0");
   /* Clear all the relevant bits. */
-  s.sw[0] &= ~((excepts & FE_ALL_EXCEPT) << 27);
+  s.sw[0] &= ~(((unsigned int) excepts & FE_ALL_EXCEPT) << 27);
   __asm__ ("fldd 0(%0),%%fr0" : : "r" (&s.l), "m" (s.l) : "%r0");
 
   /* Success.  */
-- 
2.34.1

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

[Carlos O'Donell]

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

OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.

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

Changes test from UNSUPPORTED to PASS when we should test more now that with
C2x we're saying the behaviour will result in a non-zero return... then we
should test for that.

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

We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?

e.g.

  if (!EXCEPTION_SET_FORCES_TRAP)
    { 
      if (ret == 0)
        puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
      else
        /* fail */
    }
  else
    {
      if (ret == 0)
        /* fail */
      else
        /* pass */
    }

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

Likewise. We can test more now.

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

-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

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.

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

[Carlos O'Donell]

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.

Re: [PATCH v2 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)

[Carlos O'Donell]

On 11/6/23 08:27, Adhemerval Zanella wrote:
> From: Bruno Haible <bruno@clisp.org>
> 
> Explain undefined behavior of feenableexcept in a special case.

LGTM.

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

> ---
>  manual/arith.texi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/manual/arith.texi b/manual/arith.texi
> index fa7110e992..be24c20493 100644
> --- a/manual/arith.texi
> +++ b/manual/arith.texi
> @@ -1176,6 +1176,12 @@ enabled, the status of the other exceptions is not changed.
>  
>  The function returns the previous enabled exceptions in case the
>  operation was successful, @code{-1} otherwise.
> +
> +Note: Enabling traps for an exception for which the exception flag is
> +currently already set (@pxref{Status bit operations}) has unspecified
> +consequences: it may or may not trigger a trap immediately.

Agreed.

> +@c It triggers a trap immediately on powerpc*, at the next floating-
> +@c instruction on i386, and not at all on the other CPUs.
>  @end deftypefun
>  
>  @deftypefun int fedisableexcept (int @var{excepts})

-- 
Cheers,
Carlos.

Re: [PATCH v2 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)

[Carlos O'Donell]

On 11/6/23 08:27, Adhemerval Zanella wrote:
> libc_feupdateenv_riscv should check for FE_DFL_ENV, similar to
> libc_fesetenv_riscv.
> 
> Also extend the test-fenv.c to test fenvupdate.
> 

LGTM. Thanks for extending the tests.

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

> Checked on riscv under qemu-system.
> ---
>  math/test-fenv.c                 | 131 ++++++++++++++++++++++++++++---
>  sysdeps/riscv/rvf/fenv_private.h |   8 +-
>  2 files changed, 124 insertions(+), 15 deletions(-)
> 
> diff --git a/math/test-fenv.c b/math/test-fenv.c
> index 0af7141ba7..63dceddb10 100644
> --- a/math/test-fenv.c
> +++ b/math/test-fenv.c
> @@ -196,6 +196,30 @@ set_single_exc (const char *test_name, int fe_exc, fexcept_t exception)
>    feclearexcept (exception);
>    test_exceptions (str, ALL_EXC ^ fe_exc, 0);
>  }
> +
> +static void
> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
> +		   int fe_exc_clear, int exception)
> +{
> +  char str[200];
> +  /* The standard allows the inexact exception to be set together with the
> +     underflow and overflow exceptions.  So ignore the inexact flag if the
> +     others are raised.  */
> +  int ignore_inexact = (fe_exc & (UNDERFLOW_EXC | OVERFLOW_EXC)) != 0;
> +
> +  strcpy (str, test_name);
> +  strcat (str, ": set flag, with rest not set");
> +  feclearexcept (FE_ALL_EXCEPT);
> +  feraiseexcept (exception);
> +  feupdateenv (envp);
> +  test_exceptions (str, fe_exc, ignore_inexact);
> +
> +  strcpy (str, test_name);
> +  strcat (str, ": clear flag, rest also unset");
> +  feclearexcept (exception);
> +  feupdateenv (envp);
> +  test_exceptions (str, fe_exc_clear, ignore_inexact);
> +}
>  #endif
>  
>  static void
> @@ -233,22 +257,32 @@ fe_tests (void)
>  }
>  
>  #if FE_ALL_EXCEPT
> +static const char *
> +funcname (int (*func)(const fenv_t *))
> +{
> +  if (func == fesetenv)
> +    return "fesetenv";
> +  else if (func == feupdateenv)
> +    return "feupdateenv";
> +  __builtin_unreachable ();
> +}
> +
>  /* Test that program aborts with no masked interrupts */
>  static void
> -feenv_nomask_test (const char *flag_name, int fe_exc)
> +feenv_nomask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
>  {
>  # if defined FE_NOMASK_ENV
>    int status;
>    pid_t pid;
>  
>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT)
> -      && fesetenv (FE_NOMASK_ENV) != 0)
> +      && func (FE_NOMASK_ENV) != 0)
>      {
>        printf ("Test: not testing FE_NOMASK_ENV, it isn't implemented.\n");
>        return;
>      }
>  
> -  printf ("Test: after fesetenv (FE_NOMASK_ENV) processes will abort\n");
> +  printf ("Test: after %s (FE_NOMASK_ENV) processes will abort\n", funcname (func));
>    printf ("      when feraiseexcept (%s) is called.\n", flag_name);
>    pid = fork ();
>    if (pid == 0)
> @@ -295,12 +329,12 @@ feenv_nomask_test (const char *flag_name, int fe_exc)
>  
>  /* Test that program doesn't abort with default environment */
>  static void
> -feenv_mask_test (const char *flag_name, int fe_exc)
> +feenv_mask_test (const char *flag_name, int fe_exc, int (*func)(const fenv_t *))
>  {
>    int status;
>    pid_t pid;
>  
> -  printf ("Test: after fesetenv (FE_DFL_ENV) processes will not abort\n");
> +  printf ("Test: after %s (FE_DFL_ENV) processes will not abort\n", funcname (func));
>    printf ("      when feraiseexcept (%s) is called.\n", flag_name);
>    pid = fork ();
>    if (pid == 0)
> @@ -313,7 +347,7 @@ feenv_mask_test (const char *flag_name, int fe_exc)
>        setrlimit (RLIMIT_CORE, &core_limit);
>  #endif
>  
> -      fesetenv (FE_DFL_ENV);
> +      func (FE_DFL_ENV);
>        feraiseexcept (fe_exc);
>        exit (2);
>      }
> @@ -615,10 +649,18 @@ feenable_test (const char *flag_name, int fe_exc)
>  static void
>  fe_single_test (const char *flag_name, int fe_exc)
>  {
> -  feenv_nomask_test (flag_name, fe_exc);
> -  feenv_mask_test (flag_name, fe_exc);
> +  feenv_nomask_test (flag_name, fe_exc, fesetenv);
> +  feenv_mask_test (flag_name, fe_exc, fesetenv);
>    feenable_test (flag_name, fe_exc);
>  }
> +
> +
> +static void
> +feupdate_single_test (const char *flag_name, int fe_exc)
> +{
> +  feenv_nomask_test (flag_name, fe_exc, feupdateenv);
> +  feenv_mask_test (flag_name, fe_exc, feupdateenv);
> +}
>  #endif
>  
>  
> @@ -646,6 +688,72 @@ feenv_tests (void)
>    fesetenv (FE_DFL_ENV);
>  }
>  
> +#if FE_ALL_EXCEPT
> +static void
> +feupdateenv_single_test (const char *test_name, int fe_exc, int exception)
> +{
> +  char str[100];
> +  fenv_t env;
> +  int res;
> +
> +  snprintf (str, sizeof str, "feupdateenv %s and FL_DFL_ENV", test_name);
> +  update_single_exc (str, FE_DFL_ENV, fe_exc, NO_EXC, exception);
> +
> +  feraiseexcept (FE_ALL_EXCEPT);
> +  res = fegetenv (&env);
> +  if (res != 0)
> +    {
> +      printf ("fegetenv failed: %d\n", res);
> +      ++count_errors;
> +      return;
> +    }
> +
> +  snprintf (str, sizeof str, "feupdateenv %s and FE_ALL_EXCEPT", test_name);
> +  update_single_exc (str, &env, ALL_EXC, ALL_EXC, exception);
> +}
> +#endif
> +
> +static void
> +feupdateenv_tests (void)
> +{
> +  /* We might have some exceptions still set.  */
> +  feclearexcept (FE_ALL_EXCEPT);
> +
> +#ifdef FE_DIVBYZERO
> +  feupdate_single_test ("FE_DIVBYZERO", FE_DIVBYZERO);
> +#endif
> +#ifdef FE_INVALID
> +  feupdate_single_test ("FE_INVALID", FE_INVALID);
> +#endif
> +#ifdef FE_INEXACT
> +  feupdate_single_test ("FE_INEXACT", FE_INEXACT);
> +#endif
> +#ifdef FE_UNDERFLOW
> +  feupdate_single_test ("FE_UNDERFLOW", FE_UNDERFLOW);
> +#endif
> +#ifdef FE_OVERFLOW
> +  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
> +#endif
> +
> +#ifdef FE_DIVBYZERO
> +  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
> +#endif
> +#ifdef FE_INVALID
> +  feupdateenv_single_test ("INVALID", INVALID_EXC, FE_INVALID);
> +#endif
> +#ifdef FE_INEXACT
> +  feupdateenv_single_test ("INEXACT", INEXACT_EXC, FE_INEXACT);
> +#endif
> +#ifdef FE_UNDERFLOW
> +  feupdateenv_single_test ("UNDERFLOW", UNDERFLOW_EXC, FE_UNDERFLOW);
> +#endif
> +#ifdef FE_OVERFLOW
> +  feupdateenv_single_test ("OVERFLOW", OVERFLOW_EXC, FE_OVERFLOW);
> +#endif
> +
> +  feupdateenv (FE_DFL_ENV);
> +}
> +
>  
>  static void
>  feholdexcept_tests (void)
> @@ -766,13 +874,14 @@ initial_tests (void)
>  #endif
>  }
>  
> -int
> -main (void)
> +static int
> +do_test (void)
>  {
>    initial_tests ();
>    fe_tests ();
>    feenv_tests ();
>    feholdexcept_tests ();
> +  feupdateenv_tests ();
>  
>    if (count_errors)
>      {
> @@ -782,3 +891,5 @@ main (void)
>    printf ("\n All tests passed successfully.\n");
>    return 0;
>  }
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/riscv/rvf/fenv_private.h b/sysdeps/riscv/rvf/fenv_private.h
> index 40e23661b7..d8d65458b2 100644
> --- a/sysdeps/riscv/rvf/fenv_private.h
> +++ b/sysdeps/riscv/rvf/fenv_private.h
> @@ -93,10 +93,7 @@ libc_fetestexcept_riscv (int ex)
>  static __always_inline void
>  libc_fesetenv_riscv (const fenv_t *envp)
>  {
> -  long int env = (long int) envp - (long int) FE_DFL_ENV;
> -  if (env != 0)
> -    env = *envp;
> -
> +  long int env = (envp != FE_DFL_ENV ? *envp : 0);
>    _FPU_SETCW (env);
>  }
>  
> @@ -123,7 +120,8 @@ libc_feupdateenv_test_riscv (const fenv_t *envp, int ex)
>  static __always_inline void
>  libc_feupdateenv_riscv (const fenv_t *envp)
>  {
> -  _FPU_SETCW (*envp | riscv_getflags ());
> +  long int env = (envp != FE_DFL_ENV ? *envp : 0);
> +  _FPU_SETCW (env | riscv_getflags ());
>  }
>  
>  #define libc_feupdateenv  libc_feupdateenv_riscv

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 06/11/23 13:08, Carlos O'Donell wrote:
> 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).
>>
>> 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.
> 
> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
> 
>>
>> 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.
> 
> Changes test from UNSUPPORTED to PASS when we should test more now that with
> C2x we're saying the behaviour will result in a non-zero return... then we
> should test for that.
> 
>> ---
>>  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)
> 
> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
> 
> e.g.
> 
>   if (!EXCEPTION_SET_FORCES_TRAP)
>     { 
>       if (ret == 0)
>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>       else
>         /* fail */
>     }
>   else
>     {
>       if (ret == 0)
>         /* fail */
>       else
>         /* pass */
>     }

The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
checks are not really meaningful: either the function succeeds and return 0, or it fails
for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
failure.

So if the function succeeds and no trap is generated (which terminates the process
as default on Linux) we are fine.  Otherwise, it check if the failure is expected
(EXCEPTION_SET_FORCES_TRAP).

Re: [PATCH v2 6/7] alpha: Fix fesetexceptflag (BZ 30998)

[Carlos O'Donell]

On 11/6/23 08:27, Adhemerval Zanella wrote:
> From: Bruno Haible <bruno@clisp.org>
> 
> It clears some exception flags that are outside the EXCEPTS argument.
> 
> It fixes math/test-fexcept on qemu-user.

> ---
>  sysdeps/alpha/fpu/fsetexcptflg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/alpha/fpu/fsetexcptflg.c b/sysdeps/alpha/fpu/fsetexcptflg.c
> index 70f3666a6e..63eb06845d 100644
> --- a/sysdeps/alpha/fpu/fsetexcptflg.c
> +++ b/sysdeps/alpha/fpu/fsetexcptflg.c
> @@ -27,7 +27,7 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
>    tmp = __ieee_get_fp_control ();
>  
>    /* Set all the bits that were called for.  */
> -  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
> +  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;

Does this actually work?

Assume excepts is FE_INVALID, and *flagp bit 17 is 0.
Assume currently bit 17 is 1.

tmp ^ *flagp       => bit 17 is still 1, even though bit 17 in flagp is 0.
& excepts          => bit 17 is still 1.
& SWCR_STATUS_MASK => bit 17 is still 1.
^=                 => bit 17 is still 1.

The operation will set a bit, but won't clear it?

I would expect (taken from hppa code I wrote for that port):

/* Clear all status bits we care about.  */
tmp = tmp & ~(excepts & SWCR_STATUS_MASK);
/* Install the new ones.  */
tmp |= *flagp & excepts & SWCR_STATUS_MASK;

>  
>    /* And store it back.  */
>    __ieee_set_fp_control (tmp);

-- 
Cheers,
Carlos.

Re: [PATCH v2 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983)

[Carlos O'Donell]

On 11/6/23 08:27, Adhemerval Zanella wrote:
> From: Bruno Haible <bruno@clisp.org>
> 
> The expression
> 
>   (excepts & FE_ALL_EXCEPT) << 27
> 
> produces a signed integer overflow when 'excepts' is specified as
> FE_INVALID (= 0x10), because
>   - excepts is of type 'int',
>   - FE_ALL_EXCEPT is of type 'int',
>   - thus (excepts & FE_ALL_EXCEPT) is (int) 0x10,
>   - 'int' is 32 bits wide.

Agreed.

> 
> The patched code produces the same instruction sequence as
> previosuly.

LGTM.

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

> ---
>  sysdeps/hppa/fpu/fclrexcpt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/hppa/fpu/fclrexcpt.c b/sysdeps/hppa/fpu/fclrexcpt.c
> index 055fb04ccc..46caf39ec1 100644
> --- a/sysdeps/hppa/fpu/fclrexcpt.c
> +++ b/sysdeps/hppa/fpu/fclrexcpt.c
> @@ -26,7 +26,7 @@ feclearexcept (int excepts)
>    /* Get the current status word. */
>    __asm__ ("fstd %%fr0,0(%1)" : "=m" (s.l) : "r" (&s.l) : "%r0");
>    /* Clear all the relevant bits. */
> -  s.sw[0] &= ~((excepts & FE_ALL_EXCEPT) << 27);
> +  s.sw[0] &= ~(((unsigned int) excepts & FE_ALL_EXCEPT) << 27);
>    __asm__ ("fldd 0(%0),%%fr0" : : "r" (&s.l), "m" (s.l) : "%r0");
>  
>    /* Success.  */

-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/11/23 13:08, Carlos O'Donell wrote:
>> 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).
>>>
>>> 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.
>>
>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>
>>>
>>> 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.
>>
>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>> C2x we're saying the behaviour will result in a non-zero return... then we
>> should test for that.
>>
>>> ---
>>>  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)
>>
>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>
>> e.g.
>>
>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>     { 
>>       if (ret == 0)
>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>       else
>>         /* fail */
>>     }
>>   else
>>     {
>>       if (ret == 0)
>>         /* fail */
>>       else
>>         /* pass */
>>     }
> 
> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
> checks are not really meaningful: either the function succeeds and return 0, or it fails
> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
> failure.

Sure.

> So if the function succeeds and no trap is generated (which terminates the process
> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
> (EXCEPTION_SET_FORCES_TRAP).
> 

So we go from UNSUPPORTED to... ?

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 06/11/23 14:02, Carlos O'Donell wrote:
> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>
>>
>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>> 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).
>>>>
>>>> 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.
>>>
>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>
>>>>
>>>> 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.
>>>
>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>> should test for that.
>>>
>>>> ---
>>>>  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)
>>>
>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>
>>> e.g.
>>>
>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>     { 
>>>       if (ret == 0)
>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>       else
>>>         /* fail */
>>>     }
>>>   else
>>>     {
>>>       if (ret == 0)
>>>         /* fail */
>>>       else
>>>         /* pass */
>>>     }
>>
>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>> failure.
> 
> Sure.
> 
>> So if the function succeeds and no trap is generated (which terminates the process
>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>> (EXCEPTION_SET_FORCES_TRAP).
>>
> 
> So we go from UNSUPPORTED to... ?
> 

I though about that, but the test also checks fegetexceptflag (a better option would
to split the test in two, so only the fesetexceptflag is unsupported on ppc32).

Re: [PATCH v2 6/7] alpha: Fix fesetexceptflag (BZ 30998)

[Bruno Haible]

Carlos O'Donell wrote:
> I would expect (taken from hppa code I wrote for that port):
> 
> /* Clear all status bits we care about.  */
> tmp = tmp & ~(excepts & SWCR_STATUS_MASK);
> /* Install the new ones.  */
> tmp |= *flagp & excepts & SWCR_STATUS_MASK;

This code is correct. And, as I wrote in
https://sourceware.org/bugzilla/show_bug.cgi?id=30998#c2
the code with the double-xor is faster, because it uses one less
instruction.

Basically the code you proposed masks the bits to clear
and the bits to set separately. Whereas the code with the double-xor
masks the bits to *change* - a single AND instead of two.

> >    /* Set all the bits that were called for.  */
> > -  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
> > +  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;
> 
> Does this actually work?

Yes: It computes the bits to *change*, then applies the mask, then applies
the changes.

For bits in the mask, it does
        tmp.bit[i] ^= tmp.bit[i] ^ (*flagp).bit[i];
which simplifies to
        tmp.bit[i] = (*flagp).bit[i];

For bits outside the mask, it does
        tmp.bit[i] ^= 0;
i.e. it leaves the bit unchanged.

I'm using this same idiom also in gnulib
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/fenv-except-state-set.c;h=2035f61e2714126e432c0c1f29450f18f58d81f1;hb=HEAD#l182
and I have verified that it works.

> Assume excepts is FE_INVALID, and *flagp bit 17 is 0.
> Assume currently bit 17 is 1.
> 
> tmp ^ *flagp       => bit 17 is still 1, even though bit 17 in flagp is 0.
> & excepts          => bit 17 is still 1.
> & SWCR_STATUS_MASK => bit 17 is still 1.
> ^=                 => bit 17 is still 1.

In the last step: 1 xor 1 is 0.

Bruno

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

[Adhemerval Zanella Netto]

On 06/11/23 14:11, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/11/23 14:02, Carlos O'Donell wrote:
>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>> 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).
>>>>>
>>>>> 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.
>>>>
>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>> should test for that.
>>>>
>>>>> ---
>>>>>  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)
>>>>
>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>
>>>> e.g.
>>>>
>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>     { 
>>>>       if (ret == 0)
>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>       else
>>>>         /* fail */
>>>>     }
>>>>   else
>>>>     {
>>>>       if (ret == 0)
>>>>         /* fail */
>>>>       else
>>>>         /* pass */
>>>>     }
>>>
>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>> failure.
>>
>> Sure.
>>
>>> So if the function succeeds and no trap is generated (which terminates the process
>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>> (EXCEPTION_SET_FORCES_TRAP).
>>>
>>
>> So we go from UNSUPPORTED to... ?
>>
> 
> I though about that, but the test also checks fegetexceptflag (a better option would
> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).

And the proposed changes follow current file idea, maybe the split should be done
in a different patch.

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

[Carlos O'Donell]

On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/11/23 14:02, Carlos O'Donell wrote:
>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>> 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).
>>>>>
>>>>> 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.
>>>>
>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>> should test for that.
>>>>
>>>>> ---
>>>>>  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)
>>>>
>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>
>>>> e.g.
>>>>
>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>     { 
>>>>       if (ret == 0)
>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>       else
>>>>         /* fail */
>>>>     }
>>>>   else
>>>>     {
>>>>       if (ret == 0)
>>>>         /* fail */
>>>>       else
>>>>         /* pass */
>>>>     }
>>>
>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>> failure.
>>
>> Sure.
>>
>>> So if the function succeeds and no trap is generated (which terminates the process
>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>> (EXCEPTION_SET_FORCES_TRAP).
>>>
>>
>> So we go from UNSUPPORTED to... ?
>>
> 
> I though about that, but the test also checks fegetexceptflag (a better option would
> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
> 

Perhaps the best option is to just keep the UNSUPPORTED status?

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 06/11/23 14:38, Carlos O'Donell wrote:
> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>
>>
>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>
>>>>
>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>> 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).
>>>>>>
>>>>>> 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.
>>>>>
>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>> should test for that.
>>>>>
>>>>>> ---
>>>>>>  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)
>>>>>
>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>
>>>>> e.g.
>>>>>
>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>     { 
>>>>>       if (ret == 0)
>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>       else
>>>>>         /* fail */
>>>>>     }
>>>>>   else
>>>>>     {
>>>>>       if (ret == 0)
>>>>>         /* fail */
>>>>>       else
>>>>>         /* pass */
>>>>>     }
>>>>
>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>> failure.
>>>
>>> Sure.
>>>
>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>
>>>
>>> So we go from UNSUPPORTED to... ?
>>>
>>
>> I though about that, but the test also checks fegetexceptflag (a better option would
>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>
> 
> Perhaps the best option is to just keep the UNSUPPORTED status?
> 

Fair enough.

Re: [PATCH v2 6/7] alpha: Fix fesetexceptflag (BZ 30998)

[Carlos O'Donell]

On 11/6/23 12:36, Bruno Haible wrote:
> I'm using this same idiom also in gnulib
> https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/fenv-except-state-set.c;h=2035f61e2714126e432c0c1f29450f18f58d81f1;hb=HEAD#l182
> and I have verified that it works.

That works for me. Thank you for verifying that.

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

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 06/11/23 14:56, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/11/23 14:38, Carlos O'Donell wrote:
>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>>> 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).
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>>> should test for that.
>>>>>>
>>>>>>> ---
>>>>>>>  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)
>>>>>>
>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>>
>>>>>> e.g.
>>>>>>
>>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>>     { 
>>>>>>       if (ret == 0)
>>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>>       else
>>>>>>         /* fail */
>>>>>>     }
>>>>>>   else
>>>>>>     {
>>>>>>       if (ret == 0)
>>>>>>         /* fail */
>>>>>>       else
>>>>>>         /* pass */
>>>>>>     }
>>>>>
>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>>> failure.
>>>>
>>>> Sure.
>>>>
>>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>>
>>>>
>>>> So we go from UNSUPPORTED to... ?
>>>>
>>>
>>> I though about that, but the test also checks fegetexceptflag (a better option would
>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>>
>>
>> Perhaps the best option is to just keep the UNSUPPORTED status?
>>
> 
> Fair enough.

Revising the patch, I recalled that I explicitly removed the UNSUPPORTED
so the test can now check if the fesetexcept does fails with -1 for 
!EXCEPTION_SET_FORCES_TRAP.  I am not sure if adding it back is an improvement,
it means that it won't actually check if BZ#30988 is really fixed.

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

[Carlos O'Donell]

On 11/6/23 15:46, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/11/23 14:56, Adhemerval Zanella Netto wrote:
>>
>>
>> On 06/11/23 14:38, Carlos O'Donell wrote:
>>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>>>
>>>>
>>>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>>>
>>>>>>
>>>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>>>> 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).
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>>>> should test for that.
>>>>>>>
>>>>>>>> ---
>>>>>>>>  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)
>>>>>>>
>>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>>>
>>>>>>> e.g.
>>>>>>>
>>>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>>>     { 
>>>>>>>       if (ret == 0)
>>>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>>>       else
>>>>>>>         /* fail */
>>>>>>>     }
>>>>>>>   else
>>>>>>>     {
>>>>>>>       if (ret == 0)
>>>>>>>         /* fail */
>>>>>>>       else
>>>>>>>         /* pass */
>>>>>>>     }
>>>>>>
>>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>>>> failure.
>>>>>
>>>>> Sure.
>>>>>
>>>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>>>
>>>>>
>>>>> So we go from UNSUPPORTED to... ?
>>>>>
>>>>
>>>> I though about that, but the test also checks fegetexceptflag (a better option would
>>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>>>
>>>
>>> Perhaps the best option is to just keep the UNSUPPORTED status?
>>>
>>
>> Fair enough.
> 
> Revising the patch, I recalled that I explicitly removed the UNSUPPORTED
> so the test can now check if the fesetexcept does fails with -1 for 
> !EXCEPTION_SET_FORCES_TRAP.  I am not sure if adding it back is an improvement,
> it means that it won't actually check if BZ#30988 is really fixed.
 
My apologies that we have gone around in a circle.

Let me start again.

And for the public record and your review I'll write down my assumptions.

(a) Previously calling fesetexcept() (ISO/IEC 60559) or fesetexceptflag() (ISO C)
    on POWER would raise a trap because the hardware can only raise the flag if
    it *also* forces a trap.

(b) In Bug 30988 (a) is raised as an ISO/IEC 60559 and ISO C conformance issue.
    And the fix is to return an error from fesetexcept() or fesetexceptflag() if
    the hardware cannot raise a flag without *also* forcing a trap (which fails
    to comply with the standard definition).

(c) In your patch 1/7 you want to remove the "return 77;" for the
    EXCEPTION_SET_FORCES_TRAP path because it can now be tested.

Given (c) my expectation is that we *actively* test for the failure.

Your test changes look they will cause POWER to now fail the test, particularly
since 'EXCEPTION_TESTS (float)' for POWER will always be true because we want
to test exceptions (it's just that our expectations are different).

Let me sketch out what I was expecting for both test cases:

diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
index 71b6e45b33..5ea295a5b8 100644
--- a/math/test-fesetexcept-traps.c
+++ b/math/test-fesetexcept-traps.c
@@ -23,46 +23,97 @@
 static int
 do_test (void)
 {
-  int result = 0;
+  int errors = 0;
+  int ret;
 
   fedisableexcept (FE_ALL_EXCEPT);
-  int ret = feenableexcept (FE_ALL_EXCEPT);
+  ret = feenableexcept (FE_ALL_EXCEPT);
   if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
     {
-      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
+      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
       return 77;
     }
   else if (ret != 0)
     {
-      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
-      result = 1;
-      return result;
+      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
+      errors++;
+      return errors;
     }
 
-  if (EXCEPTION_SET_FORCES_TRAP)
+  if (!EXCEPTION_SET_FORCES_TRAP)
     {
-      puts ("setting exceptions traps, cannot test on this architecture");
-      return 77;
+      /* Verify fesetexcept does not cause exception traps.  */
+      ret = fesetexcept (FE_ALL_EXCEPT);
+      if (ret == 0)
+	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
+      else
+        {
+	  /* Some architectures are expected to fail.  */
+	  if (EXCEPTION_TESTS (float))
+	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
+		  "failed as expected because testing is disabled");
+	  else
+	    {
+	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
+	      errors++;
+	    }
+	}
+      ret = feclearexcept (FE_ALL_EXCEPT);
+      if (ret == 0)
+	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
+      else
+	{
+	  /* Some architectures are expected to fail.  */
+	  if (EXCEPTION_TESTS (float))
+	    {
+	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
+		    "failed as expected because testing is disabled");
+	    }
+	  else
+	    {
+	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
+	      errors++;
+	    }
+	}
     }
-  /* Verify fesetexcept does not cause exception traps.  */
-  ret = fesetexcept (FE_ALL_EXCEPT);
-  if (ret == 0)
-    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
   else
     {
-      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
-      if (EXCEPTION_TESTS (float))
+      /* Verify fesetexcept fails because the hardware cannot set the
+	 exceptions without also raising them.  */
+      ret = fesetexcept (FE_ALL_EXCEPT);
+      if (ret == 0)
 	{
-	  puts ("failure of fesetexcept was unexpected");
-	  result = 1;
+	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
+	  errors++;
 	}
       else
-	puts ("failure of fesetexcept OK");
+	{
+	  if (EXCEPTION_TESTS (float))
+	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
+		  "failed as expected because testing is disabled");
+	  else
+	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
+	}
+      ret = feclearexcept (FE_ALL_EXCEPT);
+      if (ret == 0)
+	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
+      else
+	{
+	  /* Some architectures are expected to fail.  */
+	  if (EXCEPTION_TESTS (float))
+	    {
+	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
+		    "failed as expected because testing is disabled");
+	    }
+	  else
+	    {
+	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
+	      errors++;
+	    }
+	}
     }
-  feclearexcept (FE_ALL_EXCEPT);
 
-  return result;
+  return errors;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
---

My point is that by changing the implementation we need to test a whole
different set of conditions now and the test needs expanding, likewise
with test-fexcept-traps.c.

We need two testing paths with different expectations?

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 23/11/23 18:47, Carlos O'Donell wrote:
> On 11/6/23 15:46, Adhemerval Zanella Netto wrote:
>>
>>
>> On 06/11/23 14:56, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 14:38, Carlos O'Donell wrote:
>>>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>>>>> 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).
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>>>>> should test for that.
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  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)
>>>>>>>>
>>>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>>>>
>>>>>>>> e.g.
>>>>>>>>
>>>>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>>>>     { 
>>>>>>>>       if (ret == 0)
>>>>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>>>>       else
>>>>>>>>         /* fail */
>>>>>>>>     }
>>>>>>>>   else
>>>>>>>>     {
>>>>>>>>       if (ret == 0)
>>>>>>>>         /* fail */
>>>>>>>>       else
>>>>>>>>         /* pass */
>>>>>>>>     }
>>>>>>>
>>>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>>>>> failure.
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>>>>
>>>>>>
>>>>>> So we go from UNSUPPORTED to... ?
>>>>>>
>>>>>
>>>>> I though about that, but the test also checks fegetexceptflag (a better option would
>>>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>>>>
>>>>
>>>> Perhaps the best option is to just keep the UNSUPPORTED status?
>>>>
>>>
>>> Fair enough.
>>
>> Revising the patch, I recalled that I explicitly removed the UNSUPPORTED
>> so the test can now check if the fesetexcept does fails with -1 for 
>> !EXCEPTION_SET_FORCES_TRAP.  I am not sure if adding it back is an improvement,
>> it means that it won't actually check if BZ#30988 is really fixed.
>  
> My apologies that we have gone around in a circle.
> 
> Let me start again.
> 
> And for the public record and your review I'll write down my assumptions.
> 
> (a) Previously calling fesetexcept() (ISO/IEC 60559) or fesetexceptflag() (ISO C)
>     on POWER would raise a trap because the hardware can only raise the flag if
>     it *also* forces a trap.
> 
> (b) In Bug 30988 (a) is raised as an ISO/IEC 60559 and ISO C conformance issue.
>     And the fix is to return an error from fesetexcept() or fesetexceptflag() if
>     the hardware cannot raise a flag without *also* forcing a trap (which fails
>     to comply with the standard definition).
> 
> (c) In your patch 1/7 you want to remove the "return 77;" for the
>     EXCEPTION_SET_FORCES_TRAP path because it can now be tested.
> 
> Given (c) my expectation is that we *actively* test for the failure.
> 
> Your test changes look they will cause POWER to now fail the test, particularly
> since 'EXCEPTION_TESTS (float)' for POWER will always be true because we want
> to test exceptions (it's just that our expectations are different).

It won't fail on powerpc (I actually tested using the gcc compile farm), because
EXCEPTION_TESTS (float) won't be checked:

  volatile double a = 1.0;
  volatile double b = a + a;
  math_force_eval (b);					 // It will trigger the exception
  volatile long double al = 1.0L;
  volatile long double bl = al + al;
  math_force_eval (bl);

  if (ret == 0)						 // ret will -1 here (this very fix)
    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
  else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1
    {
      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
      if (EXCEPTION_TESTS (float))
        {
          puts ("failure of fesetexcept was unexpected");
          result = 1;
        }
      else
        puts ("failure of fesetexcept OK");
    }

> 
> Let me sketch out what I was expecting for both test cases:
> 
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 71b6e45b33..5ea295a5b8 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-traps.c
> @@ -23,46 +23,97 @@
>  static int
>  do_test (void)
>  {
> -  int result = 0;
> +  int errors = 0;
> +  int ret;
>  
>    fedisableexcept (FE_ALL_EXCEPT);
> -  int ret = feenableexcept (FE_ALL_EXCEPT);
> +  ret = feenableexcept (FE_ALL_EXCEPT);
>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
>      {
> -      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
> +      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>        return 77;
>      }
>    else if (ret != 0)
>      {
> -      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
> -      result = 1;
> -      return result;
> +      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
> +      errors++;
> +      return errors;
>      }
>  
> -  if (EXCEPTION_SET_FORCES_TRAP)
> +  if (!EXCEPTION_SET_FORCES_TRAP)
>      {
> -      puts ("setting exceptions traps, cannot test on this architecture");
> -      return 77;
> +      /* Verify fesetexcept does not cause exception traps.  */
> +      ret = fesetexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
> +      else
> +        {
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
> +		  "failed as expected because testing is disabled");
> +	  else
> +	    {
> +	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
> +	      errors++;
> +	    }
> +	}
> +      ret = feclearexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
> +      else
> +	{
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    {
> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
> +		    "failed as expected because testing is disabled");
> +	    }
> +	  else
> +	    {
> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
> +	      errors++;
> +	    }
> +	}
>      }
> -  /* Verify fesetexcept does not cause exception traps.  */
> -  ret = fesetexcept (FE_ALL_EXCEPT);
> -  if (ret == 0)
> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>    else
>      {
> -      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
> -      if (EXCEPTION_TESTS (float))
> +      /* Verify fesetexcept fails because the hardware cannot set the
> +	 exceptions without also raising them.  */
> +      ret = fesetexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
>  	{
> -	  puts ("failure of fesetexcept was unexpected");
> -	  result = 1;
> +	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
> +	  errors++;
>  	}

I think this is essentially what you think my proposed change is incomplete,
I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be
possible that either kernel might paper over this limitation (by some instruction
emulation to hide the exception signal) or a new chip revision might eventually
fix it (as i686 did with SSE2).

Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure
expectation and trigger a regression is function succeeds.

>        else
> -	puts ("failure of fesetexcept OK");
> +	{
> +	  if (EXCEPTION_TESTS (float))
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
> +		  "failed as expected because testing is disabled");
> +	  else
> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
> +	}
> +      ret = feclearexcept (FE_ALL_EXCEPT);
> +      if (ret == 0)
> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
> +      else
> +	{
> +	  /* Some architectures are expected to fail.  */
> +	  if (EXCEPTION_TESTS (float))
> +	    {
> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
> +		    "failed as expected because testing is disabled");
> +	    }
> +	  else
> +	    {
> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
> +	      errors++;
> +	    }
> +	}
>      }
> -  feclearexcept (FE_ALL_EXCEPT);
>  
> -  return result;
> +  return errors;
>  }
>  
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> ---
> 
> My point is that by changing the implementation we need to test a whole
> different set of conditions now and the test needs expanding, likewise
> with test-fexcept-traps.c.
> 
> We need two testing paths with different expectations?

No really, the whole point of the test is to check:

    int exc_before = fegetexcept ();
    ret = fesetexcept (FE_ALL_EXCEPT);
    int exc_after = fegetexcept ();

Will not change the exception mask (exc_before == exc_after) *and* not generate
any trap (which you abort the process).  Also, for i686 we need to trigger some
math operations after the fesetexcept to check no exception will be triggered.

Now, if ret is 0 everything works as expected.  If ret is -1, it would depend
whether the architecture has EXCEPTION_SET_FORCES_TRAP: 

   * if is not set, it will depend whether the architectures allows setting
     the exception for the specific float type (EXCEPTION_TESTS (float), which
     is expanded to the constants defined by math-tests-exceptions.h).  Some
     architectures does not support exceptions at all (riscv), or it depends
     of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode).

   * if it is set (powerpc and i386/x87) it means that there is no extra
     checks requires, since the failure for these architectures *is*
     expected.

So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this
below would be suffice:

  if (ret == 0)
    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
  else if (!EXCEPTION_SET_FORCES_TRAP)
    {
      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
      if (EXCEPTION_TESTS (float))
        {
          puts ("failure of fesetexcept was unexpected");
          result = 1;
        }
      else
        puts ("failure of fesetexcept OK");
    }
  else
    {
      if (ret == 0)
        puts ("unexpected fesetexcept success");
      result = ret != -1;
    }

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

[Adhemerval Zanella Netto]

On 24/11/23 09:28, Adhemerval Zanella Netto wrote:
> 
> 
> On 23/11/23 18:47, Carlos O'Donell wrote:
>> On 11/6/23 15:46, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 06/11/23 14:56, Adhemerval Zanella Netto wrote:
>>>>
>>>>
>>>> On 06/11/23 14:38, Carlos O'Donell wrote:
>>>>> On 11/6/23 12:11, Adhemerval Zanella Netto wrote:
>>>>>>
>>>>>>
>>>>>> On 06/11/23 14:02, Carlos O'Donell wrote:
>>>>>>> On 11/6/23 11:50, Adhemerval Zanella Netto wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/11/23 13:08, Carlos O'Donell wrote:
>>>>>>>>> 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).
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> OK. I reviewed ISO C 2x (n3096), and I agree this is permissible and preferable.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Changes test from UNSUPPORTED to PASS when we should test more now that with
>>>>>>>>> C2x we're saying the behaviour will result in a non-zero return... then we
>>>>>>>>> should test for that.
>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>  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)
>>>>>>>>>
>>>>>>>>> We can check for a non-zero return if EXCEPTION_SET_FORCES_TRAP?
>>>>>>>>>
>>>>>>>>> e.g.
>>>>>>>>>
>>>>>>>>>   if (!EXCEPTION_SET_FORCES_TRAP)
>>>>>>>>>     { 
>>>>>>>>>       if (ret == 0)
>>>>>>>>>         puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>>>>>>>       else
>>>>>>>>>         /* fail */
>>>>>>>>>     }
>>>>>>>>>   else
>>>>>>>>>     {
>>>>>>>>>       if (ret == 0)
>>>>>>>>>         /* fail */
>>>>>>>>>       else
>>>>>>>>>         /* pass */
>>>>>>>>>     }
>>>>>>>>
>>>>>>>> The '!EXCEPTION_SET_FORCES_TRAP && ret == 0' or 'EXCEPTION_SET_FORCES_TRAP && ret == 1'
>>>>>>>> checks are not really meaningful: either the function succeeds and return 0, or it fails
>>>>>>>> for some reason.  And for failure, EXCEPTION_SET_FORCES_TRAP really means an expected 
>>>>>>>> failure.
>>>>>>>
>>>>>>> Sure.
>>>>>>>
>>>>>>>> So if the function succeeds and no trap is generated (which terminates the process
>>>>>>>> as default on Linux) we are fine.  Otherwise, it check if the failure is expected
>>>>>>>> (EXCEPTION_SET_FORCES_TRAP).
>>>>>>>>
>>>>>>>
>>>>>>> So we go from UNSUPPORTED to... ?
>>>>>>>
>>>>>>
>>>>>> I though about that, but the test also checks fegetexceptflag (a better option would
>>>>>> to split the test in two, so only the fesetexceptflag is unsupported on ppc32).
>>>>>>
>>>>>
>>>>> Perhaps the best option is to just keep the UNSUPPORTED status?
>>>>>
>>>>
>>>> Fair enough.
>>>
>>> Revising the patch, I recalled that I explicitly removed the UNSUPPORTED
>>> so the test can now check if the fesetexcept does fails with -1 for 
>>> !EXCEPTION_SET_FORCES_TRAP.  I am not sure if adding it back is an improvement,
>>> it means that it won't actually check if BZ#30988 is really fixed.
>>  
>> My apologies that we have gone around in a circle.
>>
>> Let me start again.
>>
>> And for the public record and your review I'll write down my assumptions.
>>
>> (a) Previously calling fesetexcept() (ISO/IEC 60559) or fesetexceptflag() (ISO C)
>>     on POWER would raise a trap because the hardware can only raise the flag if
>>     it *also* forces a trap.
>>
>> (b) In Bug 30988 (a) is raised as an ISO/IEC 60559 and ISO C conformance issue.
>>     And the fix is to return an error from fesetexcept() or fesetexceptflag() if
>>     the hardware cannot raise a flag without *also* forcing a trap (which fails
>>     to comply with the standard definition).
>>
>> (c) In your patch 1/7 you want to remove the "return 77;" for the
>>     EXCEPTION_SET_FORCES_TRAP path because it can now be tested.
>>
>> Given (c) my expectation is that we *actively* test for the failure.
>>
>> Your test changes look they will cause POWER to now fail the test, particularly
>> since 'EXCEPTION_TESTS (float)' for POWER will always be true because we want
>> to test exceptions (it's just that our expectations are different).
> 
> It won't fail on powerpc (I actually tested using the gcc compile farm), because
> EXCEPTION_TESTS (float) won't be checked:
> 
>   volatile double a = 1.0;
>   volatile double b = a + a;
>   math_force_eval (b);					 // It will trigger the exception
>   volatile long double al = 1.0L;
>   volatile long double bl = al + al;
>   math_force_eval (bl);
> 
>   if (ret == 0)						 // ret will -1 here (this very fix)
>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>   else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1
>     {
>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>       if (EXCEPTION_TESTS (float))
>         {
>           puts ("failure of fesetexcept was unexpected");
>           result = 1;
>         }
>       else
>         puts ("failure of fesetexcept OK");
>     }
> 
>>
>> Let me sketch out what I was expecting for both test cases:
>>
>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
>> index 71b6e45b33..5ea295a5b8 100644
>> --- a/math/test-fesetexcept-traps.c
>> +++ b/math/test-fesetexcept-traps.c
>> @@ -23,46 +23,97 @@
>>  static int
>>  do_test (void)
>>  {
>> -  int result = 0;
>> +  int errors = 0;
>> +  int ret;
>>  
>>    fedisableexcept (FE_ALL_EXCEPT);
>> -  int ret = feenableexcept (FE_ALL_EXCEPT);
>> +  ret = feenableexcept (FE_ALL_EXCEPT);
>>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
>>      {
>> -      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>> +      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>>        return 77;
>>      }
>>    else if (ret != 0)
>>      {
>> -      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
>> -      result = 1;
>> -      return result;
>> +      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
>> +      errors++;
>> +      return errors;
>>      }
>>  
>> -  if (EXCEPTION_SET_FORCES_TRAP)
>> +  if (!EXCEPTION_SET_FORCES_TRAP)
>>      {
>> -      puts ("setting exceptions traps, cannot test on this architecture");
>> -      return 77;
>> +      /* Verify fesetexcept does not cause exception traps.  */
>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
>> +      else
>> +        {
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>> +		  "failed as expected because testing is disabled");
>> +	  else
>> +	    {
>> +	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
>> +	      errors++;
>> +	    }
>> +	}
>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>> +      else
>> +	{
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    {
>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>> +		    "failed as expected because testing is disabled");
>> +	    }
>> +	  else
>> +	    {
>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>> +	      errors++;
>> +	    }
>> +	}
>>      }
>> -  /* Verify fesetexcept does not cause exception traps.  */
>> -  ret = fesetexcept (FE_ALL_EXCEPT);
>> -  if (ret == 0)
>> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>    else
>>      {
>> -      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>> -      if (EXCEPTION_TESTS (float))
>> +      /* Verify fesetexcept fails because the hardware cannot set the
>> +	 exceptions without also raising them.  */
>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>>  	{
>> -	  puts ("failure of fesetexcept was unexpected");
>> -	  result = 1;
>> +	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
>> +	  errors++;
>>  	}
> 
> I think this is essentially what you think my proposed change is incomplete,
> I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be
> possible that either kernel might paper over this limitation (by some instruction
> emulation to hide the exception signal) or a new chip revision might eventually
> fix it (as i686 did with SSE2).
> 
> Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure
> expectation and trigger a regression is function succeeds.
> 
>>        else
>> -	puts ("failure of fesetexcept OK");
>> +	{
>> +	  if (EXCEPTION_TESTS (float))
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>> +		  "failed as expected because testing is disabled");
>> +	  else
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
>> +	}
>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>> +      else
>> +	{
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    {
>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>> +		    "failed as expected because testing is disabled");
>> +	    }
>> +	  else
>> +	    {
>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>> +	      errors++;
>> +	    }
>> +	}
>>      }
>> -  feclearexcept (FE_ALL_EXCEPT);
>>  
>> -  return result;
>> +  return errors;
>>  }
>>  
>> -#define TEST_FUNCTION do_test ()
>> -#include "../test-skeleton.c"
>> +#include <support/test-driver.c>
>> ---
>>
>> My point is that by changing the implementation we need to test a whole
>> different set of conditions now and the test needs expanding, likewise
>> with test-fexcept-traps.c.
>>
>> We need two testing paths with different expectations?
> 
> No really, the whole point of the test is to check:
> 
>     int exc_before = fegetexcept ();
>     ret = fesetexcept (FE_ALL_EXCEPT);
>     int exc_after = fegetexcept ();
> 
> Will not change the exception mask (exc_before == exc_after) *and* not generate
> any trap (which you abort the process).  Also, for i686 we need to trigger some
> math operations after the fesetexcept to check no exception will be triggered.
> 
> Now, if ret is 0 everything works as expected.  If ret is -1, it would depend
> whether the architecture has EXCEPTION_SET_FORCES_TRAP: 
> 
>    * if is not set, it will depend whether the architectures allows setting
>      the exception for the specific float type (EXCEPTION_TESTS (float), which
>      is expanded to the constants defined by math-tests-exceptions.h).  Some
>      architectures does not support exceptions at all (riscv), or it depends
>      of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode).
> 
>    * if it is set (powerpc and i386/x87) it means that there is no extra
>      checks requires, since the failure for these architectures *is*
>      expected.
> 
> So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this
> below would be suffice:
> 
>   if (ret == 0)
>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>   else if (!EXCEPTION_SET_FORCES_TRAP)
>     {
>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>       if (EXCEPTION_TESTS (float))
>         {
>           puts ("failure of fesetexcept was unexpected");
>           result = 1;
>         }
>       else
>         puts ("failure of fesetexcept OK");
>     }
>   else
>     {
>       if (ret == 0)
>         puts ("unexpected fesetexcept success");
>       result = ret != -1;
>     }

Oops, the above does not make sense:

  if (ret == 0)
    {
      if (EXCEPTION_SET_FORCES_TRAP)
        {
          puts ("unexpected fesetexcept success");
          result = 1;
        }
    }
  else if (!EXCEPTION_SET_FORCES_TRAP)
    {
      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
      if (EXCEPTION_TESTS (float))
        {
          puts ("failure of fesetexcept was unexpected");
          result = 1;
        }
      else
        puts ("failure of fesetexcept OK");
    }

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

[Carlos O'Donell]

On 11/24/23 07:28, Adhemerval Zanella Netto wrote:
> It won't fail on powerpc (I actually tested using the gcc compile farm), because
> EXCEPTION_TESTS (float) won't be checked:
> 
>   volatile double a = 1.0;
>   volatile double b = a + a;
>   math_force_eval (b);					 // It will trigger the exception
>   volatile long double al = 1.0L;
>   volatile long double bl = al + al;
>   math_force_eval (bl);
> 
>   if (ret == 0)						 // ret will -1 here (this very fix)

OK. Agreed.

>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>   else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1

OK. Agreed.

>     {
>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>       if (EXCEPTION_TESTS (float))
>         {
>           puts ("failure of fesetexcept was unexpected");
>           result = 1;

Where do we set EXCEPTION_TESTS (float) to zero for POWER?

sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float	1
sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_double	1
sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_long_double	1
sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float128	1


>         }
>       else
>         puts ("failure of fesetexcept OK");
>     }
> 
>>
>> Let me sketch out what I was expecting for both test cases:
>>
>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
>> index 71b6e45b33..5ea295a5b8 100644
>> --- a/math/test-fesetexcept-traps.c
>> +++ b/math/test-fesetexcept-traps.c
>> @@ -23,46 +23,97 @@
>>  static int
>>  do_test (void)
>>  {
>> -  int result = 0;
>> +  int errors = 0;
>> +  int ret;
>>  
>>    fedisableexcept (FE_ALL_EXCEPT);
>> -  int ret = feenableexcept (FE_ALL_EXCEPT);
>> +  ret = feenableexcept (FE_ALL_EXCEPT);
>>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
>>      {
>> -      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>> +      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>>        return 77;
>>      }
>>    else if (ret != 0)
>>      {
>> -      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
>> -      result = 1;
>> -      return result;
>> +      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
>> +      errors++;
>> +      return errors;
>>      }
>>  
>> -  if (EXCEPTION_SET_FORCES_TRAP)
>> +  if (!EXCEPTION_SET_FORCES_TRAP)
>>      {
>> -      puts ("setting exceptions traps, cannot test on this architecture");
>> -      return 77;
>> +      /* Verify fesetexcept does not cause exception traps.  */
>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
>> +      else
>> +        {
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>> +		  "failed as expected because testing is disabled");
>> +	  else
>> +	    {
>> +	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
>> +	      errors++;
>> +	    }
>> +	}
>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>> +      else
>> +	{
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    {
>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>> +		    "failed as expected because testing is disabled");
>> +	    }
>> +	  else
>> +	    {
>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>> +	      errors++;
>> +	    }
>> +	}
>>      }
>> -  /* Verify fesetexcept does not cause exception traps.  */
>> -  ret = fesetexcept (FE_ALL_EXCEPT);
>> -  if (ret == 0)
>> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>    else
>>      {
>> -      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>> -      if (EXCEPTION_TESTS (float))
>> +      /* Verify fesetexcept fails because the hardware cannot set the
>> +	 exceptions without also raising them.  */
>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>>  	{
>> -	  puts ("failure of fesetexcept was unexpected");
>> -	  result = 1;
>> +	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
>> +	  errors++;
>>  	}
> 
> I think this is essentially what you think my proposed change is incomplete,
> I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be
> possible that either kernel might paper over this limitation (by some instruction
> emulation to hide the exception signal) or a new chip revision might eventually
> fix it (as i686 did with SSE2).
> 
> Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure
> expectation and trigger a regression is function succeeds.

Correct, if the function succeeds then it is a failure, it's likely someone broke
the conditional and now we have a function that is back to raising traps by
accident like it was before. It is a regression of bug 30988 if it succeeds.

> 
>>        else
>> -	puts ("failure of fesetexcept OK");
>> +	{
>> +	  if (EXCEPTION_TESTS (float))
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>> +		  "failed as expected because testing is disabled");
>> +	  else
>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
>> +	}
>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>> +      if (ret == 0)
>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>> +      else
>> +	{
>> +	  /* Some architectures are expected to fail.  */
>> +	  if (EXCEPTION_TESTS (float))
>> +	    {
>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>> +		    "failed as expected because testing is disabled");
>> +	    }
>> +	  else
>> +	    {
>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>> +	      errors++;
>> +	    }
>> +	}
>>      }
>> -  feclearexcept (FE_ALL_EXCEPT);
>>  
>> -  return result;
>> +  return errors;
>>  }
>>  
>> -#define TEST_FUNCTION do_test ()
>> -#include "../test-skeleton.c"
>> +#include <support/test-driver.c>
>> ---
>>
>> My point is that by changing the implementation we need to test a whole
>> different set of conditions now and the test needs expanding, likewise
>> with test-fexcept-traps.c.
>>
>> We need two testing paths with different expectations?
> 
> No really, the whole point of the test is to check:
> 
>     int exc_before = fegetexcept ();
>     ret = fesetexcept (FE_ALL_EXCEPT);
>     int exc_after = fegetexcept ();
> 
> Will not change the exception mask (exc_before == exc_after) *and* not generate
> any trap (which you abort the process).  Also, for i686 we need to trigger some
> math operations after the fesetexcept to check no exception will be triggered.
> 
> Now, if ret is 0 everything works as expected.  If ret is -1, it would depend
> whether the architecture has EXCEPTION_SET_FORCES_TRAP: 
> 
>    * if is not set, it will depend whether the architectures allows setting
>      the exception for the specific float type (EXCEPTION_TESTS (float), which
>      is expanded to the constants defined by math-tests-exceptions.h).  Some
>      architectures does not support exceptions at all (riscv), or it depends
>      of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode).

Agreed.

> 
>    * if it is set (powerpc and i386/x87) it means that there is no extra
>      checks requires, since the failure for these architectures *is*
>      expected.

Agreed. Though EXCEPTION_TESTS is still relevant here to avoid regression.

> 
> So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this
> below would be suffice:
> 
>   if (ret == 0)
>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>   else if (!EXCEPTION_SET_FORCES_TRAP)
>     {
>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>       if (EXCEPTION_TESTS (float))
>         {
>           puts ("failure of fesetexcept was unexpected");
>           result = 1;
>         }
>       else
>         puts ("failure of fesetexcept OK");
>     }
>   else
>     {
>       if (ret == 0)
>         puts ("unexpected fesetexcept success");
>       result = ret != -1;
>     }
> 

Pasted below from downthread correction:

> Oops, the above does not make sense:
> 
>   if (ret == 0)
>     {
>       if (EXCEPTION_SET_FORCES_TRAP)
>         {
>           puts ("unexpected fesetexcept success");
>           result = 1;

Yes, this catches a POWER target regression for bug 30988.

For the sake of completeness and the use of internal macro APIs
it is conceivable that EXCEPTION_TESTS could be used to check if the
test should even be checked (like my suggestion does).

I consider it a simplification that you are applying target
knowledge from other files in the tree to skip that check
i.e. you know there is no EXCEPTION_SET_FORCES_TRAP true target that
is also EXCEPTION_TESTS true target.

Is it correct to apply that simplification to this code?

Or should the code handle both EXCEPTION_SET_FORCES_TRAP and
EXCEPTION_TESTS permutations in a generic fashion?

>         }
>     }
>   else if (!EXCEPTION_SET_FORCES_TRAP)

OK. This is all other architecture failure paths.

>     {
>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");

OK.

>       if (EXCEPTION_TESTS (float))
>         {
>           puts ("failure of fesetexcept was unexpected");
>           result = 1;

OK. This is the failure path for all targets that can do these operations.

>         }
>       else
>         puts ("failure of fesetexcept OK");

OK. Because it shouldn't be tested e.g. np-fpu targets.

>     }



-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 24/11/23 13:22, Carlos O'Donell wrote:
> On 11/24/23 07:28, Adhemerval Zanella Netto wrote:
>> It won't fail on powerpc (I actually tested using the gcc compile farm), because
>> EXCEPTION_TESTS (float) won't be checked:
>>
>>   volatile double a = 1.0;
>>   volatile double b = a + a;
>>   math_force_eval (b);					 // It will trigger the exception
>>   volatile long double al = 1.0L;
>>   volatile long double bl = al + al;
>>   math_force_eval (bl);
>>
>>   if (ret == 0)						 // ret will -1 here (this very fix)
> 
> OK. Agreed.
> 
>>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>   else if (!EXCEPTION_SET_FORCES_TRAP)			 // EXCEPTION_SET_FORCES_TRAP is set to 1
> 
> OK. Agreed.
> 
>>     {
>>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>>       if (EXCEPTION_TESTS (float))
>>         {
>>           puts ("failure of fesetexcept was unexpected");
>>           result = 1;
> 
> Where do we set EXCEPTION_TESTS (float) to zero for POWER?
> 
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_double	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_long_double	1
> sysdeps/generic/math-tests-exceptions.h:#define EXCEPTION_TESTS_float128	1

We don't, powerpc does support exceptions.  The issues is a powerpc limitation
that Bruno has pointed out in BZ 30988:

  setting a floating-point exception flag triggers a trap, when traps are enabled
  for the particular exception and globally for the thread (via
  prctl (PR_SET_FPEXC, PR_FP_EXC_PRECISE)).

It is because feenableexcept on powerpc enables the PR_FP_EXC_PRECISE mode.

> 
> 
>>         }
>>       else
>>         puts ("failure of fesetexcept OK");
>>     }
>>
>>>
>>> Let me sketch out what I was expecting for both test cases:
>>>
>>> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
>>> index 71b6e45b33..5ea295a5b8 100644
>>> --- a/math/test-fesetexcept-traps.c
>>> +++ b/math/test-fesetexcept-traps.c
>>> @@ -23,46 +23,97 @@
>>>  static int
>>>  do_test (void)
>>>  {
>>> -  int result = 0;
>>> +  int errors = 0;
>>> +  int ret;
>>>  
>>>    fedisableexcept (FE_ALL_EXCEPT);
>>> -  int ret = feenableexcept (FE_ALL_EXCEPT);
>>> +  ret = feenableexcept (FE_ALL_EXCEPT);
>>>    if (!EXCEPTION_ENABLE_SUPPORTED (FE_ALL_EXCEPT) && (ret == -1))
>>>      {
>>> -      puts ("feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>>> +      puts ("UNSUPPORTED: feenableexcept (FE_ALL_EXCEPT) not supported, cannot test");
>>>        return 77;
>>>      }
>>>    else if (ret != 0)
>>>      {
>>> -      puts ("feenableexcept (FE_ALL_EXCEPT) failed");
>>> -      result = 1;
>>> -      return result;
>>> +      puts ("FAIL: feenableexcept (FE_ALL_EXCEPT)");
>>> +      errors++;
>>> +      return errors;
>>>      }
>>>  
>>> -  if (EXCEPTION_SET_FORCES_TRAP)
>>> +  if (!EXCEPTION_SET_FORCES_TRAP)
>>>      {
>>> -      puts ("setting exceptions traps, cannot test on this architecture");
>>> -      return 77;
>>> +      /* Verify fesetexcept does not cause exception traps.  */
>>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>>> +      if (ret == 0)
>>> +	puts ("PASS: fesetexcept (FE_ALL_EXCEPT)");
>>> +      else
>>> +        {
>>> +	  /* Some architectures are expected to fail.  */
>>> +	  if (EXCEPTION_TESTS (float))
>>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>>> +		  "failed as expected because testing is disabled");
>>> +	  else
>>> +	    {
>>> +	      puts ("FAIL: fesetexcept (FE_ALL_EXCEPT)");
>>> +	      errors++;
>>> +	    }
>>> +	}
>>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>>> +      if (ret == 0)
>>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>>> +      else
>>> +	{
>>> +	  /* Some architectures are expected to fail.  */
>>> +	  if (EXCEPTION_TESTS (float))
>>> +	    {
>>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>>> +		    "failed as expected because testing is disabled");
>>> +	    }
>>> +	  else
>>> +	    {
>>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>>> +	      errors++;
>>> +	    }
>>> +	}
>>>      }
>>> -  /* Verify fesetexcept does not cause exception traps.  */
>>> -  ret = fesetexcept (FE_ALL_EXCEPT);
>>> -  if (ret == 0)
>>> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>>    else
>>>      {
>>> -      puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>>> -      if (EXCEPTION_TESTS (float))
>>> +      /* Verify fesetexcept fails because the hardware cannot set the
>>> +	 exceptions without also raising them.  */
>>> +      ret = fesetexcept (FE_ALL_EXCEPT);
>>> +      if (ret == 0)
>>>  	{
>>> -	  puts ("failure of fesetexcept was unexpected");
>>> -	  result = 1;
>>> +	  puts ("FAIL: fesetexcept (FE_ALL_EXCEPT) succeeded unexpectedly");
>>> +	  errors++;
>>>  	}
>>
>> I think this is essentially what you think my proposed change is incomplete,
>> I assume that EXCEPTION_SET_FORCES_TRAP is a hit since I think it might be
>> possible that either kernel might paper over this limitation (by some instruction
>> emulation to hide the exception signal) or a new chip revision might eventually
>> fix it (as i686 did with SSE2).
>>
>> Maybe it would be better to assume that EXCEPTION_SET_FORCES_TRAP is a failure
>> expectation and trigger a regression is function succeeds.
> 
> Correct, if the function succeeds then it is a failure, it's likely someone broke
> the conditional and now we have a function that is back to raising traps by
> accident like it was before. It is a regression of bug 30988 if it succeeds.

Agreed.

> 
>>
>>>        else
>>> -	puts ("failure of fesetexcept OK");
>>> +	{
>>> +	  if (EXCEPTION_TESTS (float))
>>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) "
>>> +		  "failed as expected because testing is disabled");
>>> +	  else
>>> +	    puts ("PASS: fesetexcept (FE_ALL_EXCEPT) failed as expected");
>>> +	}
>>> +      ret = feclearexcept (FE_ALL_EXCEPT);
>>> +      if (ret == 0)
>>> +	puts ("PASS: feclearexcept (FE_ALL_EXCEPT)");
>>> +      else
>>> +	{
>>> +	  /* Some architectures are expected to fail.  */
>>> +	  if (EXCEPTION_TESTS (float))
>>> +	    {
>>> +	      puts ("PASS: feclearexcept (FE_ALL_EXCEPT) "
>>> +		    "failed as expected because testing is disabled");
>>> +	    }
>>> +	  else
>>> +	    {
>>> +	      puts ("FAIL: feclearexcept (FE_ALL_EXCEPT) failed");
>>> +	      errors++;
>>> +	    }
>>> +	}
>>>      }
>>> -  feclearexcept (FE_ALL_EXCEPT);
>>>  
>>> -  return result;
>>> +  return errors;
>>>  }
>>>  
>>> -#define TEST_FUNCTION do_test ()
>>> -#include "../test-skeleton.c"
>>> +#include <support/test-driver.c>
>>> ---
>>>
>>> My point is that by changing the implementation we need to test a whole
>>> different set of conditions now and the test needs expanding, likewise
>>> with test-fexcept-traps.c.
>>>
>>> We need two testing paths with different expectations?
>>
>> No really, the whole point of the test is to check:
>>
>>     int exc_before = fegetexcept ();
>>     ret = fesetexcept (FE_ALL_EXCEPT);
>>     int exc_after = fegetexcept ();
>>
>> Will not change the exception mask (exc_before == exc_after) *and* not generate
>> any trap (which you abort the process).  Also, for i686 we need to trigger some
>> math operations after the fesetexcept to check no exception will be triggered.
>>
>> Now, if ret is 0 everything works as expected.  If ret is -1, it would depend
>> whether the architecture has EXCEPTION_SET_FORCES_TRAP: 
>>
>>    * if is not set, it will depend whether the architectures allows setting
>>      the exception for the specific float type (EXCEPTION_TESTS (float), which
>>      is expanded to the constants defined by math-tests-exceptions.h).  Some
>>      architectures does not support exceptions at all (riscv), or it depends
>>      of the ABI (arc, arm, loongarch, and ork1 in soft-fp mode).
> 
> Agreed.
> 
>>
>>    * if it is set (powerpc and i386/x87) it means that there is no extra
>>      checks requires, since the failure for these architectures *is*
>>      expected.
> 
> Agreed. Though EXCEPTION_TESTS is still relevant here to avoid regression.
> 
>>
>> So assuming EXCEPTION_SET_FORCES_TRAP is a hard indication, I think this
>> below would be suffice:
>>
>>   if (ret == 0)
>>     puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
>>   else if (!EXCEPTION_SET_FORCES_TRAP)
>>     {
>>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
>>       if (EXCEPTION_TESTS (float))
>>         {
>>           puts ("failure of fesetexcept was unexpected");
>>           result = 1;
>>         }
>>       else
>>         puts ("failure of fesetexcept OK");
>>     }
>>   else
>>     {
>>       if (ret == 0)
>>         puts ("unexpected fesetexcept success");
>>       result = ret != -1;
>>     }
>>
> 
> Pasted below from downthread correction:
> 
>> Oops, the above does not make sense:
>>
>>   if (ret == 0)
>>     {
>>       if (EXCEPTION_SET_FORCES_TRAP)
>>         {
>>           puts ("unexpected fesetexcept success");
>>           result = 1;
> 
> Yes, this catches a POWER target regression for bug 30988.
> 
> For the sake of completeness and the use of internal macro APIs
> it is conceivable that EXCEPTION_TESTS could be used to check if the
> test should even be checked (like my suggestion does).> 
> I consider it a simplification that you are applying target
> knowledge from other files in the tree to skip that check
> i.e. you know there is no EXCEPTION_SET_FORCES_TRAP true target that
> is also EXCEPTION_TESTS true target.
> 
> Is it correct to apply that simplification to this code?
> 
> Or should the code handle both EXCEPTION_SET_FORCES_TRAP and
> EXCEPTION_TESTS permutations in a generic fashion?

I think the simplification applies here, because the issue is a 
powerpc/x87 architecture limitation here setting the floating-point 
register status will trigger a floating point exception (x87 would 
trigger in the next floating point operation, but it is essentially 
the same issue).

So the fesetexcept/fesetexceptflag would either:

  1. Raise a floating point exception, aborting the testcase (current
     code).
  2. Fail where it should not.
  3. Rail where it should (powerpc/x87).
  4. Succeeds.

So 1. and 2. are considered a regression, where 3. and 4. is the
expected result.

> 
>>         }
>>     }
>>   else if (!EXCEPTION_SET_FORCES_TRAP)
> 
> OK. This is all other architecture failure paths.
> 
>>     {
>>       puts ("fesetexcept (FE_ALL_EXCEPT) failed");
> 
> OK.
> 
>>       if (EXCEPTION_TESTS (float))
>>         {
>>           puts ("failure of fesetexcept was unexpected");
>>           result = 1;
> 
> OK. This is the failure path for all targets that can do these operations.
> 
>>         }
>>       else
>>         puts ("failure of fesetexcept OK");
> 
> OK. Because it shouldn't be tested e.g. np-fpu targets.
> 
>>     }
> 
> 
> 

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

[Carlos O'Donell]

On 11/24/23 12:53, Adhemerval Zanella Netto wrote:
> I think the simplification applies here, because the issue is a 
> powerpc/x87 architecture limitation here setting the floating-point 
> register status will trigger a floating point exception (x87 would 
> trigger in the next floating point operation, but it is essentially 
> the same issue).

In which case I'd rather the test code assert that the two macros can't be
used that way in the test with a macro check or static assert that
checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true.
 
> So the fesetexcept/fesetexceptflag would either:
> 
>   1. Raise a floating point exception, aborting the testcase (current
>      code).
>   2. Fail where it should not.
>   3. Rail where it should (powerpc/x87).
>   4. Succeeds.

5. Succeed where it should not (powerpc/x87) ?

> 
> So 1. and 2. are considered a regression, where 3. and 4. is the
> expected result.

... and 5?

-- 
Cheers,
Carlos.

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

[Adhemerval Zanella Netto]

On 24/11/23 15:15, Carlos O'Donell wrote:
> On 11/24/23 12:53, Adhemerval Zanella Netto wrote:
>> I think the simplification applies here, because the issue is a 
>> powerpc/x87 architecture limitation here setting the floating-point 
>> register status will trigger a floating point exception (x87 would 
>> trigger in the next floating point operation, but it is essentially 
>> the same issue).
> 
> In which case I'd rather the test code assert that the two macros can't be
> used that way in the test with a macro check or static assert that
> checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true.

I think you mean EXCEPTION_TESTS (float), and it seems reasonable:

  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
                 "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
                 "architecture suports exceptions");

>  
>> So the fesetexcept/fesetexceptflag would either:
>>
>>   1. Raise a floating point exception, aborting the testcase (current
>>      code).
>>   2. Fail where it should not.
>>   3. Rail where it should (powerpc/x87).
>>   4. Succeeds.
> 
> 5. Succeed where it should not (powerpc/x87) ?

Indeed.

> 
>>
>> So 1. and 2. are considered a regression, where 3. and 4. is the
>> expected result.
> 
> ... and 5?

Yes, and 1., 2., and 5. should be considered a regression.

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

[Adhemerval Zanella Netto]

On 24/11/23 15:46, Adhemerval Zanella Netto wrote:
> 
> 
> On 24/11/23 15:15, Carlos O'Donell wrote:
>> On 11/24/23 12:53, Adhemerval Zanella Netto wrote:
>>> I think the simplification applies here, because the issue is a 
>>> powerpc/x87 architecture limitation here setting the floating-point 
>>> register status will trigger a floating point exception (x87 would 
>>> trigger in the next floating point operation, but it is essentially 
>>> the same issue).
>>
>> In which case I'd rather the test code assert that the two macros can't be
>> used that way in the test with a macro check or static assert that
>> checks 'EXCEPTION_SET_FORCES_TRAP && !FLOATING_TESTS(float)' is true.
> 
> I think you mean EXCEPTION_TESTS (float), and it seems reasonable:
> 
>   _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
>                  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
>                  "architecture suports exceptions");
> 
>>  
>>> So the fesetexcept/fesetexceptflag would either:
>>>
>>>   1. Raise a floating point exception, aborting the testcase (current
>>>      code).
>>>   2. Fail where it should not.
>>>   3. Rail where it should (powerpc/x87).
>>>   4. Succeeds.
>>
>> 5. Succeed where it should not (powerpc/x87) ?
> 
> Indeed.
> 
>>
>>>
>>> So 1. and 2. are considered a regression, where 3. and 4. is the
>>> expected result.
>>
>> ... and 5?
> 
> Yes, and 1., 2., and 5. should be considered a regression.

Hi Carlos,

Updated patch below:

--

From c10d1d59321d3fce0c532304b428dcef3c8cbb3a Mon Sep 17 00:00:00 2001
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Tue, 24 Oct 2023 08:37:14 -0300
Subject: [PATCH 1/7] powerpc: Do not raise exception traps for
 fesetexcept/fesetexceptflag (BZ 30988)

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      | 24 ++++++++++++++++--------
 math/test-fexcept-traps.c          | 16 +++++++++-------
 sysdeps/powerpc/fpu/fesetexcept.c  |  5 +++++
 sysdeps/powerpc/fpu/fsetexcptflg.c |  9 ++++++++-
 4 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
index 71b6e45b33..7efcd0343a 100644
--- a/math/test-fesetexcept-traps.c
+++ b/math/test-fesetexcept-traps.c
@@ -39,16 +39,24 @@ 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);
+
+  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
+		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
+		  "architecture suports exceptions");
+
   if (ret == 0)
-    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
-  else
+    {
+      if (EXCEPTION_SET_FORCES_TRAP)
+	{
+	  puts ("unexpected fesetexcept success");
+	  result = 1;
+	}
+    }
+  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..998c241058 100644
--- a/math/test-fexcept-traps.c
+++ b/math/test-fexcept-traps.c
@@ -63,14 +63,16 @@ 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)
+
+  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
+		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
+		  "architecture suports exceptions");
+
+  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

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

[Carlos O'Donell]

On 11/27/23 08:46, Adhemerval Zanella Netto wrote:
> Hi Carlos,
> 
> Updated patch below:

LGTM, but because this is submitted under the existing thread patchwork doesn't
consider this a submission.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> --
> 
> From c10d1d59321d3fce0c532304b428dcef3c8cbb3a Mon Sep 17 00:00:00 2001
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date: Tue, 24 Oct 2023 08:37:14 -0300
> Subject: [PATCH 1/7] powerpc: Do not raise exception traps for
>  fesetexcept/fesetexceptflag (BZ 30988)
> 
> 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      | 24 ++++++++++++++++--------
>  math/test-fexcept-traps.c          | 16 +++++++++-------
>  sysdeps/powerpc/fpu/fesetexcept.c  |  5 +++++
>  sysdeps/powerpc/fpu/fsetexcptflg.c |  9 ++++++++-
>  4 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
> index 71b6e45b33..7efcd0343a 100644
> --- a/math/test-fesetexcept-traps.c
> +++ b/math/test-fesetexcept-traps.c
> @@ -39,16 +39,24 @@ 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.  */

OK.

>    ret = fesetexcept (FE_ALL_EXCEPT);
> +
> +  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
> +		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
> +		  "architecture suports exceptions");

OK. Good _Static_assert to avoid machine misconfiguration.

> +
>    if (ret == 0)
> -    puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
> -  else
> +    {
> +      if (EXCEPTION_SET_FORCES_TRAP)
> +	{
> +	  puts ("unexpected fesetexcept success");
> +	  result = 1;
> +	}
> +    }
> +  else if (!EXCEPTION_SET_FORCES_TRAP)

OK. Tests both sides pass/fail. I like this version.

>      {
>        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..998c241058 100644
> --- a/math/test-fexcept-traps.c
> +++ b/math/test-fexcept-traps.c
> @@ -63,14 +63,16 @@ 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.  */

OK.

>    ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
> -  if (ret != 0)
> +
> +  _Static_assert (!(EXCEPTION_SET_FORCES_TRAP && !EXCEPTION_TESTS(float)),
> +		  "EXCEPTION_SET_FORCES_TRAP only makes sense if the "
> +		  "architecture suports exceptions");

OK.

> +
> +  if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP)
>      {
>        puts ("fesetexceptflag failed");
>        result = 1;

OK.

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

-- 
Cheers,
Carlos.