* [PATCH v2 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86
@ 2023-10-24 11:37 Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2023-10-24 11:37 UTC (permalink / raw)
To: libc-alpha; +Cc: Bruno Haible
Bruno has found fesetexcept/fesetexceptflag wrongly raise floating-point
exception flags on x86 and powerpc.
Adhemerval Zanella (2):
powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag
(BZ 30988)
i686: Do not raise exception traps on fesetexcept (BZ 30989)
Bruno Haible (1):
x86: Do not raises floating-point exception traps on fesetexceptflag
(BZ 30990)
math/test-fesetexcept-traps.c | 22 ++++++---
math/test-fexcept-traps.c | 22 ++++++---
sysdeps/i386/fpu/fesetexcept.c | 41 +++++++++++++++--
sysdeps/i386/fpu/fsetexcptflg.c | 58 +++++++++++++++---------
sysdeps/i386/fpu/math-tests-trap-force.h | 29 ++++++++++++
sysdeps/powerpc/fpu/fesetexcept.c | 5 ++
sysdeps/powerpc/fpu/fsetexcptflg.c | 9 +++-
sysdeps/x86_64/fpu/fsetexcptflg.c | 24 ++++++----
8 files changed, 160 insertions(+), 50 deletions(-)
create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988)
2023-10-24 11:37 [PATCH v2 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86 Adhemerval Zanella
@ 2023-10-24 11:37 ` Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
2 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2023-10-24 11:37 UTC (permalink / raw)
To: libc-alpha; +Cc: 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] 9+ messages in thread
* [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)
2023-10-24 11:37 [PATCH v2 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86 Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
@ 2023-10-24 11:37 ` Adhemerval Zanella
2023-10-24 13:43 ` Bruno Haible
2023-10-30 15:21 ` Bruno Haible
2023-10-24 11:37 ` [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
2 siblings, 2 replies; 9+ messages in thread
From: Adhemerval Zanella @ 2023-10-24 11:37 UTC (permalink / raw)
To: libc-alpha; +Cc: 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 | 11 +++++++
sysdeps/i386/fpu/fesetexcept.c | 41 +++++++++++++++++++++---
sysdeps/i386/fpu/math-tests-trap-force.h | 29 +++++++++++++++++
3 files changed, 77 insertions(+), 4 deletions(-)
create mode 100644 sysdeps/i386/fpu/math-tests-trap-force.h
diff --git a/math/test-fesetexcept-traps.c b/math/test-fesetexcept-traps.c
index 96f6c4752f..2f63e9ba23 100644
--- a/math/test-fesetexcept-traps.c
+++ b/math/test-fesetexcept-traps.c
@@ -19,6 +19,7 @@
#include <fenv.h>
#include <stdio.h>
#include <math-tests.h>
+#include <math-barriers.h>
static int
do_test (void)
@@ -43,6 +44,16 @@ do_test (void)
where setting the exception might result in traps the function should
return a nonzero value. */
ret = fesetexcept (FE_ALL_EXCEPT);
+
+ /* Execute some floating-point operations, since on some CPUs exceptions
+ triggers a trap only at the next floating-point instruction. */
+ volatile double a = 1.0;
+ volatile double b = a + a;
+ math_force_eval (b);
+ volatile long double al = 1.0L;
+ volatile long double bl = al + al;
+ math_force_eval (bl);
+
if (ret == 0)
puts ("fesetexcept (FE_ALL_EXCEPT) succeeded");
else if (!EXCEPTION_SET_FORCES_TRAP)
diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
index 18949e982a..6eeb5ab5b0 100644
--- a/sysdeps/i386/fpu/fesetexcept.c
+++ b/sysdeps/i386/fpu/fesetexcept.c
@@ -17,15 +17,48 @@
<https://www.gnu.org/licenses/>. */
#include <fenv.h>
+#include <ldsodefs.h>
int
fesetexcept (int excepts)
{
- fenv_t temp;
+ /* The flags can be set in the 387 unit or in the SSE unit. To set a flag,
+ it is sufficient to do it in the SSE unit, because that is guaranteed to
+ not trap. However, on i386 CPUs that have only a 387 unit, set the flags
+ in the 387, as long as this cannot trap. */
- __asm__ ("fnstenv %0" : "=m" (*&temp));
- temp.__status_word |= excepts & FE_ALL_EXCEPT;
- __asm__ ("fldenv %0" : : "m" (*&temp));
+ excepts &= FE_ALL_EXCEPT;
+
+ if (CPU_FEATURE_USABLE (SSE))
+ {
+ /* And now similarly for SSE. */
+ unsigned int mxcsr;
+ __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
+
+ /* Set relevant flags. */
+ mxcsr |= excepts;
+
+ /* Put the new data in effect. */
+ __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
+ }
+ else
+ {
+ fenv_t temp;
+
+ __asm__ ("fnstenv %0" : "=m" (*&temp));
+
+ /* Clear or set relevant flags. */
+ temp.__status_word |= temp.__status_word & excepts;
+
+ if ((~temp.__control_word) & excepts)
+ /* Setting the exception flags may trigger a trap (at the next
+ floating-point instruction, but that does not matter).
+ ISO C23 (7.6.4.4) does not allow it. */
+ return -1;
+
+ /* Store the new status word (along with the rest of the environment). */
+ __asm__ ("fldenv %0" : : "m" (*&temp));
+ }
return 0;
}
diff --git a/sysdeps/i386/fpu/math-tests-trap-force.h b/sysdeps/i386/fpu/math-tests-trap-force.h
new file mode 100644
index 0000000000..20f4ead98d
--- /dev/null
+++ b/sysdeps/i386/fpu/math-tests-trap-force.h
@@ -0,0 +1,29 @@
+/* Configuration for math tests: support for setting exception flags
+ without causing enabled traps. i686 version.
+ Copyright (C) 2023 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef I386_FPU_MATH_TESTS_TRAP_FORCE_H
+#define I386_FPU_MATH_TESTS_TRAP_FORCE_H 1
+
+#include <cpu-features.h>
+
+/* Setting exception flags in FPU Status Register results in enabled traps for
+ those exceptions being taken. */
+#define EXCEPTION_SET_FORCES_TRAP (CPU_FEATURE_USABLE (SSE))
+
+#endif /* math-tests-trap-force.h. */
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
2023-10-24 11:37 [PATCH v2 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86 Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
@ 2023-10-24 11:37 ` Adhemerval Zanella
2023-10-30 15:22 ` Bruno Haible
2 siblings, 1 reply; 9+ messages in thread
From: Adhemerval Zanella @ 2023-10-24 11:37 UTC (permalink / raw)
To: libc-alpha; +Cc: 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 | 11 ++++++
sysdeps/i386/fpu/fsetexcptflg.c | 58 ++++++++++++++++++++-----------
sysdeps/x86_64/fpu/fsetexcptflg.c | 24 +++++++------
3 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
index 9b8f583ae6..f10bf2d9a3 100644
--- a/math/test-fexcept-traps.c
+++ b/math/test-fexcept-traps.c
@@ -19,6 +19,7 @@
#include <fenv.h>
#include <stdio.h>
#include <math-tests.h>
+#include <math-barriers.h>
static int
do_test (void)
@@ -67,6 +68,16 @@ do_test (void)
where setting the exception might result in traps the function should
return a nonzero value. */
ret = fesetexceptflag (&saved, FE_ALL_EXCEPT);
+
+ /* Execute some floating-point operations, since on some CPUs exceptions
+ triggers a trap only at the next floating-point instruction. */
+ volatile double a = 1.0;
+ volatile double b = a + a;
+ math_force_eval (b);
+ volatile long double al = 1.0L;
+ volatile long double bl = al + al;
+ math_force_eval (bl);
+
if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP)
{
puts ("fesetexceptflag failed");
diff --git a/sysdeps/i386/fpu/fsetexcptflg.c b/sysdeps/i386/fpu/fsetexcptflg.c
index e724b7d6fd..ccbcf35e8e 100644
--- a/sysdeps/i386/fpu/fsetexcptflg.c
+++ b/sysdeps/i386/fpu/fsetexcptflg.c
@@ -17,42 +17,58 @@
<https://www.gnu.org/licenses/>. */
#include <fenv.h>
-#include <math.h>
-#include <unistd.h>
#include <ldsodefs.h>
-#include <dl-procinfo.h>
int
__fesetexceptflag (const fexcept_t *flagp, int excepts)
{
- fenv_t temp;
+ /* The flags can be set in the 387 unit or in the SSE unit. When we need to
+ clear a flag, we need to do so in both units, due to the way fetestexcept
+ is implemented.
+ When we need to set a flag, it is sufficient to do it in the SSE unit,
+ because that is guaranteed to not trap. However, on i386 CPUs that have
+ only a 387 unit, set the flags in the 387, as long as this cannot trap. */
- /* Get the current environment. We have to do this since we cannot
- separately set the status word. */
- __asm__ ("fnstenv %0" : "=m" (*&temp));
+ fenv_t temp;
- temp.__status_word &= ~(excepts & FE_ALL_EXCEPT);
- temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT;
+ excepts &= FE_ALL_EXCEPT;
- /* Store the new status word (along with the rest of the environment.
- Possibly new exceptions are set but they won't get executed unless
- the next floating-point instruction. */
- __asm__ ("fldenv %0" : : "m" (*&temp));
+ /* Get the current x87 FPU environment. We have to do this since we
+ cannot separately set the status word. */
+ __asm__ ("fnstenv %0" : "=m" (*&temp));
- /* If the CPU supports SSE, we set the MXCSR as well. */
if (CPU_FEATURE_USABLE (SSE))
{
- unsigned int xnew_exc;
+ unsigned int mxcsr;
+
+ /* Clear relevant flags. */
+ temp.__status_word &= ~(excepts & ~ *flagp);
- /* Get the current MXCSR. */
- __asm__ ("stmxcsr %0" : "=m" (*&xnew_exc));
+ /* Store the new status word (along with the rest of the environment). */
+ __asm__ ("fldenv %0" : : "m" (*&temp));
- /* Set the relevant bits. */
- xnew_exc &= ~(excepts & FE_ALL_EXCEPT);
- xnew_exc |= *flagp & excepts & FE_ALL_EXCEPT;
+ /* And now similarly for SSE. */
+ __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
+
+ /* Clear or set relevant flags. */
+ mxcsr ^= (mxcsr ^ *flagp) & excepts;
/* Put the new data in effect. */
- __asm__ ("ldmxcsr %0" : : "m" (*&xnew_exc));
+ __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
+ }
+ else
+ {
+ /* Clear or set relevant flags. */
+ temp.__status_word ^= (temp.__status_word ^ *flagp) & excepts;
+
+ if ((~temp.__control_word) & temp.__status_word & excepts)
+ /* Setting the exception flags may trigger a trap (at the next
+ floating-point instruction, but that does not matter).
+ ISO C 23 § 7.6.4.5 does not allow it. */
+ return -1;
+
+ /* Store the new status word (along with the rest of the environment). */
+ __asm__ ("fldenv %0" : : "m" (*&temp));
}
/* Success. */
diff --git a/sysdeps/x86_64/fpu/fsetexcptflg.c b/sysdeps/x86_64/fpu/fsetexcptflg.c
index a3ac1dea01..2ce2b509f2 100644
--- a/sysdeps/x86_64/fpu/fsetexcptflg.c
+++ b/sysdeps/x86_64/fpu/fsetexcptflg.c
@@ -22,30 +22,34 @@
int
fesetexceptflag (const fexcept_t *flagp, int excepts)
{
+ /* The flags can be set in the 387 unit or in the SSE unit.
+ When we need to clear a flag, we need to do so in both units,
+ due to the way fetestexcept() is implemented.
+ When we need to set a flag, it is sufficient to do it in the SSE unit,
+ because that is guaranteed to not trap. */
+
fenv_t temp;
unsigned int mxcsr;
- /* XXX: Do we really need to set both the exception in both units?
- Shouldn't it be enough to set only the SSE unit? */
+ excepts &= FE_ALL_EXCEPT;
/* Get the current x87 FPU environment. We have to do this since we
cannot separately set the status word. */
__asm__ ("fnstenv %0" : "=m" (*&temp));
- temp.__status_word &= ~(excepts & FE_ALL_EXCEPT);
- temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT;
+ /* Clear relevant flags. */
+ temp.__status_word &= ~(excepts & ~ *flagp);
- /* Store the new status word (along with the rest of the environment.
- Possibly new exceptions are set but they won't get executed unless
- the next floating-point instruction. */
+ /* Store the new status word (along with the rest of the environment). */
__asm__ ("fldenv %0" : : "m" (*&temp));
- /* And now the same for SSE. */
+ /* And now similarly for SSE. */
__asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
- mxcsr &= ~(excepts & FE_ALL_EXCEPT);
- mxcsr |= *flagp & excepts & FE_ALL_EXCEPT;
+ /* Clear or set relevant flags. */
+ mxcsr ^= (mxcsr ^ *flagp) & excepts;
+ /* Put the new data in effect. */
__asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
/* Success. */
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)
2023-10-24 11:37 ` [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
@ 2023-10-24 13:43 ` Bruno Haible
2023-10-26 18:39 ` Adhemerval Zanella Netto
2023-10-30 15:21 ` Bruno Haible
1 sibling, 1 reply; 9+ messages in thread
From: Bruno Haible @ 2023-10-24 13:43 UTC (permalink / raw)
To: libc-alpha, Adhemerval Zanella
Looks all good, except for two comments:
Adhemerval Zanella wrote:
> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
> index 18949e982a..6eeb5ab5b0 100644
> --- a/sysdeps/i386/fpu/fesetexcept.c
> +++ b/sysdeps/i386/fpu/fesetexcept.c
> @@ -17,15 +17,48 @@
> <https://www.gnu.org/licenses/>. */
>
> #include <fenv.h>
> +#include <ldsodefs.h>
>
> int
> fesetexcept (int excepts)
> {
> - fenv_t temp;
> + /* The flags can be set in the 387 unit or in the SSE unit. To set a flag,
> + it is sufficient to do it in the SSE unit, because that is guaranteed to
> + not trap. However, on i386 CPUs that have only a 387 unit, set the flags
> + in the 387, as long as this cannot trap. */
>
> - __asm__ ("fnstenv %0" : "=m" (*&temp));
> - temp.__status_word |= excepts & FE_ALL_EXCEPT;
> - __asm__ ("fldenv %0" : : "m" (*&temp));
> + excepts &= FE_ALL_EXCEPT;
> +
> + if (CPU_FEATURE_USABLE (SSE))
> + {
> + /* And now similarly for SSE. */
> + unsigned int mxcsr;
> + __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
The comment "And now similarly for SSE." is odd, since there was no similar code
before it. How about
/* Set the flags in the SSE unit. */
> + /* Set relevant flags. */
> + mxcsr |= excepts;
> +
> + /* Put the new data in effect. */
> + __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
> + }
> + else
> + {
> + fenv_t temp;
> +
> + __asm__ ("fnstenv %0" : "=m" (*&temp));
> +
> + /* Clear or set relevant flags. */
> + temp.__status_word |= temp.__status_word & excepts;
The comment here should be
/* Set relevant flags. */
since here, unlike in fesetexceptflag(), we don't need to clear any flags.
Bruno
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)
2023-10-24 13:43 ` Bruno Haible
@ 2023-10-26 18:39 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2023-10-26 18:39 UTC (permalink / raw)
To: Bruno Haible, libc-alpha
On 24/10/23 10:43, Bruno Haible wrote:
> Looks all good, except for two comments:
>
> Adhemerval Zanella wrote:
>> diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
>> index 18949e982a..6eeb5ab5b0 100644
>> --- a/sysdeps/i386/fpu/fesetexcept.c
>> +++ b/sysdeps/i386/fpu/fesetexcept.c
>> @@ -17,15 +17,48 @@
>> <https://www.gnu.org/licenses/>. */
>>
>> #include <fenv.h>
>> +#include <ldsodefs.h>
>>
>> int
>> fesetexcept (int excepts)
>> {
>> - fenv_t temp;
>> + /* The flags can be set in the 387 unit or in the SSE unit. To set a flag,
>> + it is sufficient to do it in the SSE unit, because that is guaranteed to
>> + not trap. However, on i386 CPUs that have only a 387 unit, set the flags
>> + in the 387, as long as this cannot trap. */
>>
>> - __asm__ ("fnstenv %0" : "=m" (*&temp));
>> - temp.__status_word |= excepts & FE_ALL_EXCEPT;
>> - __asm__ ("fldenv %0" : : "m" (*&temp));
>> + excepts &= FE_ALL_EXCEPT;
>> +
>> + if (CPU_FEATURE_USABLE (SSE))
>> + {
>> + /* And now similarly for SSE. */
>> + unsigned int mxcsr;
>> + __asm__ ("stmxcsr %0" : "=m" (*&mxcsr));
>
> The comment "And now similarly for SSE." is odd, since there was no similar code
> before it. How about
> /* Set the flags in the SSE unit. */
Ack.
>
>> + /* Set relevant flags. */
>> + mxcsr |= excepts;
>> +
>> + /* Put the new data in effect. */
>> + __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr));
>> + }
>> + else
>> + {
>> + fenv_t temp;
>> +
>> + __asm__ ("fnstenv %0" : "=m" (*&temp));
>> +
>> + /* Clear or set relevant flags. */
>> + temp.__status_word |= temp.__status_word & excepts;
>
> The comment here should be
> /* Set relevant flags. */
> since here, unlike in fesetexceptflag(), we don't need to clear any flags.
Ack.
I changed both locally.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)
2023-10-24 11:37 ` [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
2023-10-24 13:43 ` Bruno Haible
@ 2023-10-30 15:21 ` Bruno Haible
2023-10-30 16:05 ` Adhemerval Zanella Netto
1 sibling, 1 reply; 9+ messages in thread
From: Bruno Haible @ 2023-10-30 15:21 UTC (permalink / raw)
To: libc-alpha, Adhemerval Zanella
[-- Attachment #1: Type: text/plain, Size: 717 bytes --]
Hi Adhemerval,
In the error case, the new code mistakenly masks all floating-point
exceptions (i.e. as if someone had called fedisableexcept (FE_ALL_EXCEPT)).
This is because the FNSTENV instruction is documented as
"Saves the current FPU operating environment at the memory
location specified with the destination operand, and then
masks all floating-point exceptions."
The mistake came from my initial proposed fix of BZ 30990. Sorry about that.
Here's a proposed fix, on top of your patch. I've verified that the sequence
of instructions
__asm__ ("fnstenv %0" : "=m" (*&temp));
__asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
does restore the exceptions trapping bits.
Bruno
[-- Attachment #2: BZ30989-fnstenv-fix.diff --]
[-- Type: text/x-patch, Size: 1249 bytes --]
diff --git a/sysdeps/i386/fpu/fesetexcept.c b/sysdeps/i386/fpu/fesetexcept.c
index 6eeb5ab5b0..8c93264b0c 100644
--- a/sysdeps/i386/fpu/fesetexcept.c
+++ b/sysdeps/i386/fpu/fesetexcept.c
@@ -46,15 +46,20 @@ fesetexcept (int excepts)
fenv_t temp;
__asm__ ("fnstenv %0" : "=m" (*&temp));
+ /* Note: fnstenv masks all floating-point exceptions until the fldenv
+ or fldcw below. */
/* Clear or set relevant flags. */
temp.__status_word |= temp.__status_word & excepts;
if ((~temp.__control_word) & excepts)
- /* Setting the exception flags may trigger a trap (at the next
- floating-point instruction, but that does not matter).
- ISO C23 (7.6.4.4) does not allow it. */
- return -1;
+ {
+ /* Setting the exception flags may trigger a trap (at the next
+ floating-point instruction, but that does not matter).
+ ISO C23 § 7.6.4.4 does not allow it. */
+ __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
+ return -1;
+ }
/* Store the new status word (along with the rest of the environment). */
__asm__ ("fldenv %0" : : "m" (*&temp));
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
2023-10-24 11:37 ` [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
@ 2023-10-30 15:22 ` Bruno Haible
0 siblings, 0 replies; 9+ messages in thread
From: Bruno Haible @ 2023-10-30 15:22 UTC (permalink / raw)
To: libc-alpha, Adhemerval Zanella
[-- Attachment #1: Type: text/plain, Size: 731 bytes --]
Hi Adhemerval,
Like in the PATCH v2 2/3, in the error case, the new code mistakenly masks
all floating-point exceptions (i.e. as if someone had called
fedisableexcept (FE_ALL_EXCEPT)).
This is because the FNSTENV instruction is documented as
"Saves the current FPU operating environment at the memory
location specified with the destination operand, and then
masks all floating-point exceptions."
The mistake came from my initial proposed fix. Sorry about that.
Here's a proposed fix, on top of your patch. I've verified that the sequence
of instructions
__asm__ ("fnstenv %0" : "=m" (*&temp));
__asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
does restore the exceptions trapping bits.
Bruno
[-- Attachment #2: BZ30990-fnstenv-fix.diff --]
[-- Type: text/x-patch, Size: 1478 bytes --]
diff --git a/sysdeps/i386/fpu/fsetexcptflg.c b/sysdeps/i386/fpu/fsetexcptflg.c
index ccbcf35e8e..8bff9026a1 100644
--- a/sysdeps/i386/fpu/fsetexcptflg.c
+++ b/sysdeps/i386/fpu/fsetexcptflg.c
@@ -36,6 +36,8 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
/* Get the current x87 FPU environment. We have to do this since we
cannot separately set the status word. */
__asm__ ("fnstenv %0" : "=m" (*&temp));
+ /* Note: fnstenv masks all floating-point exceptions until the fldenv
+ or fldcw below. */
if (CPU_FEATURE_USABLE (SSE))
{
@@ -62,10 +64,13 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
temp.__status_word ^= (temp.__status_word ^ *flagp) & excepts;
if ((~temp.__control_word) & temp.__status_word & excepts)
- /* Setting the exception flags may trigger a trap (at the next
- floating-point instruction, but that does not matter).
- ISO C 23 § 7.6.4.5 does not allow it. */
- return -1;
+ {
+ /* Setting the exception flags may trigger a trap (at the next
+ floating-point instruction, but that does not matter).
+ ISO C 23 § 7.6.4.5 does not allow it. */
+ __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
+ return -1;
+ }
/* Store the new status word (along with the rest of the environment). */
__asm__ ("fldenv %0" : : "m" (*&temp));
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989)
2023-10-30 15:21 ` Bruno Haible
@ 2023-10-30 16:05 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2023-10-30 16:05 UTC (permalink / raw)
To: Bruno Haible, libc-alpha
On 30/10/23 12:21, Bruno Haible wrote:
> Hi Adhemerval,
>
> In the error case, the new code mistakenly masks all floating-point
> exceptions (i.e. as if someone had called fedisableexcept (FE_ALL_EXCEPT)).
> This is because the FNSTENV instruction is documented as
> "Saves the current FPU operating environment at the memory
> location specified with the destination operand, and then
> masks all floating-point exceptions."
>
> The mistake came from my initial proposed fix of BZ 30990. Sorry about that.
>
> Here's a proposed fix, on top of your patch. I've verified that the sequence
> of instructions
> __asm__ ("fnstenv %0" : "=m" (*&temp));
> __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word));
> does restore the exceptions trapping bits.
>
> Bruno
Right, it would be good to have a regression check for this. I will update
the patch.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-30 16:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-24 11:37 [PATCH v2 0/3] Fix fesetexcept/fesetexceptflag on powerpc and x86 Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 1/3] powerpc: Do not raise exception traps for fesetexcept/fesetexceptflag (BZ 30988) Adhemerval Zanella
2023-10-24 11:37 ` [PATCH v2 2/3] i686: Do not raise exception traps on fesetexcept (BZ 30989) Adhemerval Zanella
2023-10-24 13:43 ` Bruno Haible
2023-10-26 18:39 ` Adhemerval Zanella Netto
2023-10-30 15:21 ` Bruno Haible
2023-10-30 16:05 ` Adhemerval Zanella Netto
2023-10-24 11:37 ` [PATCH v2 3/3] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990) Adhemerval Zanella
2023-10-30 15:22 ` 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).