Re: [PATCH v2 5/10] Tilera (and Linux asm-generic) support for glibc

[Chris Metcalf <cmetcalf at tilera dot com>]

On 11/11/2011 12:59 PM, Roland McGrath wrote:
> >  diff --git a/include/fenv.h b/include/fenv.h
> >  index 3605f81..c061eb9 100644
> >  --- a/include/fenv.h
> >  +++ b/include/fenv.h
> >  @@ -20,4 +20,10 @@ libm_hidden_proto (feholdexcept)
> >   libm_hidden_proto (feupdateenv)
> >   libm_hidden_proto (fetestexcept)
> >
> >  +/* Allow coding feraiseexcept() without guarding the call with an
> >  +   ifdef of the argument, to suport platforms without FP exceptions.  */
>  There's a typo in this comment.

Oops, so there is.  I had to re-read it twice to notice it, for some reason.

> >  +#if FE_ALL_EXCEPT == 0
> >  +# define feraiseexcept(e) ({ 1; })
> >  +#endif
>  This is just generically bad macro practice: it fails to evaluate its argument.

Except in this case, that's exactly why this macro is defined the way
it is.  The issue is that commit 77425c63e72b (last month) removed a
lot of the guards around calls to feraiseexcept(), with diffs that
looked like this:

@@ -112,10 +107,8 @@ __cexp (__complex__ double x)
       __real__ retval = __nan ("");
       __imag__ retval = __nan ("");

-#ifdef FE_INVALID
       if (rcls != FP_NAN || icls != FP_NAN)
        feraiseexcept (FE_INVALID);
-#endif
     }

   return retval;

The removal of the guards wasn't mentioned in the commit message,
which just said it was for adding branch prediction; I assume it was
viewed as just normal cleanup that didn't require mentioning.  A
number of other minor cleanups (variable scope, whitespace, etc.) also
happened in this commit.  There are still some files that test with an
"#ifdef FE_INVALID", for example (s_ccoshf.c, etc.) but the majority
of them no longer do.

The problem is that if some platform does not support FE_INVALID,
there will be no definition of it in<fenv.h>, and this code then
can't compile.  So my workaround is to observe that if a given
platform has no exceptions at all, then feraiseexcept() will be a
no-op no matter what its argument is.  By ignoring the argument we
then allow the math code to compile.  It's a bit of a hack, but it
does mean we don't have to put back all the "#ifdef FE_INVALID", etc.,
lines (though from a standards point of view that's probably still the
best solution).

I would have preferred to put the feraiseexcept() hack in some
tile-specific header, but I couldn't see an obvious one.
<bits/mathdef.h>  is of course installed, so although I could add an
"#if _LIBC" stanza there, it seemed inappropriate.  The alternative
would be a new file, e.g., sysdeps/generic/libc-fenv.h, which just
includes<fenv.h>  by default, but overrideable by platform, which the
math source files would be switched over to use.  But that seemed
pretty heavyweight, so I didn't go down that route.

Sorry not to have included more rationale in the original submissions;
in retrospect it was an obvious oversight.

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com