public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/7] Multiple floating-point environment fixes
@ 2023-11-03 12:24 Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
                   ` (7 more replies)
  0 siblings, 8 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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                         | 132 +++++++++++++++++++++--
 math/test-fesetexcept-traps.c            |  37 +++++--
 math/test-fexcept-traps.c                |  33 ++++--
 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         |   6 +-
 sysdeps/x86/fpu/test-fenv-sse-2.c        |  23 +---
 sysdeps/x86_64/fpu/fsetexcptflg.c        |  24 +++--
 14 files changed, 332 insertions(+), 85 deletions(-)
 create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h

-- 
2.34.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-06  0:15   ` Bruno Haible
  2023-11-03 12:24 ` [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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..2e9580207c 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))
+    {
+      /* And now similarly for SSE.  */
+      unsigned int mxcsr;
+      __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
+
+      /* Set relevant flags.  */
+      mxcsr |= excepts;
+
+      /* Put the new data in effect.  */
+      __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
+    }
+  else
+    {
+      fenv_t temp;
+
+      /* 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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-06  0:17   ` Bruno Haible
  2023-11-03 12:24 ` [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019) Adhemerval Zanella
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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         | 22 ++++++++++-
 sysdeps/i386/fpu/fsetexcptflg.c   | 63 ++++++++++++++++++++-----------
 sysdeps/x86_64/fpu/fsetexcptflg.c | 24 +++++++-----
 3 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
index 9b8f583ae6..13aff229c3 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)
@@ -66,7 +67,26 @@ 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);
+  {
+    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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
                   ` (2 preceding siblings ...)
  2023-11-03 12:24 ` [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-03 16:20   ` Joseph Myers
  2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

From: Bruno Haible <bruno@clisp.org>

* manual/arith.texi (Control Functions): 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..68c6446a5b 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 undefined
+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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
                   ` (3 preceding siblings ...)
  2023-11-03 12:24 ` [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-06  0:21   ` Bruno Haible
                     ` (2 more replies)
  2023-11-03 12:24 ` [PATCH 6/7] alpha: Fix fesetexceptflag (BZ 30998) Adhemerval Zanella
                   ` (2 subsequent siblings)
  7 siblings, 3 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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                 | 132 ++++++++++++++++++++++++++++---
 sysdeps/riscv/rvf/fenv_private.h |   6 +-
 2 files changed, 127 insertions(+), 11 deletions(-)

diff --git a/math/test-fenv.c b/math/test-fenv.c
index 0af7141ba7..734e0a219d 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, fexcept_t 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,73 @@ feenv_tests (void)
   fesetenv (FE_DFL_ENV);
 }
 
+#if FE_ALL_EXCEPT
+static void
+feupdateenv_single_test (const char *test_name, int fe_exc,
+			 fexcept_t 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 +875,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 +892,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..c91d871160 100644
--- a/sysdeps/riscv/rvf/fenv_private.h
+++ b/sysdeps/riscv/rvf/fenv_private.h
@@ -123,7 +123,11 @@ 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 = (long int) envp - (long int) FE_DFL_ENV;
+  if (env != 0)
+    env = *envp;
+
+  _FPU_SETCW (env | riscv_getflags ());
 }
 
 #define libc_feupdateenv  libc_feupdateenv_riscv
-- 
2.34.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 6/7] alpha: Fix fesetexceptflag (BZ 30998)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
                   ` (4 preceding siblings ...)
  2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-03 12:24 ` [PATCH 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983) Adhemerval Zanella
  2023-11-06  0:25 ` [PATCH 0/7] Multiple floating-point environment fixes Bruno Haible
  7 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983)
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
                   ` (5 preceding siblings ...)
  2023-11-03 12:24 ` [PATCH 6/7] alpha: Fix fesetexceptflag (BZ 30998) Adhemerval Zanella
@ 2023-11-03 12:24 ` Adhemerval Zanella
  2023-11-06  0:25 ` [PATCH 0/7] Multiple floating-point environment fixes Bruno Haible
  7 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella @ 2023-11-03 12:24 UTC (permalink / raw)
  To: libc-alpha, Bruno Haible

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)
  2023-11-03 12:24 ` [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019) Adhemerval Zanella
@ 2023-11-03 16:20   ` Joseph Myers
  2023-11-06  0:17     ` Bruno Haible
  0 siblings, 1 reply; 22+ messages in thread
From: Joseph Myers @ 2023-11-03 16:20 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: libc-alpha, Bruno Haible

On Fri, 3 Nov 2023, Adhemerval Zanella wrote:

> +Note: Enabling traps for an exception for which the exception flag is
> +currently already set (@pxref{Status bit operations}) has undefined
> +consequences: it may or may not trigger a trap immediately.

I think we should say "unspecified", not "undefined" (in the manual and 
commit message).  It might trap immediately, on a future floating-point 
instruction or not at all, but there shouldn't be any other possibilities; 
it's not undefined behavior.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989)
  2023-11-03 12:24 ` [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
@ 2023-11-06  0:15   ` Bruno Haible
  2023-11-06 11:24     ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:15 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella

In sysdeps/i386/fpu/fesetexcept.c, I suggest a comment change:

  And now similarly for SSE.
->
  Get the control word of the SSE unit.


Bruno




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
  2023-11-03 12:24 ` [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
@ 2023-11-06  0:17   ` Bruno Haible
  2023-11-06 11:24     ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:17 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella

In math/test-fexcept-traps.c, around line 70, I would add the same comment as
in patch 2/7:

   Also check if the function does not alter the exception mask.  */


Bruno




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)
  2023-11-03 16:20   ` Joseph Myers
@ 2023-11-06  0:17     ` Bruno Haible
  2023-11-06 11:25       ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:17 UTC (permalink / raw)
  To: Adhemerval Zanella, Joseph Myers; +Cc: libc-alpha

Joseph Myers wrote:
> I think we should say "unspecified", not "undefined" (in the manual and 
> commit message).

I agree. Thanks Joseph for the more precise wording.

Bruno




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
@ 2023-11-06  0:21   ` Bruno Haible
  2023-11-06 11:28     ` Adhemerval Zanella Netto
  2023-11-06  0:24   ` Bruno Haible
  2024-01-09 13:44   ` Szabolcs Nagy
  2 siblings, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:21 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella

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

This code gave me puzzles:

  long int env = (long int) envp - (long int) FE_DFL_ENV;
  if (env != 0)
    env = *envp;

* What is the intent of the code?
* Can signed integer overflow happen in the first line? (It cannot, due to
  the value of FE_DFL_ENV being -1 and envp being otherwise aligned mod 4.
  But one should not have to think that far.)
* It assumes that intptr_t is the same as 'long int'. Which is not
  universally true.

I would suggest to replace the function with this code:

libc_feupdateenv_riscv (const fenv_t *envp)
{
  long int env = (envp != FE_DFL_ENV ? *envp : 0);

  _FPU_SETCW (env | riscv_getflags ());
}

* It is clearer.
* It does not make assumptions regarding FE_DFL_ENV or alignment.
* It produces the exact same code; see attached foo.c and foo.s,
  generated by "riscv64-linux-gnu-gcc -O2 -S foo.c".

Probably the idiom that you copied was meant to allow the use of a
conditional load instruction. But I would avoid such micro-optimizations
and instead rely on the compiler to choose the best instructions for a
task.

Bruno

[-- Attachment #2: foo.c --]
[-- Type: text/x-csrc, Size: 632 bytes --]

typedef unsigned int fenv_t;
#define FE_DFL_ENV      ((__const fenv_t *) -1)
# define _FPU_SETCW(cw) __asm__ volatile ("fssr %z0" : : "rJ" (cw))

static __attribute__ ((__always_inline__)) int
riscv_getflags (void)
{
  int flags;
  asm volatile ("frflags %0" : "=r" (flags));
  return flags;
}

void
libc_feupdateenv_riscv_az (const fenv_t *envp)
{
  long int env = (long int) envp - (long int) FE_DFL_ENV;
  if (env != 0)
    env = *envp;

  _FPU_SETCW (env | riscv_getflags ());
}

void
libc_feupdateenv_riscv_bh (const fenv_t *envp)
{
  long int env = (envp != FE_DFL_ENV ? *envp : 0);

  _FPU_SETCW (env | riscv_getflags ());
}

[-- Attachment #3: foo.s --]
[-- Type: text/plain, Size: 839 bytes --]

	.file	"foo.c"
	.option pic
	.text
	.align	1
	.globl	libc_feupdateenv_riscv_az
	.type	libc_feupdateenv_riscv_az, @function
libc_feupdateenv_riscv_az:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L2
	lwu	a4,0(a0)
.L2:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 20 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_az, .-libc_feupdateenv_riscv_az
	.align	1
	.globl	libc_feupdateenv_riscv_bh
	.type	libc_feupdateenv_riscv_bh, @function
libc_feupdateenv_riscv_bh:
	li	a5,-1
	li	a4,0
	beq	a0,a5,.L6
	lwu	a4,0(a0)
.L6:
#APP
# 9 "foo.c" 1
	frflags a5
# 0 "" 2
#NO_APP
	sext.w	a5,a5
	or	a5,a5,a4
#APP
# 28 "foo.c" 1
	fssr a5
# 0 "" 2
#NO_APP
	ret
	.size	libc_feupdateenv_riscv_bh, .-libc_feupdateenv_riscv_bh
	.ident	"GCC: (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0"
	.section	.note.GNU-stack,"",@progbits

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
  2023-11-06  0:21   ` Bruno Haible
@ 2023-11-06  0:24   ` Bruno Haible
  2023-11-06 11:32     ` Adhemerval Zanella Netto
  2024-01-09 13:44   ` Szabolcs Nagy
  2 siblings, 1 reply; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:24 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella

> +feupdateenv_single_test (const char *test_name, int fe_exc,
> +			 fexcept_t exception)

The 3rd argument type should be 'int', not 'fexcept_t'.

The type fexcept_t is only for use with the functions fegetexceptflag,
fesetexceptflag, fetestexceptflag. It would be perfectly reasonable for
a platform to define fexcept_t to 'unsigned char', regardless of the
values of the FE_* constants.

> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
> +                  int fe_exc_clear, fexcept_t exception)

Likewise.

Bruno




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/7] Multiple floating-point environment fixes
  2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
                   ` (6 preceding siblings ...)
  2023-11-03 12:24 ` [PATCH 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983) Adhemerval Zanella
@ 2023-11-06  0:25 ` Bruno Haible
  7 siblings, 0 replies; 22+ messages in thread
From: Bruno Haible @ 2023-11-06  0:25 UTC (permalink / raw)
  To: libc-alpha, Adhemerval Zanella

Patches 1/7, 6/7, 7/7 look OK to me. Specific remarks on the other ones.

Bruno




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989)
  2023-11-06  0:15   ` Bruno Haible
@ 2023-11-06 11:24     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-11-06 11:24 UTC (permalink / raw)
  To: Bruno Haible, libc-alpha



On 05/11/23 21:15, Bruno Haible wrote:
> In sysdeps/i386/fpu/fesetexcept.c, I suggest a comment change:
> 
>   And now similarly for SSE.
> ->
>   Get the control word of the SSE unit.
> 
> 
> Bruno
> 

Ack.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
  2023-11-06  0:17   ` Bruno Haible
@ 2023-11-06 11:24     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-11-06 11:24 UTC (permalink / raw)
  To: Bruno Haible, libc-alpha



On 05/11/23 21:17, Bruno Haible wrote:
> In math/test-fexcept-traps.c, around line 70, I would add the same comment as
> in patch 2/7:
> 
>    Also check if the function does not alter the exception mask.  */
> 
> 
> Bruno
> 

Ack.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019)
  2023-11-06  0:17     ` Bruno Haible
@ 2023-11-06 11:25       ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-11-06 11:25 UTC (permalink / raw)
  To: Bruno Haible, Joseph Myers; +Cc: libc-alpha



On 05/11/23 21:17, Bruno Haible wrote:
> Joseph Myers wrote:
>> I think we should say "unspecified", not "undefined" (in the manual and 
>> commit message).
> 
> I agree. Thanks Joseph for the more precise wording.
> 
> Bruno

Ack.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-06  0:21   ` Bruno Haible
@ 2023-11-06 11:28     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-11-06 11:28 UTC (permalink / raw)
  To: Bruno Haible, libc-alpha



On 05/11/23 21:21, Bruno Haible wrote:
> This code gave me puzzles:
> 
>   long int env = (long int) envp - (long int) FE_DFL_ENV;
>   if (env != 0)
>     env = *envp;
> 
> * What is the intent of the code?
> * Can signed integer overflow happen in the first line? (It cannot, due to
>   the value of FE_DFL_ENV being -1 and envp being otherwise aligned mod 4.
>   But one should not have to think that far.)
> * It assumes that intptr_t is the same as 'long int'. Which is not
>   universally true.
> 
> I would suggest to replace the function with this code:
> 
> libc_feupdateenv_riscv (const fenv_t *envp)
> {
>   long int env = (envp != FE_DFL_ENV ? *envp : 0);
> 
>   _FPU_SETCW (env | riscv_getflags ());
> }
> 
> * It is clearer.
> * It does not make assumptions regarding FE_DFL_ENV or alignment.
> * It produces the exact same code; see attached foo.c and foo.s,
>   generated by "riscv64-linux-gnu-gcc -O2 -S foo.c".

Indeed I was not sure why libc_fesetenv_riscv used this specific idiom
to check whether the input is FE_DFL_ENV.  I will change to this, it
is clear.

> 
> Probably the idiom that you copied was meant to allow the use of a
> conditional load instruction. But I would avoid such micro-optimizations
> and instead rely on the compiler to choose the best instructions for a
> task.
> 
> Bruno

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-06  0:24   ` Bruno Haible
@ 2023-11-06 11:32     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-11-06 11:32 UTC (permalink / raw)
  To: Bruno Haible, libc-alpha



On 05/11/23 21:24, Bruno Haible wrote:
>> +feupdateenv_single_test (const char *test_name, int fe_exc,
>> +			 fexcept_t exception)
> 
> The 3rd argument type should be 'int', not 'fexcept_t'.
> 
> The type fexcept_t is only for use with the functions fegetexceptflag,
> fesetexceptflag, fetestexceptflag. It would be perfectly reasonable for
> a platform to define fexcept_t to 'unsigned char', regardless of the
> values of the FE_* constants.

Ok.

> 
>> +update_single_exc (const char *test_name, const fenv_t *envp, int fe_exc,
>> +                  int fe_exc_clear, fexcept_t exception)
> 
> Likewise.

Ok.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
  2023-11-06  0:21   ` Bruno Haible
  2023-11-06  0:24   ` Bruno Haible
@ 2024-01-09 13:44   ` Szabolcs Nagy
  2024-01-09 18:16     ` Adhemerval Zanella Netto
  2 siblings, 1 reply; 22+ messages in thread
From: Szabolcs Nagy @ 2024-01-09 13:44 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha, Bruno Haible

The 11/03/2023 09:24, 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.
> 
> Checked on riscv under qemu-system.

the test fails on aarch64 when trapping is supported.

> +++ 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, fexcept_t 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);

this relies on disabled trapping.

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

trapping is enabled here on targets where trapping is optional.

> +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);
> +}

trapping is still on after this function.

> +static void
> +feupdateenv_single_test (const char *test_name, int fe_exc,
> +			 fexcept_t 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);

this relies on disabled trapping.

> +static void
> +feupdateenv_tests (void)
> +{
...
> +#ifdef FE_OVERFLOW
> +  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
> +#endif
> +
> +#ifdef FE_DIVBYZERO
> +  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
> +#endif

this test fails because previous enables trapping mode.

fwiw a feupdateenv (FE_DFL_ENV); in between the tests
makes math/test-fenv pass.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022)
  2024-01-09 13:44   ` Szabolcs Nagy
@ 2024-01-09 18:16     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2024-01-09 18:16 UTC (permalink / raw)
  To: Szabolcs Nagy, libc-alpha, Bruno Haible



On 09/01/24 10:44, Szabolcs Nagy wrote:
> The 11/03/2023 09:24, 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.
>>
>> Checked on riscv under qemu-system.
> 
> the test fails on aarch64 when trapping is supported.
> 
>> +++ 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, fexcept_t 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);
> 
> this relies on disabled trapping.
> 
>> +  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);
>> +}
> ...
>>  /* 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)
> 
> trapping is enabled here on targets where trapping is optional.
> 
>> +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);
>> +}
> 
> trapping is still on after this function.
> 
>> +static void
>> +feupdateenv_single_test (const char *test_name, int fe_exc,
>> +			 fexcept_t 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);
> 
> this relies on disabled trapping.
> 
>> +static void
>> +feupdateenv_tests (void)
>> +{
> ...
>> +#ifdef FE_OVERFLOW
>> +  feupdate_single_test ("FE_OVERFLOW", FE_OVERFLOW);
>> +#endif
>> +
>> +#ifdef FE_DIVBYZERO
>> +  feupdateenv_single_test ("DIVBYZERO", DIVBYZERO_EXC, FE_DIVBYZERO);
>> +#endif
> 
> this test fails because previous enables trapping mode.
> 
> fwiw a feupdateenv (FE_DFL_ENV); in between the tests
> makes math/test-fenv pass.

I will take a look, thanks for the heads up.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2024-01-09 18:16 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-03 12:24 [PATCH 0/7] Multiple floating-point environment fixes Adhemerval Zanella
2023-11-03 12:24 ` [PATCH 1/7] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
2023-11-03 12:24 ` [PATCH 2/7] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
2023-11-06  0:15   ` Bruno Haible
2023-11-06 11:24     ` Adhemerval Zanella Netto
2023-11-03 12:24 ` [PATCH 3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
2023-11-06  0:17   ` Bruno Haible
2023-11-06 11:24     ` Adhemerval Zanella Netto
2023-11-03 12:24 ` [PATCH 4/7] manual: Clarify undefined behavior of feenableexcept (BZ 31019) Adhemerval Zanella
2023-11-03 16:20   ` Joseph Myers
2023-11-06  0:17     ` Bruno Haible
2023-11-06 11:25       ` Adhemerval Zanella Netto
2023-11-03 12:24 ` [PATCH 5/7] riscv: Fix feenvupdate with FE_DFL_ENV (BZ 31022) Adhemerval Zanella
2023-11-06  0:21   ` Bruno Haible
2023-11-06 11:28     ` Adhemerval Zanella Netto
2023-11-06  0:24   ` Bruno Haible
2023-11-06 11:32     ` Adhemerval Zanella Netto
2024-01-09 13:44   ` Szabolcs Nagy
2024-01-09 18:16     ` Adhemerval Zanella Netto
2023-11-03 12:24 ` [PATCH 6/7] alpha: Fix fesetexceptflag (BZ 30998) Adhemerval Zanella
2023-11-03 12:24 ` [PATCH 7/7] hppa: Fix undefined behaviour in feclearexcept (BZ 30983) Adhemerval Zanella
2023-11-06  0:25 ` [PATCH 0/7] Multiple floating-point environment fixes Bruno Haible

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).