#27721 closed defect (fixed)
Add wrappers around flint headers in Sage
Reported by:  embray  Owned by:  

Priority:  critical  Milestone:  sage8.9 
Component:  interfaces  Keywords:  
Cc:  jdemeyer, wbhart, dimpase, fredrik.johansson  Merged in:  
Authors:  Erik Bray  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  0aa0177 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
There is a problem in a few of the packages they use (flint is not the only one, but it is one where this specific problem is most pervasive) in that they all use the name ulong
in some way or other. flint also has an slong
but that does not seem to have as many conflicts.
I first reported this in #27714 but this ticket is to propose a specific fix.
I've noted this problem in at least:
 flint/flint.h:
#define ulong mp_limb_t #define slong mp_limb_signed_t
 zn_poly/zn_poly.h:
/* I get really sick of typing unsigned long. */ typedef unsigned long ulong;
 pari/parigen.h:
#ifdef _WIN64 typedef unsigned long long pari_ulong; #define long long long #define labs llabs #else typedef unsigned long pari_ulong; #endif #define ulong pari_ulong
And for completeness, it's worth noting these definitions from gmp.h:
#ifdef __GMP_SHORT_LIMB typedef unsigned int mp_limb_t; typedef int mp_limb_signed_t; #else #ifdef _LONG_LONG_LIMB typedef unsigned long long int mp_limb_t; typedef long long int mp_limb_signed_t; #else typedef unsigned long int mp_limb_t; typedef long int mp_limb_signed_t; #endif #endif typedef unsigned long int mp_bitcnt_t;
The problem is most evident in modules that happen to combine any two of these headers. For example in src/sage/rings/polynomial/polynomial_zmod_flint.pyx both flint.h and zn_poly.h are used. Because of the #define ulong mp_limb_t
in flint.h, the typedef in zn_poly.h expands to: typedef unsigned long mp_limb_t
.
Because this compiles to a C++ file this happens to be just fine most of the time because C++ allows benign retypedefs and indeed it just happens that this is compatible with the original definition of mp_limb_t
from gmp.h, at least most of the time.
However, as I found in #27714 that doesn't always have to be true. The GMP system package on Cygwin has GMP configured in such a way that _LONG_LONG_LIMB
is defined, so it defines typedef long long int mp_limb_t
, and all this fragile overloading breaks down.
I think in some sense flint is the "worst" problem here since it ties its ulong
and slong
macros to typedefs in GMP which themselves can, we now know, be platformdependent. So this introduces a lot of problems.
I'm experimenting with adding a wrapper around all includes of flint headers in Sage in order to work around this problem, but I'd be happy to have other suggestions.
Change History (24)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
That might be preferable, but I'm not sure what the best thing would be to do that wouldn't massively break backwards compatibility with other code that uses flint (and hence its ulong and slong macros).
comment:3 Changed 3 years ago by
 Cc fredrik.johansson added
comment:4 Changed 3 years ago by
USE TYPEDEFS, PEOPLE!!!
comment:5 Changed 3 years ago by
I had to solve a similar problem in cypari2: https://github.com/sagemath/cypari2/blob/master/cypari2/cypari.h included from https://github.com/sagemath/cypari2/blob/7bf42118a6df470ae67fae6d403a0f0796c17355/cypari2/types.pxd#L142
The same idea (#undef ulong
) could be used for FLINT, provided that they don't expose the name ulong
in other macros. For example, this macros will break after #undef ulong
:
#define PTR_TO_COEFF(x) (((ulong) (x) >> 2)  (WORD(1) << (FLINT_BITS  2)))
but Sage doesn't use that.
Let me see if this quick fix works.
comment:6 Changed 3 years ago by
Before you do anything, I already have a patch in progress (actually mostly done; just testing).
In addition to #undef ulong
it also adds two new typedefs typedef mp_limb_t fulong
(f for flint) and a similar on for fslong
. Then code in sage that previously used the fake types ulong
and slong
are updated to use these instead.
It also requires wrapping the arb headers since most of them include flint.h.
comment:7 Changed 3 years ago by
 Branch set to u/embray/ticket27721
 Commit set to a9694f661c15fc27e221148be430c31bec975dbf
 Status changed from new to needs_review
Here is my proposal for wrappers for the flint and arb headers to use when including them in Sage. Rather than make many wrappers for each individual header, I just include all the flint headers in the flint wrapper, and similarly in the arb wrapper (with flint/NTLinterface.h
being one special exception).
This of course results in "overincluding" in some cases, but I don't think this is a serious problem.
I'm also aware that in newer versions of Cython it's possible to add arbitrary postinclude code in a docstring under a cdef extern
statement, but I felt that this would be more reliable and manageable than dozens of essentially duplicate "docstrings".
Between this, and the small PR to flint from #27714 this resolved all build issues for me.
New commits:
a9694f6  Ticket #27721: Add wrappers for flint and arb headers when used in Sage.

comment:8 Changed 3 years ago by
If there's anything I could have done more simply I'd be happy to hear it, but this was the cleanest approach I could come up with.
comment:9 followup: ↓ 10 Changed 3 years ago by
I like the general idea but there is some room for bikeshedding:
 I would prefer to keep the name
ulong
in Cython code. There we have namespacing if we need (we can usesage.libs.flint.ulong
instead of justulong
) and it makes it easier to just copypaste new declarations from the flint/arb headers. So we don't actually need the new namesfulong
andfslong
, we can just doctypedef mp_limb_t ulong
in Cython (withoutcdef extern
).
 In flint, I like that you kept the old header names for documentation, but I would remove the commentedout
cdef extern from
(but keep the header name) and I would do the same for arb.
What do you think?
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 3 years ago by
Replying to jdemeyer:
I like the general idea but there is some room for bikeshedding:
 I would prefer to keep the name
ulong
in Cython code. There we have namespacing if we need (we can usesage.libs.flint.ulong
instead of justulong
) and it makes it easier to just copypaste new declarations from the flint/arb headers. So we don't actually need the new namesfulong
andfslong
, we can just doctypedef mp_limb_t ulong
in Cython (withoutcdef extern
).
I think I see what you're saying. But how would this work in cases where we want to use flint and zn_poly together, or flint and pari together? IIUC ctypedef mp_limb_t ulong
will put an actual typedef mp_limb_t ulong
into the C sources, which could conflict with the ulong
from those other libraries' headers.
Or should we provide wrappers for them too?
Maybe I'm missing something though. I'm willing to try it.
 In flint, I like that you kept the old header names for documentation, but I would remove the commentedout
cdef extern from
(but keep the header name) and I would do the same for arb.What do you think?
So keep the comments but just the original header name, like
# flint/flint.h
or something like that?
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 3 years ago by
Replying to embray:
ctypedef mp_limb_t ulong
will put an actualtypedef mp_limb_t ulong
into the C sources
Cython mangles names which are not cdef extern
. You'll get something like
typedef mp_limb_t __pyx_sage_libs_flint_ulong;
(not exactly but you get the point)
So keep the comments but just the original header name, like
# flint/flint.hor something like that?
+1
comment:12 in reply to: ↑ 11 Changed 3 years ago by
Replying to jdemeyer:
Replying to embray:
ctypedef mp_limb_t ulong
will put an actualtypedef mp_limb_t ulong
into the C sourcesCython mangles names which are not
cdef extern
. You'll get something liketypedef mp_limb_t __pyx_sage_libs_flint_ulong;(not exactly but you get the point)
Ah, that's right, that's what I was missing. When looking at Cythongenerated code I'm so used at this point to just ignoring the __pyx
prefixes that I forget they're there :) I'll give that a try then.
comment:13 Changed 3 years ago by
 Commit changed from a9694f661c15fc27e221148be430c31bec975dbf to 38b8e6b7014064e229e339b50dd5e7065b4ad4ca
comment:14 Changed 3 years ago by
Rebased and addressed review comments, plus fixed some more build issues I had on Linux.
Would still like to squash before merging since otherwise this makes a mess of git blame on several of these files.
comment:15 Changed 2 years ago by
Is there anything holding this up?
comment:16 Changed 2 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
Feel free to squash the commits...
comment:17 Changed 2 years ago by
Thanks. I'll squash in a few minutes.
comment:18 Changed 2 years ago by
 Commit changed from 38b8e6b7014064e229e339b50dd5e7065b4ad4ca to 0aa0177220205a67e4981b994f2efecdadb057ac
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:
0aa0177  Ticket #27721: Add wrappers for flint and arb headers when used in Sage.

comment:19 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:20 Changed 2 years ago by
 Branch changed from u/embray/ticket27721 to 0aa0177220205a67e4981b994f2efecdadb057ac
 Resolution set to fixed
 Status changed from positive_review to closed
comment:21 Changed 2 years ago by
 Milestone changed from sage8.8 to sage8.9
Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.
comment:22 Changed 2 years ago by
 Commit 0aa0177220205a67e4981b994f2efecdadb057ac deleted
I get (with python3) this new failure in 8.9.b0:
File "src/sage/rings/polynomial/polynomial_rational_flint.pyx", line 2055, in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.galois_group Failed example: G = f.galois_group(); G Expected: Transitive group number 5 of degree 4 Got: Exception (FLINT memory_manager). Unable to allocate memory. Transitive group number 5 of degree 4
Could that be caused by this ticket ?
comment:23 Changed 2 years ago by
Probably not, though I would never rule anything out entirely at this point. It's strange that you only get it on Python 3 though. That's the main reason I'm doubtful. Please open a separate ticket and I can look into it when I can.
comment:24 Changed 2 years ago by
I have made #28103
let us do an upstream fix instead. they are quite patchwelcoming.