Skip to content

Windows support #10

Closed
#28
#26
Closed
Windows support#10
#28
#26

Description

@isuruf
isuruf
opened on Jun 9, 2020

I ran the test suite and for each python version, the error is different.
python 3.6

>python test/test.py 
test_fmpz...

python 3.7

>python test/test.py 
Traceback (most recent call last):
  File "test/test.py", line 439, in <module>
    sys.stdout.write("test_fmpz..."); test_fmpz(); print("OK")
  File "test/test.py", line 19, in test_fmpz
    assert flint.fmpz() == flint.fmpz(0)
AssertionError

python 3.8

>python test/test.py 
Traceback (most recent call last):
  File "test/test.py", line 439, in <module>
    sys.stdout.write("test_fmpz..."); test_fmpz(); print("OK")
  File "test/test.py", line 26, in test_fmpz
    assert ltype(s) + rtype(t) == s + t
AssertionError

One possible issue could be that, long is used everywhere instead of long long. For example, PyLong_AsLongLongAndOverflow should be used instead of PyLong_AsLongAndOverflow at https://github.com/fredrik-johansson/python-flint/blob/master/src/fmpz.pyx#L4.

Activity

  1. fredrik-johansson commented on Jun 9, 2020

    @fredrik-johansson
    fredrik-johansson
    on Jun 9, 2020
    Collaborator

    Yes. When I started this project, I believe Flint was still using 'long' everywhere, and I followed the same convention. There needs to be a 'slong' typedef to mirror 'ulong' for Windows 64 support.

  2. oscarbenjamin commented on Dec 5, 2022

    @oscarbenjamin
    oscarbenjamin
    on Dec 5, 2022
    Collaborator

    So I was trying to figure out what exactly slong and ulong are supposed to be here and then I found this in the Arb docs:

    Since the C types long and unsigned long do not have a standardized size in practice, FLINT defines slong and ulong types which are guaranteed to be 32 bits on a 32-bit system and 64 bits on a 64-bit system. They are also guaranteed to have the same size as GMP’s mp_limb_t. GMP builds with a different limb size configuration are not supported at all. For convenience, the macro FLINT_BITS specifies the word length (32 or 64) of the system.

    https://arblib.org/issues.html

    Then looking in the python-flint source I find this:
    https://github.com/fredrik-johansson/python-flint/blob/4ec3a4b9abb6afb71b68e9c181e9f51681cc30f2/src/flint/_flint.pxd#L1-L2
    I guess that means that python-flint does not work for any 32 bit system (although I thought that I previously had it running successfully in CI for 32 bit Linux...)

    We can see ulong and slong defined in python-flint here:
    https://github.com/fredrik-johansson/python-flint/blob/4ec3a4b9abb6afb71b68e9c181e9f51681cc30f2/src/flint/_flint.pxd#L21-L32

    The problem then is that on Windows both long and unsigned long are 32 bits even on a 64 bit system. If I understand this correctly then long long doesn't really help use here. The purpose of long long is to be always at least 64 bits but that's not what we want on a 32 bit system.

    I suppose that the most important thing for now is ensuring that slong and ulong are used consistently in python-flint when accessing Flint/Arb functions that are typed with those. Then fixing it up to make python-flint do the right thing on 32 bit systems is just a case of figuring out the right typedef for slong and ulong for those systems which can be done later.

    If python-flint only works for a 64 bit system is there any reason not to just set slong and ulong as just int64_t and uint64_t for now?

    It's a long time since I wrote Cython code. Is it not possible to just the ulong and slong types from the flint headers?

  3. fredrik-johansson commented on Dec 5, 2022

    @fredrik-johansson
    fredrik-johansson
    on Dec 5, 2022
    Collaborator

    I guess that means that python-flint does not work for any 32 bit system (although I thought that I previously had it running successfully in CI for 32 bit Linux...)

    That FLINT_BITS doesn't seem to be used in any critical place so having the wrong value is not (currently) causing any breakage.

    It's a long time since I wrote Cython code. Is it not possible to just the ulong and slong types from the flint headers?

    Cython compilation works in two stages: Cython -> C translation, and C compilation. C headers are only used in the second stage. You need to provide equivalent Cython-format definitions for the first stage.

    I think the top of flint.pxd needs to look something like this:

    IF WINDOWS64:
    ctypedef unsigned long long ulong
    ctypedef long long slong
ELSE:
    ctypedef unsigned long ulong
    ctypedef long slong

ctypedef ulong mp_limb_t
ctypedef slong mp_size_t
ctypedef slong mp_exp_t
ctypedef ulong mp_bitcnt_t
ctypedef slong fmpz_struct
ctypedef ulong flint_bitcnt_t

    Then we just need to replace long -> slong throughout the code and fix a few isolated cases like calls to PyInt_FromLong.

    I don't know the best way to check for WINDOWS64.

    An alternative might be to typedef slong/ulong to ssize_t/size_t. Not sure if they are always the right size. Logically they ought to be, but C isn't logical :-)

  4. isuruf commented on Dec 5, 2022

    @isuruf
    isuruf
    on Dec 5, 2022
    MemberAuthor

    Cython compilation works in two stages: Cython -> C translation, and C compilation. C headers are only used in the second stage. You need to provide equivalent Cython-format definitions for the first stage.

    Not really. If the type definitions come from a C header file Cython will use those. Cython just needs to know the "approximate type".

    From cython docs at https://cython.readthedocs.io/en/stable/src/userguide/external_C_code.html:

    If the header file uses typedef names such as word to refer to platform-dependent flavours of numeric types, you will need a corresponding ctypedef statement, but you don’t need to match the type exactly, just use something of the right general kind (int, float, etc). For example,:

    ctypedef int word

    will work okay whatever the actual size of a word is (provided the header file defines it correctly). Conversion to and from Python types, if any, will also be used for this new type.

    Issue is that slong and two others are defined outside of a header https://github.com/fredrik-johansson/python-flint/blob/4ec3a4b9abb6afb71b68e9c181e9f51681cc30f2/src/flint/_flint.pxd#L30-L32. Pull them inside a cdef extern from "flint/fmpz.h": and Cython will not define slong.

  5. oscarbenjamin commented on Dec 5, 2022

    @oscarbenjamin
    oscarbenjamin
    on Dec 5, 2022
    Collaborator

    Not really. If the type definitions come from a C header file Cython will use those. Cython just needs to know the "approximate type".

    Thanks both! That's very helpful. I'll give this a try.

  6. oscarbenjamin commented on Dec 6, 2022

    @oscarbenjamin
    oscarbenjamin
    on Dec 6, 2022
    Collaborator

    This is enough to get a working wheel that passes all tests on 64 bit Windows:

    diff --git a/src/flint/_flint.pxd b/src/flint/_flint.pxd
index c471237..5121cb6 100644
--- a/src/flint/_flint.pxd
+++ b/src/flint/_flint.pxd
@@ -9,11 +9,13 @@ cdef extern from "Python.h":
     int PyObject_TypeCheck(object, PyTypeObject*)
     int PyInt_Check(PyObject *o)
     PyObject* PyInt_FromLong(long ival)
+    PyObject* PyInt_FromLongLong(long ival)
     int PyLong_Check(PyObject *o)
     long PyInt_AS_LONG(PyObject *io)
     double PyFloat_AS_DOUBLE(PyObject *io)
     Py_ssize_t PyList_GET_SIZE(PyObject *list)
     long PyLong_AsLongAndOverflow(PyObject *pylong, int *overflow)
+    long long PyLong_AsLongLongAndOverflow(PyObject *pylong, int *overflow)

 DEF FMPZ_UNKNOWN = 0
 DEF FMPZ_REF = 1
diff --git a/src/flint/fmpz.pyx b/src/flint/fmpz.pyx
index 5cf5ff0..77adea1 100644
--- a/src/flint/fmpz.pyx
+++ b/src/flint/fmpz.pyx
@@ -1,7 +1,7 @@
 cdef inline int fmpz_set_pylong(fmpz_t x, obj):
     cdef int overflow
-    cdef long longval
-    longval = PyLong_AsLongAndOverflow(<PyObject*>obj, &overflow)
+    cdef long long longval
+    longval = PyLong_AsLongLongAndOverflow(<PyObject*>obj, &overflow)
     if overflow:
         s = "%x" % obj
         fmpz_set_str(x, chars_from_str(s), 16)
@@ -28,7 +28,7 @@ cdef fmpz_get_intlong(fmpz_t x):
         libc.stdlib.free(s)
         return v
     else:
-        return <long>x[0]
+        return <long long>x[0]

 cdef int fmpz_set_any_ref(fmpz_t x, obj):
     if typecheck(obj, fmpz):

  7. oscarbenjamin commented on Dec 6, 2022

    @oscarbenjamin
    oscarbenjamin
    on Dec 6, 2022
    Collaborator

    Wait, that's the wrong diff. It's this:

    -# fixme!
-DEF FLINT_BITS = 64
+# _flint.pxd
+#
+# Define the contents of the Python, GMP, Flint and Arb headers.

 cdef extern from "Python.h":
     ctypedef void PyObject
@@ -7,17 +8,27 @@ cdef extern from "Python.h":
     ctypedef long Py_ssize_t
     int PyObject_TypeCheck(object, PyTypeObject*)
     int PyInt_Check(PyObject *o)
-    PyObject* PyInt_FromLong(long ival)
     int PyLong_Check(PyObject *o)
     long PyInt_AS_LONG(PyObject *io)
     double PyFloat_AS_DOUBLE(PyObject *io)
     Py_ssize_t PyList_GET_SIZE(PyObject *list)
     long PyLong_AsLongAndOverflow(PyObject *pylong, int *overflow)
+    long long PyLong_AsLongLongAndOverflow(PyObject *pylong, int *overflow)

 DEF FMPZ_UNKNOWN = 0
 DEF FMPZ_REF = 1
 DEF FMPZ_TMP = 2

+#
+# Note: ulong and slong are used throughout Flint/Arb. They are expected to be
+# 32 bit unsigned and signed integer types on a 32 bit system and 64 bit on a
+# 64 bit system. We denote them as unsigned long and long here which would be
+# incorrect on 64 bit Windows but the definition here does not matter because
+# their actual sizes will be determined by the values from gmp.h and
+# flint/flint.h. Their size in bits (32 or 64) is recorded in the FLINT_BITS
+# macro which is defined in flint/flint.h.
+#
+
 cdef extern from "gmp.h":
     ctypedef unsigned long ulong
     ctypedef unsigned long mp_limb_t
@@ -27,11 +38,14 @@ cdef extern from "gmp.h":
     ctypedef mp_limb_t* mp_srcptr
     ctypedef unsigned long mp_bitcnt_t

-ctypedef long fmpz_struct
-ctypedef long slong
-ctypedef ulong flint_bitcnt_t
+cdef extern from "flint/fmpz.h":
+    ctypedef long slong
+    ctypedef ulong flint_bitcnt_t
+
+ctypedef slong fmpz_struct

 cdef extern from "flint/flint.h":
+    const int FLINT_BITS
     ctypedef void * flint_rand_t
     void flint_randinit(flint_rand_t state)
     void flint_randclear(flint_rand_t state)
diff --git a/src/flint/fmpz.pyx b/src/flint/fmpz.pyx
index 5cf5ff0..77adea1 100644
--- a/src/flint/fmpz.pyx
+++ b/src/flint/fmpz.pyx
@@ -1,7 +1,7 @@
 cdef inline int fmpz_set_pylong(fmpz_t x, obj):
     cdef int overflow
-    cdef long longval
-    longval = PyLong_AsLongAndOverflow(<PyObject*>obj, &overflow)
+    cdef long long longval
+    longval = PyLong_AsLongLongAndOverflow(<PyObject*>obj, &overflow)
     if overflow:
         s = "%x" % obj
         fmpz_set_str(x, chars_from_str(s), 16)
@@ -28,7 +28,7 @@ cdef fmpz_get_intlong(fmpz_t x):
         libc.stdlib.free(s)
         return v
     else:
-        return <long>x[0]
+        return <long long>x[0]

 cdef int fmpz_set_any_ref(fmpz_t x, obj):
     if typecheck(obj, fmpz):

  8. oscarbenjamin commented on Dec 6, 2022

    @oscarbenjamin
    oscarbenjamin
    on Dec 6, 2022
    Collaborator

    I've opened a PR (#28) that I think fixes this.

  9. oscarbenjamin
    closed this as completedin #28on Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions