diff mbox

powerpc: Fix inline feraiseexcept, feclearexcept macros

Message ID 54C6449B.60609@linux.vnet.ibm.com
State New
Headers show

Commit Message

Adhemerval Zanella Jan. 26, 2015, 1:43 p.m. UTC
On 24-01-2015 00:21, Joseph Myers wrote:
> On Fri, 23 Jan 2015, Adhemerval Zanella wrote:
>
>> This patch fixes the inline feraiseexcept and feclearexcept macros for
>> powerpc by casting the input argument to integer before operation on it.
>>
>> It fixes BZ#17776.
> This should add an (architecture-independent) testcase (probably covering 
> such arguments to all <fenv.h> functions that accept an integer argument, 
> not just those with the bug).

Thanks for the review, what about this testcase.  It surely do not cover all
cases, but tests double arguments for current tested functions.

>
>> +    if (__builtin_constant_p (__e)					      \
> Have you made sure __builtin_constant_p works for variables assigned like 
> this from a macro argument?
>
The code:

#define DOUBLE_ARG(__x) ({ double __arg = __x; __arg; })
#define DOUBLE_ARG2     (1.0)

void
f (void)
{
  feclearexcept (1.0);
  feclearexcept (DOUBLE_ARG (1));
  feclearexcept (DOUBLE_ARG2);
}

All generate just 'mtfsb0 31' instructions with GCC 4.8.

--

	[BZ #17776]
	* sysdeps/powerpc/bits/fenvinline.h (feraiseexcept): Convert input to
	integer before bitwise and assembly operations.
	(feclearexcept): Likewise.
	* math/test-fenvinline.c: New file.
	* math/Makefile: Add test-fenvinline test.

--

Comments

Andreas Schwab Jan. 26, 2015, 2:14 p.m. UTC | #1
Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes:

> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
> index 35c2114..999c6b2 100644
> --- a/sysdeps/powerpc/bits/fenvinline.h
> +++ b/sysdeps/powerpc/bits/fenvinline.h
> @@ -34,29 +34,41 @@
>  
>  /* Inline definition for feraiseexcept.  */
>  #  define feraiseexcept(__excepts) \
> -  ((__builtin_constant_p (__excepts)					      \
> -    && ((__excepts) & ((__excepts)-1)) == 0				      \
> -    && (__excepts) != FE_INVALID)					      \
> -   ? ((__excepts) != 0							      \
> -      ? (__extension__ ({ __asm__ __volatile__				      \
> -			  ("mtfsb1 %s0"					      \
> -			   : : "i#*X"(__builtin_ffs (__excepts)));	      \
> -			  0; }))					      \
> -      : 0)								      \
> -   : (feraiseexcept) (__excepts))
> +  (__extension__  ({ 							      \
> +    int __e = __excepts;						      \
> +    int __ret;								      \
> +    if (__builtin_constant_p (__e)					      \
> +        && ((__e & (__e-1)) == 0)					      \

> +        && (__e != FE_INVALID))						      \

Please also add the missing space around the operator while you are
changing it, and remove the redundant pair of parens.

Andreas.
Joseph Myers Jan. 26, 2015, 6:17 p.m. UTC | #2
On Mon, 26 Jan 2015, Adhemerval Zanella wrote:

> On 24-01-2015 00:21, Joseph Myers wrote:
> > On Fri, 23 Jan 2015, Adhemerval Zanella wrote:
> >
> >> This patch fixes the inline feraiseexcept and feclearexcept macros for
> >> powerpc by casting the input argument to integer before operation on it.
> >>
> >> It fixes BZ#17776.
> > This should add an (architecture-independent) testcase (probably covering 
> > such arguments to all <fenv.h> functions that accept an integer argument, 
> > not just those with the bug).
> 
> Thanks for the review, what about this testcase.  It surely do not cover all
> cases, but tests double arguments for current tested functions.

I think the relevant functions (that take integer arguments) are: 
feclearexcept fegetexceptflag feraiseexcept fesetexceptflag fetestexcept 
fesetround feenableexcept fedisableexcept.  The tests of the individual 
functions don't need to be particularly thorough, but passing a double 
argument in at least one call to each would seem a good idea.

> >> +    if (__builtin_constant_p (__e)					      \
> > Have you made sure __builtin_constant_p works for variables assigned like 
> > this from a macro argument?
> >
> The code:
> 
> #define DOUBLE_ARG(__x) ({ double __arg = __x; __arg; })
> #define DOUBLE_ARG2     (1.0)
> 
> void
> f (void)
> {
>   feclearexcept (1.0);
>   feclearexcept (DOUBLE_ARG (1));
>   feclearexcept (DOUBLE_ARG2);
> }
> 
> All generate just 'mtfsb0 31' instructions with GCC 4.8.

And if you pass a non-constant argument, the function is then not called 
inline?

> +/* To make sure the fenv inline function are used.  */
> +#undef __NO_MATH_INLINES
> +#define __NO_MATH_INLINES 0

But sysdeps/powerpc/bits/fenvinline.h tests "# ifndef __NO_MATH_INLINES", 
so a define to 0 will mean the macros aren't tested (i.e. this test 
doesn't test what it's meant to).  These macros tested in headers are 
generally defined/undefined rather than nonzero/zero.  So you should just 
has #undef without the #define.

> +static int
> +do_test (void)
> +{
> +#if FE_ALL_EXCEPT
> +  /* clear all exceptions and test if all are cleared  */
> +  feclearexcept (FE_ALL_EXCEPT);
> +  test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions",
> +                   NO_EXC);
> +
> +  /* Same test, but using double as argument  */
> +  feclearexcept ((double)FE_ALL_EXCEPT);
> +  test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions",
> +                   NO_EXC);

You should raise the exceptions before clearing them, for the tests of 
clearing to verify that something actually happened.  (So interleave the 
raise / clear tests: raise, test exceptions are raised, clear, test they 
are cleared, repeat with double arguments.)

> +#else
> +  puts ("No exceptions defined, cannot test");

You can still test that feraiseexcept and feclearexcept accept arguments 
of (double) FE_ALL_EXCEPT.

Indeed, I'm not sure you need the conditional in do_test at all.  
test_exceptions won't have anything to do if FE_ALL_EXCEPT is zero and so 
the individual exceptions are undefined (it will just print the test 
name), but you still get the test that do_test compiles (and the original 
problem was after all a compile failure).
diff mbox

Patch

diff --git a/NEWS b/NEWS
index 51714b7..b2f383b 100644
--- a/NEWS
+++ b/NEWS
@@ -17,8 +17,8 @@  Version 2.21
   17608, 17616, 17625, 17630, 17633, 17634, 17635, 17647, 17653, 17657,
   17658, 17664, 17665, 17668, 17682, 17702, 17717, 17719, 17722, 17723,
   17724, 17725, 17732, 17733, 17744, 17745, 17746, 17747, 17748, 17775,
-  17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797, 17803, 17806,
-  17834, 17844, 17848, 17868, 17869, 17870
+  17776, 17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797, 17803,
+  17806, 17834, 17844, 17848, 17868, 17869, 17870
 
 * A new semaphore algorithm has been implemented in generic C code for all
   machines. Previous custom assembly implementations of semaphore were
diff --git a/math/Makefile b/math/Makefile
index fec7627..3904e41 100644
--- a/math/Makefile
+++ b/math/Makefile
@@ -90,7 +90,8 @@  tests = test-matherr test-fenv atest-exp atest-sincos atest-exp2 basic-test \
 	test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \
 	test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \
 	test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \
-	test-fenv-tls test-fenv-preserve test-fenv-return $(tests-static)
+	test-fenv-tls test-fenv-preserve test-fenv-return test-fenvinline \
+	$(tests-static)
 tests-static = test-fpucw-static test-fpucw-ieee-static
 # We do the `long double' tests only if this data type is available and
 # distinct from `double'.
diff --git a/math/test-fenvinline.c b/math/test-fenvinline.c
new file mode 100644
index 0000000..e81e95f
--- /dev/null
+++ b/math/test-fenvinline.c
@@ -0,0 +1,135 @@ 
+/* Test for fenv inline implementations.
+   Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _GNU_SOURCE
+# define _GNU_SOURCE
+#endif
+
+/* To make sure the fenv inline function are used.  */
+#undef __NO_MATH_INLINES
+#define __NO_MATH_INLINES 0
+
+#include <fenv.h>
+#include <stdio.h>
+
+/*
+  Since not all architectures might define all exceptions, we define
+  a private set and map accordingly.
+*/
+#define NO_EXC 0
+#define INEXACT_EXC 0x1
+#define DIVBYZERO_EXC 0x2
+#define UNDERFLOW_EXC 0x04
+#define OVERFLOW_EXC 0x08
+#define INVALID_EXC 0x10
+#define ALL_EXC \
+        (INEXACT_EXC | DIVBYZERO_EXC | UNDERFLOW_EXC | OVERFLOW_EXC | \
+         INVALID_EXC)
+static int count_errors;
+
+#if FE_ALL_EXCEPT
+/* Test whether a given exception was raised.  */
+static void
+test_single_exception_fp (int exception,
+                          int exc_flag,
+                          double fe_flag,
+                          const char *flag_name)
+{
+  if (exception & exc_flag)
+    {
+      if (fetestexcept (fe_flag))
+        printf ("  Pass: Exception \"%s\" is set\n", flag_name);
+      else
+        {
+          printf ("  Fail: Exception \"%s\" is not set\n", flag_name);
+          ++count_errors;
+        }
+    }
+  else
+    {
+      if (fetestexcept (fe_flag))
+        {
+          printf ("  Fail: Exception \"%s\" is set\n", flag_name);
+          ++count_errors;
+        }
+      else
+        printf ("  Pass: Exception \"%s\" is not set\n", flag_name);
+    }
+}
+#endif
+
+static void
+test_exceptions (const char *test_name, int exception)
+{
+  printf ("Test: %s\n", test_name);
+#ifdef FE_DIVBYZERO
+  test_single_exception_fp (exception, DIVBYZERO_EXC, FE_DIVBYZERO,
+                            "DIVBYZERO");
+#endif
+#ifdef FE_INVALID
+  test_single_exception_fp (exception, INVALID_EXC, FE_INVALID,
+                            "INVALID");
+#endif
+#ifdef FE_INEXACT
+  test_single_exception_fp (exception, INEXACT_EXC, FE_INEXACT,
+			    "inexact");
+#endif
+#ifdef FE_UNDERFLOW
+  test_single_exception_fp (exception, UNDERFLOW_EXC, FE_UNDERFLOW,
+                            "UNDERFLOW");
+#endif
+#ifdef FE_OVERFLOW
+  test_single_exception_fp (exception, OVERFLOW_EXC, FE_OVERFLOW,
+                            "OVERFLOW");
+#endif
+}
+
+static int
+do_test (void)
+{
+#if FE_ALL_EXCEPT
+  /* clear all exceptions and test if all are cleared  */
+  feclearexcept (FE_ALL_EXCEPT);
+  test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions",
+                   NO_EXC);
+
+  /* Same test, but using double as argument  */
+  feclearexcept ((double)FE_ALL_EXCEPT);
+  test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions",
+                   NO_EXC);
+
+  /* raise all exceptions and test if all are raised  */
+  feraiseexcept (FE_ALL_EXCEPT);
+  test_exceptions ("feraiseexcept (FE_ALL_EXCEPT) raises all exceptions",
+                   ALL_EXC);
+  feclearexcept (FE_ALL_EXCEPT);
+
+  /* Same test, but using double as argument  */
+  feraiseexcept ((double)FE_ALL_EXCEPT);
+  test_exceptions ("feraiseexcept (FE_ALL_EXCEPT) raises all exceptions",
+                   ALL_EXC);
+  feclearexcept (FE_ALL_EXCEPT);
+#else
+  puts ("No exceptions defined, cannot test");
+#endif
+
+  return count_errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
index 35c2114..999c6b2 100644
--- a/sysdeps/powerpc/bits/fenvinline.h
+++ b/sysdeps/powerpc/bits/fenvinline.h
@@ -34,29 +34,41 @@ 
 
 /* Inline definition for feraiseexcept.  */
 #  define feraiseexcept(__excepts) \
-  ((__builtin_constant_p (__excepts)					      \
-    && ((__excepts) & ((__excepts)-1)) == 0				      \
-    && (__excepts) != FE_INVALID)					      \
-   ? ((__excepts) != 0							      \
-      ? (__extension__ ({ __asm__ __volatile__				      \
-			  ("mtfsb1 %s0"					      \
-			   : : "i#*X"(__builtin_ffs (__excepts)));	      \
-			  0; }))					      \
-      : 0)								      \
-   : (feraiseexcept) (__excepts))
+  (__extension__  ({ 							      \
+    int __e = __excepts;						      \
+    int __ret;								      \
+    if (__builtin_constant_p (__e)					      \
+        && ((__e & (__e-1)) == 0)					      \
+        && (__e != FE_INVALID))						      \
+      {									      \
+	if (__e != 0)							      \
+	  __asm__ __volatile__ ("mtfsb1 %s0"				      \
+				: : "i#*X" (__builtin_ffs (__e)));	      \
+        __ret = 0;							      \
+      }									      \
+    else								      \
+      __ret = feraiseexcept (__e);					      \
+    __ret;								      \
+  }))
 
 /* Inline definition for feclearexcept.  */
 #  define feclearexcept(__excepts) \
-  ((__builtin_constant_p (__excepts)					      \
-    && ((__excepts) & ((__excepts)-1)) == 0				      \
-    && (__excepts) != FE_INVALID)					      \
-   ? ((__excepts) != 0							      \
-      ? (__extension__ ({ __asm__ __volatile__				      \
-			  ("mtfsb0 %s0"					      \
-			   : : "i#*X"(__builtin_ffs (__excepts)));	      \
-			  0; }))					      \
-      : 0)								      \
-   : (feclearexcept) (__excepts))
+  (__extension__  ({ 							      \
+    int __e = __excepts;						      \
+    int __ret;								      \
+    if (__builtin_constant_p (__e)					      \
+        && ((__e & (__e-1)) == 0)					      \
+        && (__e != FE_INVALID))						      \
+      {									      \
+	if (__e != 0)							      \
+	  __asm__ __volatile__ ("mtfsb0 %s0"				      \
+				: : "i#*X" (__builtin_ffs (__e)));	      \
+        __ret = 0;							      \
+      }									      \
+    else								      \
+      __ret = feclearexcept (__e);					      \
+    __ret;								      \
+  }))
 
 # endif /* !__NO_MATH_INLINES.  */