diff mbox series

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

Message ID 20231023213659.3236496-4-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series Fix fesetexcept/fesetexceptflag on powerpc and x86 | expand

Commit Message

Adhemerval Zanella Oct. 23, 2023, 9:36 p.m. UTC 
From: Bruno Haible <bruno@clisp.org>

According to ISO C23 (7.6.4.5), fesetexceptflag is supposed to set
floating-point exception flags without raising a trap.

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

Comments

Bruno Haible Oct. 24, 2023, 12:19 a.m. UTC | #1 
Adhemerval Zanella wrote:
> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
> index 9b8f583ae6..a486d17951 100644
> --- a/math/test-fexcept-traps.c
> +++ b/math/test-fexcept-traps.c
> @@ -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.  */
> +  double a = 1.0;
> +  double b = a + a;
> +  math_force_eval (b);
> +  long double al = 1.0L;
> +  long double bl = al + al;

Like in [2/3], I would mark the variables a, b, al, bl as 'volatile',
otherwise it's too easy for GCC to do constant-folding and thus eliminate
the floating-point operations.

Bruno
Adhemerval Zanella Oct. 24, 2023, 11:13 a.m. UTC | #2 
On 23/10/23 21:19, Bruno Haible wrote:
> Adhemerval Zanella wrote:
>> diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
>> index 9b8f583ae6..a486d17951 100644
>> --- a/math/test-fexcept-traps.c
>> +++ b/math/test-fexcept-traps.c
>> @@ -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.  */
>> +  double a = 1.0;
>> +  double b = a + a;
>> +  math_force_eval (b);
>> +  long double al = 1.0L;
>> +  long double bl = al + al;
> 
> Like in [2/3], I would mark the variables a, b, al, bl as 'volatile',
> otherwise it's too easy for GCC to do constant-folding and thus eliminate
> the floating-point operations.

Ack.
diff mbox series

Patch

diff --git a/math/test-fexcept-traps.c b/math/test-fexcept-traps.c
index 9b8f583ae6..a486d17951 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.  */
+  double a = 1.0;
+  double b = a + a;
+  math_force_eval (b);
+  long double al = 1.0L;
+  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.  */