Re: [PATCH] powerpc: Fix inline feraiseexcept, feclearexcept macros

[Joseph Myers <joseph at codesourcery dot com>]

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

-- 
Joseph S. Myers
joseph@codesourcery.com