[Cython] Cygwin: Handling missing C99 long double functions

[Erik Bray]

On Tue, May 3, 2016 at 7:15 PM, Robert Bradshaw <robertwb at gmail.com> wrote:
> On Tue, May 3, 2016 at 3:04 AM, Erik Bray <erik.m.bray at gmail.com> wrote:
>>
>> On Thu, Apr 28, 2016 at 9:29 AM, Robert Bradshaw
>> <robertwb at math.washington.edu> wrote:
>> > On Wed, Apr 27, 2016 at 3:07 AM, Erik Bray <erik.m.bray at gmail.com>
>> > wrote:
>> >>
>> >> On Tue, Apr 26, 2016 at 10:55 PM, Robert Bradshaw <robertwb at gmail.com>
>> >> wrote:
>> >> > On Tue, Apr 26, 2016 at 8:36 AM, Erik Bray <erik.m.bray at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> On Tue, Apr 26, 2016 at 5:16 PM, Dima Pasechnik
>> >> >> <dimpase+github at gmail.com> wrote:
>> >> >> > Hi,
>> >> >> > certainly we did something with Sage on cygwin to work around
>> >> >> > these...
>> >> >> > Just in case,
>> >> >>
>> >> >> From what I can tell there are several places where Sage has hacked
>> >> >> around this issue in different packages, but it's not doing anything
>> >> >> specifically with it for Cython.  Sage uses the Cephes math lib to
>> >> >> support these functions on FreeBSD--and apparently used to use it on
>> >> >> Cygwin too, but disabled that for some reason.
>> >> >>
>> >> >> Regardless, Cython should ultimately do something sensible in these
>> >> >> cases.
>> >> >>
>> >> >> My general thinking is that in cases where Cython generates code
>> >> >> containing C math functions, it ought to support a fallback.  This
>> >> >> will require some feature checks so that Cython can generate
>> >> >> wrappers,
>> >> >> when necessary, around the double versions of those functions (as
>> >> >> Numpy currently does).
>> >> >
>> >> >
>> >> > Let's make things concrete. You're complaining that something like
>> >> >
>> >> > cdef extern from "math.h":
>> >> >     long double sqrtl(long double)
>> >> >
>> >> > def foo(long double x):
>> >> >     return sqrtl(x)
>> >> >
>> >> > Doesn't work on Cygwin?
>> >> >
>> >> > The same is true for *any* C function that you use that's not totally
>> >> > portable (this is the bane of trying to use C). I don't think Cython
>> >> > should
>> >> > be detecting this and substituting a (less accurate) sqrt for sqrtl
>> >> > in
>> >> > this
>> >> > case. If you want do do this, write your own headers that
>> >> > (conditionally)
>> >> > define these things however you want.
>> >>
>> >> No, not at all.  That would be silly.  Let me be clearer...
>> >>
>> >> > Or, are you complaining that Cython's test suite doesn't pass on some
>> >> > Cygwin
>> >> > because there are tests of features not available on Cygwin? (Your
>> >> > original
>> >> > email isn't clear.) If so, the test framework can be set up to
>> >> > exclude
>> >> > these
>> >> > tests on that platform.
>> >>
>> >> There are really two concerns.  This is one of them, yes.  The first
>> >> question is what would be the best way to exclude certain tests on
>> >> certain platforms?  There's no clear documentation on that (which is
>> >> fine, but it's why I'm asking :)
>> >
>> >
>> > Probably add a tag and then modify runtests.py to detect Cygwin and
>> > automatically add this to the exclusion list.
>>
>> Okay.
>>
>> >> The second concern, and more serious, is that there *are* cases where
>> >> Cython generates code that uses long double functions where, for
>> >> example, long double is passed as an argument.  For example the
>> >> following code
>> >>
>> >>     def truncate_long_double(long double x):
>> >>         cdef float r = int(x)
>> >>         return r
>> >>
>> >> compiles to something that includes:
>> >>
>> >>       /* "truncl.pyx":2
>> >>      * def truncate_long_double(long double x):
>> >>      *     cdef float r = int(x)             # <<<<<<<<<<<<<<
>> >>      *     return r
>> >>      */
>> >>       __pyx_t_1 = truncl(__pyx_v_x);
>> >>       __pyx_v_r = __pyx_t_1;
>> >>
>> >>       /* "truncl.pyx":3
>> >>      * def truncate_long_double(long double x):
>> >>      *     cdef float r = int(x)
>> >>      *     return r             # <<<<<<<<<<<<<<
>> >>      */
>> >>       __Pyx_XDECREF(__pyx_r);
>> >>       __pyx_t_2 = PyFloat_FromDouble(__pyx_v_r); if
>> >> (unlikely(!__pyx_t_2)) __PYX_ERR(0, 3, __pyx_L1_error)
>> >>
>> >> It's not not clear what the best way would be to *not* use this code
>> >> on platforms where truncl missing (and there are a handful of other
>> >> examples besides truncl). This code is generated by an optimization
>> >> that was added here: http://trac.cython.org/ticket/400  In this case
>> >> replacing truncl with trunc shouldn't hurt.
>> >
>> >
>> > Here, yes, but in general truncl(x) != trunc(x) for, say, 2**54 + 1.
>> >
>> >> It's not uncommon (I think) to ship pre-cythonized C sources in a
>> >> package's source distribution so that it can be installed (especially
>> >> by pip) without requiring the user to have Cython.  This code will
>> >> fail to compile on platforms like Cygwin.
>> >>
>> >> But it's not clear how to handle this at Cythonization time either
>> >> since it doesn't know in advance what platform it will be targeting.
>> >> I'm suggesting that maybe rather than using these functions directly
>> >> Cython should generate some fallbacks for where they're not available,
>> >> or at least have the option to.
>> >
>> >
>> > The safest is to never use the long double type in this case--we assume
>> > that
>> > if long double is present (used), so are the corresponding functions in
>> > math.h (which is wrong for this platform, but if we need to use powl I
>> > don't
>> > know what else to do). Alternatively, ship your own (conditionally
>> > defined)
>> > fallbacks.
>>
>> I agree that it would be safest not to use long double where long
>> double functions are not support, and an application that does should
>> probably check for that.
>>
>> The problem is that Cython is generating code that simply *cannot* be
>> compiled on such platforms.
>
>
> Does Cython ever generate such code when long doubles are not used in the
> source? If so, we should fix that. But I don't think Cython should be
> swapping in double versions for long double operations.

No, but the problem is that Cython is a pretty heavy layer of
abstraction.  In the example I gave no long double function was ever
used explicitly--it was added by a compiler optimization.  I probably
wouldn't write that code thinking that I will need a workaround on
platforms that don't have truncl.

In the slightly more explicit cases of fmod and pow I might agree.
Though there still isn't a great way to anticipate the need to write
separate handling for long double functions into a module, for a
platform that doesn't support them.  I can't write something in a
Cython module like:

    if sys.platform != 'cygwin':
        def my_long_double_func(long double x):
            ....

Maybe would be nice to have a simple compiler directive that outright
disables compiling a function depending on a platform check. (I think
this would also be useful for the test framework--the current system
of module-level tags is not fine-grained as I would like, I'm finding.

>> My suggestion would be to replace the
>> current calls to [trunc,fmod,pow]l to the appropriate
>> __Pyx_%(func)_%(type) wrapper which would provide two alternatives:
>> One that works properly, and one that falls back to using the double
>> version of the function (Numpy does this so there is some precedent).
>> I would go further and suggest throwing up a pre-compiler warning when
>> using the double versions.
>>
>> The only question is what condition (in my mind) is what to use as a
>> condition for selecting the fallbacks.  For now I'm thinking something
>> quite specific like
>>
>>     #if defined(__CYGWIN__) && defined(_LDBL_EQ_DBL)
>>
>> since I'm apparently the only person who cares about this at the
>> moment, and Cygwin is the platform I'm targeting.
>>
>> Again, my only real concern right now is that Cython generates code
>> that can be compiled on Cygwin.
>
>
> If you want this behavior, wouldn't including a header
>
> #if defined(__CYGWIN__) && defined(_LDBL_EQ_DBL)
> #define truncl trunc
> #define fmodl fmod
> #define powl pow
> #endif
>
> as part of your package do the job?

That's a reasonable workaround. But I would argue that should be baked
into Cythonized code, at least optionally, and should generate a
warning.

Best,
Erik