[PATCH][PR libgfortran/78314] Fix ieee_support_halting

[PATCH][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

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

When fpu trapping is enabled in libgfortran, the return value of
feenableexcept is not checked.  Glibc reports there if the operation
was unsuccessful which happens if the target has no trapping support.

There seems to be a separate api for checking trapping support:
ieee_support_halting, but it only checked if the exception status
flags are available, so check trapping support too by enabling
and disabling traps.

Updated the test that changed trapping to use ieee_support_halting,
(I think this is better than XFAILing the test case as it tests
for things that work without trapping support just fine.)

Tested on aarch64-linux-gnu and x86_64-linux-gnu.

gcc/testsuite/
2016-11-15  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libgfortran/78314
	* gfortran.dg/ieee/ieee_6.f90: Use ieee_support_halting.

libgfortran/
2016-11-15  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libgfortran/78314
	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.

[-- Attachment #2: fptrap.diff --]
[-- Type: text/x-patch, Size: 2635 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90 b/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
index 8fb4f6f..43aa3bf 100644
--- a/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
+++ b/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
@@ -9,7 +9,7 @@
   implicit none
 
   type(ieee_status_type) :: s1, s2
-  logical :: flags(5), halt(5)
+  logical :: flags(5), halt(5), haltworks
   type(ieee_round_type) :: mode
   real :: x
 
@@ -18,6 +18,7 @@
   call ieee_set_flag(ieee_all, .false.)
   call ieee_set_rounding_mode(ieee_down)
   call ieee_set_halting_mode(ieee_all, .false.)
+  haltworks = ieee_support_halting(ieee_overflow)
 
   call ieee_get_status(s1)
   call ieee_set_status(s1)
@@ -46,7 +47,7 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
   call ieee_set_status(s2)
 
@@ -58,7 +59,7 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
   call ieee_set_status(s1)
 
@@ -79,6 +80,6 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
 end
diff --git a/libgfortran/config/fpu-glibc.h b/libgfortran/config/fpu-glibc.h
index 6e505da..e254fb1 100644
--- a/libgfortran/config/fpu-glibc.h
+++ b/libgfortran/config/fpu-glibc.h
@@ -121,7 +121,43 @@ get_fpu_trap_exceptions (void)
 int
 support_fpu_trap (int flag)
 {
-  return support_fpu_flag (flag);
+  int exceptions = 0;
+  int old, ret;
+
+  if (!support_fpu_flag (flag))
+    return 0;
+
+#ifdef FE_INVALID
+  if (flag & GFC_FPE_INVALID) exceptions |= FE_INVALID;
+#endif
+
+#ifdef FE_DIVBYZERO
+  if (flag & GFC_FPE_ZERO) exceptions |= FE_DIVBYZERO;
+#endif
+
+#ifdef FE_OVERFLOW
+  if (flag & GFC_FPE_OVERFLOW) exceptions |= FE_OVERFLOW;
+#endif
+
+#ifdef FE_UNDERFLOW
+  if (flag & GFC_FPE_UNDERFLOW) exceptions |= FE_UNDERFLOW;
+#endif
+
+#ifdef FE_DENORMAL
+  if (flag & GFC_FPE_DENORMAL) exceptions |= FE_DENORMAL;
+#endif
+
+#ifdef FE_INEXACT
+  if (flag & GFC_FPE_INEXACT) exceptions |= FE_INEXACT;
+#endif
+
+  old = fedisableexcept (exceptions);
+  if (old == -1)
+    return 0;
+
+  ret = feenableexcept (exceptions) != -1;
+  feenableexcept (old);
+  return ret;
 }
 
 

Re: [PATCH][PR libgfortran/78314] Fix ieee_support_halting

[FX]

Hi,

> There seems to be a separate api for checking trapping support:
> ieee_support_halting, but it only checked if the exception status
> flags are available, so check trapping support too by enabling
> and disabling traps.

Thanks for the patch.

I am worried about the unnecessary operations that we’re doing here: doesn’t glibc have a way to tell you what it supports without having to do it (twice, enabling then disabling)?

Also, the glibc doc states that: "Each of the macros FE_DIVBYZERO, FE_INEXACT, FE_INVALID, FE_OVERFLOW, FE_UNDERFLOW is defined when the implementation supports handling of the corresponding exception”. It evens says:

> Each constant is defined if and only if the FPU you are compiling for supports that exception, so you can test for FPU support with ‘#ifdef’.

So it seems rather clear that compile-time tests are the recommended way to go.

FX

Re: [PATCH][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

On 15/11/16 16:22, FX wrote:
>> There seems to be a separate api for checking trapping support:
>> ieee_support_halting, but it only checked if the exception status
>> flags are available, so check trapping support too by enabling
>> and disabling traps.
> 
> Thanks for the patch.
> 
> I am worried about the unnecessary operations that we’re doing here: doesn’t glibc have a way to tell you what it supports without having to do it (twice, enabling then disabling)?
> 
> Also, the glibc doc states that: "Each of the macros FE_DIVBYZERO, FE_INEXACT, FE_INVALID, FE_OVERFLOW, FE_UNDERFLOW is defined when the implementation supports handling of the corresponding exception”. It evens says:
> 
>> Each constant is defined if and only if the FPU you are compiling for supports that exception, so you can test for FPU support with ‘#ifdef’.
> 
> So it seems rather clear that compile-time tests are the recommended way to go.

i think that's a documentation bug then, it
should say that the macros imply the support
of fpu exception status flags, but not trapping.

(otherwise glibc could not provide iso c annex
f conforming fenv on aarch64 and arm, where FE_*
must be defined, but only status flag support
is required.)

disabling/enabling makes this api a lot heavier
than before, but trapping cannot be decided at
compile-time, although the result may be cached,
i think this should not be a frequent operation.

otoh rereading my patch i think i fail to restore
the original exception state correctly.

Re: [PATCH][PR libgfortran/78314] Fix ieee_support_halting

[FX]

> disabling/enabling makes this api a lot heavier
> than before, but trapping cannot be decided at
> compile-time, although the result may be cached,
> i think this should not be a frequent operation.
> 
> otoh rereading my patch i think i fail to restore
> the original exception state correctly.

Well, if we have no choice, then let’s do it. (With an updated patch)

FX

[PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

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

ieee_support_halting only checked the availability of status
flags, not trapping support.  On some targets the later can
only be checked at runtime: feenableexcept reports if
enabling traps failed.

So check trapping support by enabling/disabling it.

Updated the test that enabled trapping to check if it is
supported.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.

gcc/testsuite/
2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libgfortran/78314
	* gfortran.dg/ieee/ieee_6.f90: Use ieee_support_halting.

libgfortran/
2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libgfortran/78314
	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.


[-- Attachment #2: fptrap3.diff --]
[-- Type: text/x-patch, Size: 2596 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90 b/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
index 8fb4f6f..43aa3bf 100644
--- a/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
+++ b/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
@@ -9,7 +9,7 @@
   implicit none
 
   type(ieee_status_type) :: s1, s2
-  logical :: flags(5), halt(5)
+  logical :: flags(5), halt(5), haltworks
   type(ieee_round_type) :: mode
   real :: x
 
@@ -18,6 +18,7 @@
   call ieee_set_flag(ieee_all, .false.)
   call ieee_set_rounding_mode(ieee_down)
   call ieee_set_halting_mode(ieee_all, .false.)
+  haltworks = ieee_support_halting(ieee_overflow)
 
   call ieee_get_status(s1)
   call ieee_set_status(s1)
@@ -46,7 +47,7 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
   call ieee_set_status(s2)
 
@@ -58,7 +59,7 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
   call ieee_set_status(s1)
 
@@ -79,6 +80,6 @@
   call ieee_get_rounding_mode(mode)
   if (mode /= ieee_to_zero) call abort
   call ieee_get_halting_mode(ieee_all, halt)
-  if ((.not. halt(1)) .or. any(halt(2:))) call abort
+  if ((haltworks .and. .not. halt(1)) .or. any(halt(2:))) call abort
 
 end
diff --git a/libgfortran/config/fpu-glibc.h b/libgfortran/config/fpu-glibc.h
index 6e505da..8b29a76 100644
--- a/libgfortran/config/fpu-glibc.h
+++ b/libgfortran/config/fpu-glibc.h
@@ -121,7 +121,41 @@ get_fpu_trap_exceptions (void)
 int
 support_fpu_trap (int flag)
 {
-  return support_fpu_flag (flag);
+  int exceptions = 0;
+  int old;
+
+  if (!support_fpu_flag (flag))
+    return 0;
+
+#ifdef FE_INVALID
+  if (flag & GFC_FPE_INVALID) exceptions |= FE_INVALID;
+#endif
+
+#ifdef FE_DIVBYZERO
+  if (flag & GFC_FPE_ZERO) exceptions |= FE_DIVBYZERO;
+#endif
+
+#ifdef FE_OVERFLOW
+  if (flag & GFC_FPE_OVERFLOW) exceptions |= FE_OVERFLOW;
+#endif
+
+#ifdef FE_UNDERFLOW
+  if (flag & GFC_FPE_UNDERFLOW) exceptions |= FE_UNDERFLOW;
+#endif
+
+#ifdef FE_DENORMAL
+  if (flag & GFC_FPE_DENORMAL) exceptions |= FE_DENORMAL;
+#endif
+
+#ifdef FE_INEXACT
+  if (flag & GFC_FPE_INEXACT) exceptions |= FE_INEXACT;
+#endif
+
+  old = feenableexcept (exceptions);
+  if (old == -1)
+    return 0;
+  fedisableexcept (exceptions & ~old);
+  return 1;
 }
 
 

Re: [PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[FX]

> gcc/testsuite/
> 2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	PR libgfortran/78314
> 	* gfortran.dg/ieee/ieee_6.f90: Use ieee_support_halting.
> 
> libgfortran/
> 2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	PR libgfortran/78314
> 	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.

OK to commit.

FX

Re: [PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

On 16/11/16 16:53, Szabolcs Nagy wrote:
> ieee_support_halting only checked the availability of status
> flags, not trapping support.  On some targets the later can
> only be checked at runtime: feenableexcept reports if
> enabling traps failed.
> 
> So check trapping support by enabling/disabling it.
> 
> Updated the test that enabled trapping to check if it is
> supported.
> 
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.
> 
> gcc/testsuite/
> 2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	PR libgfortran/78314
> 	* gfortran.dg/ieee/ieee_6.f90: Use ieee_support_halting.
> 
> libgfortran/
> 2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	PR libgfortran/78314
> 	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.
> 

it seems this broke ieee_8.f90 which tests compile time
vs runtime value of ieee_support_halting

if fortran needs this, then support_halting should be
always false on arm and aarch64.

but i'm not familiar enough with fortran to tell if
there is some better workaround.

Re: [PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[FX]

Dear Nagy,

> it seems this broke ieee_8.f90 which tests compile time vs runtime value of ieee_support_halting
> if fortran needs this, then support_halting should be always false on arm and aarch64.
> but i'm not familiar enough with fortran to tell if there is some better workaround.

Can you XFAIL the test on your platform, open a PR and assign it to me?
This is a corner of the Fortran standard that is not entirely clear, and there have been some corrigenda on the topic, so I need to review it.

Thanks,
FX

Re: [PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

On 21/11/16 14:16, FX wrote:
>> it seems this broke ieee_8.f90 which tests compile time vs runtime value of ieee_support_halting
>> if fortran needs this, then support_halting should be always false on arm and aarch64.
>> but i'm not familiar enough with fortran to tell if there is some better workaround.
> 
> Can you XFAIL the test on your platform, open a PR and assign it to me?
> This is a corner of the Fortran standard that is not entirely clear, and there have been some corrigenda on the topic, so I need to review it.
> 

i opened PR 78449

will do the xfail later.

Re: [PATCH v2][PR libgfortran/78314] Fix ieee_support_halting

[Szabolcs Nagy]

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

On 21/11/16 14:16, FX wrote:
> Can you XFAIL the test on your platform, open a PR and assign it to me?

OK. Committed.

ARM and AArch64 may not support trapping so runtime and
compile time check can differ.

gcc/testsuite/
2016-11-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libgfortran/78449
	* gfortran.dg/ieee/ieee_8.f90 (aarch64*gnu, arm*gnu*): Mark xfail.


[-- Attachment #2: xfail.diff --]
[-- Type: text/x-patch, Size: 413 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/ieee/ieee_8.f90 b/gcc/testsuite/gfortran.dg/ieee/ieee_8.f90
index 9806bcf..7d0cdfd 100644
--- a/gcc/testsuite/gfortran.dg/ieee/ieee_8.f90
+++ b/gcc/testsuite/gfortran.dg/ieee/ieee_8.f90
@@ -1,4 +1,5 @@
-! { dg-do run }
+! { dg-do run { xfail aarch64*-*-gnu arm*-*-gnueabi arm*-*-gnueabihf } }
+! XFAIL because of PR libfortran/78449.
 
 module foo
   use :: ieee_exceptions