[v2,3/7] x86: Do not raises floating-point exception traps on fesetexceptflag (BZ 30990)
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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. */
@@ -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");
@@ -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. */
@@ -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. */