Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#27721 closed defect (fixed)

Add wrappers around flint headers in Sage

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.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:

Status badges

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 re-typedefs 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 platform-dependent. 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 dimpase

let us do an upstream fix instead. they are quite patch-welcoming.

comment:2 Changed 3 years ago by embray

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 dimpase

  • Cc fredrik.johansson added

comment:4 Changed 3 years ago by dimpase

USE TYPEDEFS, PEOPLE!!!

comment:5 Changed 3 years ago by jdemeyer

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 embray

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 embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-27721
  • 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/NTL-interface.h being one special exception).

This of course results in "over-including" 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 post-include 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:

a9694f6Ticket #27721: Add wrappers for flint and arb headers when used in Sage.

comment:8 Changed 3 years ago by embray

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 follow-up: Changed 3 years ago by jdemeyer

I like the general idea but there is some room for bikeshedding:

  1. I would prefer to keep the name ulong in Cython code. There we have namespacing if we need (we can use sage.libs.flint.ulong instead of just ulong) and it makes it easier to just copy-paste new declarations from the flint/arb headers. So we don't actually need the new names fulong and fslong, we can just do ctypedef mp_limb_t ulong in Cython (without cdef extern).
  1. In flint, I like that you kept the old header names for documentation, but I would remove the commented-out 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 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

I like the general idea but there is some room for bikeshedding:

  1. I would prefer to keep the name ulong in Cython code. There we have namespacing if we need (we can use sage.libs.flint.ulong instead of just ulong) and it makes it easier to just copy-paste new declarations from the flint/arb headers. So we don't actually need the new names fulong and fslong, we can just do ctypedef mp_limb_t ulong in Cython (without cdef 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.

  1. In flint, I like that you kept the old header names for documentation, but I would remove the commented-out 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 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

ctypedef mp_limb_t ulong will put an actual typedef 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.h

or something like that?

+1

comment:12 in reply to: ↑ 11 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

ctypedef mp_limb_t ulong will put an actual typedef 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)

Ah, that's right, that's what I was missing. When looking at Cython-generated 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.

Last edited 3 years ago by embray (previous) (diff)

comment:13 Changed 3 years ago by git

  • Commit changed from a9694f661c15fc27e221148be430c31bec975dbf to 38b8e6b7014064e229e339b50dd5e7065b4ad4ca

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6b19feeTicket #27721: Add wrappers for flint and arb headers when used in Sage.
ba313c5fix up documentation commentary
9591189Revert ulong/slong -> fulong/fslong
38b8e6bRework these wrappers a bit more:

comment:14 Changed 3 years ago by embray

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 embray

Is there anything holding this up?

comment:16 Changed 2 years ago by vbraun

  • 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 embray

Thanks. I'll squash in a few minutes.

comment:18 Changed 2 years ago by git

  • 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:

0aa0177Ticket #27721: Add wrappers for flint and arb headers when used in Sage.

comment:19 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

comment:20 Changed 2 years ago by vbraun

  • Branch changed from u/embray/ticket-27721 to 0aa0177220205a67e4981b994f2efecdadb057ac
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 2 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.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 chapoton

  • 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 embray

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 chapoton

I have made #28103

Note: See TracTickets for help on using tickets.