| Message ID | 20231106132713.953501-4-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Multiple floating-point environment fixes | expand |
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. */
diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c index 9b8f583ae6..6bfb5124da 100644 --- a/math/test-fexcept-traps.c +++ b/math/test-fexcept-traps.c @@ -19,6 +19,7 @@ #include <fenv.h> #include <stdio.h> #include <math-tests.h> +#include <math-barriers.h> static int do_test (void) @@ -65,8 +66,28 @@ do_test (void) /* The test is that this does not cause exception traps. For architectures where setting the exception might result in traps the function should - return a nonzero value. */ - ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); + return a nonzero value. + Also check if the function does not alter the exception mask. */ + { + int exc_before = fegetexcept (); + ret = fesetexceptflag (&saved, FE_ALL_EXCEPT); + int exc_after = fegetexcept (); + if (exc_before != exc_after) + { + puts ("fesetexceptflag (FE_ALL_EXCEPT) changed the exceptions mask"); + return 1; + } + } + + /* Execute some floating-point operations, since on some CPUs exceptions + triggers a trap only at the next floating-point instruction. */ + volatile double a = 1.0; + volatile double b = a + a; + math_force_eval (b); + volatile long double al = 1.0L; + volatile long double bl = al + al; + math_force_eval (bl); + if (ret != 0 && !EXCEPTION_SET_FORCES_TRAP) { puts ("fesetexceptflag failed"); diff --git a/sysdeps/i386/fpu/fsetexcptflg.c b/sysdeps/i386/fpu/fsetexcptflg.c index e724b7d6fd..480165cff9 100644 --- a/sysdeps/i386/fpu/fsetexcptflg.c +++ b/sysdeps/i386/fpu/fsetexcptflg.c @@ -17,42 +17,63 @@ <https://www.gnu.org/licenses/>. */ #include <fenv.h> -#include <math.h> -#include <unistd.h> #include <ldsodefs.h> -#include <dl-procinfo.h> int __fesetexceptflag (const fexcept_t *flagp, int excepts) { - fenv_t temp; + /* The flags can be set in the 387 unit or in the SSE unit. When we need to + clear a flag, we need to do so in both units, due to the way fetestexcept + is implemented. + When we need to set a flag, it is sufficient to do it in the SSE unit, + because that is guaranteed to not trap. However, on i386 CPUs that have + only a 387 unit, set the flags in the 387, as long as this cannot trap. */ - /* Get the current environment. We have to do this since we cannot - separately set the status word. */ - __asm__ ("fnstenv %0" : "=m" (*&temp)); + fenv_t temp; - temp.__status_word &= ~(excepts & FE_ALL_EXCEPT); - temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT; + excepts &= FE_ALL_EXCEPT; - /* Store the new status word (along with the rest of the environment. - Possibly new exceptions are set but they won't get executed unless - the next floating-point instruction. */ - __asm__ ("fldenv %0" : : "m" (*&temp)); + /* Get the current x87 FPU environment. We have to do this since we + cannot separately set the status word. + Note: fnstenv masks all floating-point exceptions until the fldenv + or fldcw below. */ + __asm__ ("fnstenv %0" : "=m" (*&temp)); - /* If the CPU supports SSE, we set the MXCSR as well. */ if (CPU_FEATURE_USABLE (SSE)) { - unsigned int xnew_exc; + unsigned int mxcsr; + + /* Clear relevant flags. */ + temp.__status_word &= ~(excepts & ~ *flagp); - /* Get the current MXCSR. */ - __asm__ ("stmxcsr %0" : "=m" (*&xnew_exc)); + /* Store the new status word (along with the rest of the environment). */ + __asm__ ("fldenv %0" : : "m" (*&temp)); - /* Set the relevant bits. */ - xnew_exc &= ~(excepts & FE_ALL_EXCEPT); - xnew_exc |= *flagp & excepts & FE_ALL_EXCEPT; + /* And now similarly for SSE. */ + __asm__ ("stmxcsr %0" : "=m" (*&mxcsr)); + + /* Clear or set relevant flags. */ + mxcsr ^= (mxcsr ^ *flagp) & excepts; /* Put the new data in effect. */ - __asm__ ("ldmxcsr %0" : : "m" (*&xnew_exc)); + __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr)); + } + else + { + /* Clear or set relevant flags. */ + temp.__status_word ^= (temp.__status_word ^ *flagp) & excepts; + + if ((~temp.__control_word) & temp.__status_word & excepts) + { + /* Setting the exception flags may trigger a trap (at the next + floating-point instruction, but that does not matter). + ISO C 23 § 7.6.4.5 does not allow it. */ + __asm__ volatile ("fldcw %0" : : "m" (*&temp.__control_word)); + return -1; + } + + /* Store the new status word (along with the rest of the environment). */ + __asm__ ("fldenv %0" : : "m" (*&temp)); } /* Success. */ diff --git a/sysdeps/x86_64/fpu/fsetexcptflg.c b/sysdeps/x86_64/fpu/fsetexcptflg.c index a3ac1dea01..2ce2b509f2 100644 --- a/sysdeps/x86_64/fpu/fsetexcptflg.c +++ b/sysdeps/x86_64/fpu/fsetexcptflg.c @@ -22,30 +22,34 @@ int fesetexceptflag (const fexcept_t *flagp, int excepts) { + /* The flags can be set in the 387 unit or in the SSE unit. + When we need to clear a flag, we need to do so in both units, + due to the way fetestexcept() is implemented. + When we need to set a flag, it is sufficient to do it in the SSE unit, + because that is guaranteed to not trap. */ + fenv_t temp; unsigned int mxcsr; - /* XXX: Do we really need to set both the exception in both units? - Shouldn't it be enough to set only the SSE unit? */ + excepts &= FE_ALL_EXCEPT; /* Get the current x87 FPU environment. We have to do this since we cannot separately set the status word. */ __asm__ ("fnstenv %0" : "=m" (*&temp)); - temp.__status_word &= ~(excepts & FE_ALL_EXCEPT); - temp.__status_word |= *flagp & excepts & FE_ALL_EXCEPT; + /* Clear relevant flags. */ + temp.__status_word &= ~(excepts & ~ *flagp); - /* Store the new status word (along with the rest of the environment. - Possibly new exceptions are set but they won't get executed unless - the next floating-point instruction. */ + /* Store the new status word (along with the rest of the environment). */ __asm__ ("fldenv %0" : : "m" (*&temp)); - /* And now the same for SSE. */ + /* And now similarly for SSE. */ __asm__ ("stmxcsr %0" : "=m" (*&mxcsr)); - mxcsr &= ~(excepts & FE_ALL_EXCEPT); - mxcsr |= *flagp & excepts & FE_ALL_EXCEPT; + /* Clear or set relevant flags. */ + mxcsr ^= (mxcsr ^ *flagp) & excepts; + /* Put the new data in effect. */ __asm__ ("ldmxcsr %0" : : "m" (*&mxcsr)); /* Success. */
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(-)