Fix dependencies on PARI

[jdemeyer]

The extension modules linking against PARI should explicitly depend on the PARI headers. This is in particular true for sage/ext/interrupt/interrupt.pyx.

Since every module cimporting something from PARI will cimport sage.libs.pari.types, we add the explicit dependency there.

In order to have the correct distutils keywords for sage/ext/interrupt/interrupt.pyx, we cimport sage.libs.pari.paridecl there.

CC: @kiwifb @jpflori

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 5d0e4d3

Reviewer: Jean-Pierre Flori

Issue created by migration from https://trac.sagemath.org/ticket/19610

[jdemeyer]

Branch: u/jdemeyer/fix_dependencies_on_pari

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

Commit: 5d0e4d3

[jdemeyer]

New commits:

5d0e4d3

Fix dependencies on PARI

[nexttime]

comment:4

Maybe I misunderstood the purpose of this ticket (cf. the description I added), but after pulling the branch here, interrupt.so still gets linked to both PARI and GMP (i.e., MPIR).

(Probably you just intended a proper rebuild of it upon changes to libgmp, but the patch looks as if you intended to remove the libraries from the extension module.)

IMHO at most PARI should get linked to it.

[nexttime]

comment:5

Ok, I see, in sage/libs/pari/paridecl.pxd, there's (still)

# distutils: libraries = gmp pari

Do we really need gmp there as well?

[nexttime]

comment:6

Certainly interrupt.so doesn't use/need GMP (but PARI_SIGINT_* from PARI), but can we (easily) remove GMP from it?

AFAICS, the declarations are only needed/used in sage/ext/interrupt/implementation.c, i.e., genuine C code, not Cython.

[nexttime]

comment:7

The following works for me to get rid of GMP (i.e., avoids overlinking interrupt.so with libgmp):

diff --git a/src/sage/ext/interrupt/implementation.c b/src/sage/ext/interrupt/implementation.c
index 7b96199..55dac47 100644
--- a/src/sage/ext/interrupt/implementation.c
+++ b/src/sage/ext/interrupt/implementation.c
@@ -42,7 +42,18 @@ AUTHORS:
 #ifdef __linux__
 #include <sys/prctl.h>
 #endif
-#include <pari/pari.h>
+
+#include <pari/paricfg.h>
+#include <pari/parisys.h>      /* needed for THREAD and VOLATILE */
+#if 0
+/* further includes would be needed, see below */
+#include <pari/paricom.h>      /* needed for PARI_SIGINT_* */
+#else
+/* It's hard to selectively include PARI headers (i.e., declarations), */
+/* so we *copy* them (from pari/paricom.h): */
+extern THREAD VOLATILE int PARI_SIGINT_block, PARI_SIGINT_pending;
+#endif
+
 #include "interrupt/struct_signals.h"
 #include "interrupt/interrupt.h"
 
diff --git a/src/sage/ext/interrupt/interrupt.pxd b/src/sage/ext/interrupt/interrupt.pxd
index 3818447..c1a3b54 100644
--- a/src/sage/ext/interrupt/interrupt.pxd
+++ b/src/sage/ext/interrupt/interrupt.pxd
@@ -1,4 +1,5 @@
 # distutils: depends = INTERRUPT_DEPENDS
+# distutils: libraries = pari
 #
 # NOTE: these functions are actually defined in "macros.h".
 # However, we intentionally do not mention that file here, because
diff --git a/src/sage/ext/interrupt/interrupt.pyx b/src/sage/ext/interrupt/interrupt.pyx
index ed4552a..97dd96d 100644
--- a/src/sage/ext/interrupt/interrupt.pyx
+++ b/src/sage/ext/interrupt/interrupt.pyx
@@ -23,9 +23,6 @@ from libc.signal cimport *
 from libc.stdio cimport freopen, stdin
 from cpython.exc cimport PyErr_Occurred
 
-# Needed for PARI_SIGINT_block in implementation.c:
-cimport sage.libs.pari.paridecl
-
 cdef extern from "interrupt/implementation.c":
     sage_signals_t _signals "_signals"
     void setup_sage_signal_handler() nogil

(preliminary hack)

Probably another distutils: depends for PARI is needed in sage/ext/interrupt/interrupt.pxd.

[nexttime]

comment:8

P.S.: The copied declarations are IMHO unlikely to change.

[nexttime]

comment:9

Replying to @nexttime:

The following works for me to get rid of GMP (i.e., avoids overlinking interrupt.so with libgmp):
...
(preliminary hack)

Probably another distutils: depends for PARI is needed in sage/ext/interrupt/interrupt.pxd.

Anyway, I pushed a slightly cleaner version to u/leif/19610_reviewer-remove_libgmp_from_interrupt.so.

[jdemeyer]

comment:10

Replying to @nexttime:

Maybe I misunderstood the purpose of this ticket

You did.

[jdemeyer]

comment:11

Replying to @nexttime:

Ok, I see, in sage/libs/pari/paridecl.pxd, there's (still)

# distutils: libraries = gmp pari

Do we really need gmp there as well?

Yes, because PARI needs gmp:

$ ldd local/lib/libpari.so 
        linux-vdso.so.1 (0x00007fffaa5fe000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fb572c81000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fb572992000)
        libgmp.so.16 => /usr/local/src/sage-git/local/lib/libgmp.so.16 (0x00007fb57271d000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fb572519000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fb5738fd000)

[nexttime]

comment:12

Replying to @jdemeyer:

Replying to @nexttime:

Ok, I see, in sage/libs/pari/paridecl.pxd, there's (still)

# distutils: libraries = gmp pari

Do we really need gmp there as well?

Yes, because PARI needs gmp:

$ ldd local/lib/libpari.so 
        linux-vdso.so.1 (0x00007fffaa5fe000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fb572c81000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fb572992000)
        libgmp.so.16 => /usr/local/src/sage-git/local/lib/libgmp.so.16 (0x00007fb57271d000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fb572519000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fb5738fd000)

Well, of course PARI needs GMP, but not (necessarily) modules using PARI; that's a transitive dependency. Unless you link statically, you don't have to explicitly specify GMP (and you shouldn't), because the dependency of PARI on GMP is recorded in libpari.so.

If GMP changes, PARI will get rebuilt, and hence also modules depending on PARI.

The reason libgmp was listed twice (and different versions of it!) in your ldd output is exactly such overlinking.

[jdemeyer]

comment:13

Replying to @nexttime:

Well, of course PARI needs GMP, but not (necessarily) modules using PARI; that's a transitive dependency. Unless you link statically, you don't have to explicitly specify GMP (and you shouldn't), because the dependency of PARI on GMP is recorded in libpari.so.

Do you have a reference for this? It's the first time I hear somebody say this.

[vbraun]

comment:14

References:

• https://wiki.openmandriva.org/en/Overlinking_issues_in_packaging
• https://en.altlinux.org/UnderOverLinkProblems

[jdemeyer]

comment:15

The linked documents are only about Linux (thanks for the links though). I'm not at all convinced that the same holds for all operating systems.

[jdemeyer]

comment:16

If we decide to stop "overlinking", that would be a major change in how we build Sage. It is something which should be discussed on sage-devel first. Given that I'm not convinced that there is a problem, I'm not going to write about this.

[jdemeyer]

comment:17

What you are proposing is not portable: see the first few slides of https://people.debian.org/~vorlon/dependency-hell/img0.html

[nexttime]

comment:18

Replying to @jdemeyer:

If we decide to stop "overlinking", that would be a major change in how we build Sage.

No. We've cleaned up module_list.py a couple of times in the past with respect to this.

Note that it's (only) the common case that you don't have to specify libraries used by the libraries you use. (In particular, building the whole Sage library with the linker option --as-needed doesn't work, but would work for most modules.)

The main exceptions are:

• A library is intentionally "underlinked", e.g. to allow the user to choose from different (binary-compatible) alternatives at the final link stage or even at run-time, or the user/application has to provide some symbols the library uses (e.g. memory management functions, error handlers etc.).

• The interface of the library (i.e., the header) exposes symbols of a library it uses, such that your code at least may contain references to the otherwise only indirectly used library. This is in principle the same as if you would explicitly use the other library directly as well, but not always as obvious, and may (or usually does) depend on your code, not necessarily just the header(s) you include. (Stupid example: PARI could have a function pari_foo(x) implemented as a macro or inline function which expands to gmp_bar(x) or of course more complicated code containing calls of GMP functions. Then you'd have to explicitly link with GMP if your code actually uses that macro or inline function. But it's also a poor example, in that for especially PARI/GMP, this is not the case -- AFAIK. A better example is probably MPC/MPFR, but you hardly ever use MPC without also explicitly using MPFR.)

[nexttime]

comment:21

Replying to @jdemeyer:

Replying to @nexttime:

Replying to @jdemeyer:

If we decide to stop "overlinking", that would be a major change in how we build Sage.

No. We've cleaned up module_list.py a couple of times in the past with respect to this.

Are you sure? We've certainly cleaned up module_list.py a couple of times, but I don't think it ever was to remove libraries only appearing as indirect dependencies.

ROFL, e.g. #18777? (The others date back much longer...)

[jdemeyer]

comment:22

Replying to @nexttime:

ROFL, e.g. #18777? (The others date back much longer...)

#18777 removed libraries which were unused by that module. You are here proposing something different: to remove libraries which are used indirectly by a module.

[jdemeyer]

comment:23

Replying to @nexttime:

What exactly do you think is not portable

I doubt that the following statement is true on all operating systems:

Unless you link statically, you don't have to explicitly specify GMP, because the dependency of PARI on GMP is recorded in libpari.so.

[jdemeyer]

comment:24

Anyway, can we please go back to the review of this ticket? If you want to discuss more general linking issues, open a thread on sage-devel.

[nexttime]

comment:25

Replying to @jdemeyer:

If you want to discuss more general linking issues, open a thread on sage-devel.

Well, you asked general questions.

Anyway, can we please go back to the review of this ticket?

interrupt.so still does not need GMP, but with your current patch, GMP gets linked to it. (Worse, not even the Cython code, interrupt.*, where paridecl gets imported, uses any PARI stuff.) I agree that copy-pasting parts of PARI headers (although it's just a single line) is not nice, but that's more a problem of the PARI headers and I was simply getting bored of trying which to include and which not. (Probably the top-level one is ok as well, but seems overkill to me.)

I still don't like the IMHO redundant libraries = pari gmp (in paridecl.pxd), but it's not a major issue either (until we or some people switch to PARI with a different kernel at least).

Perhaps cc some more reviewers, e.g. François.

[nexttime]

comment:26

Replying to @jdemeyer:

Replying to @nexttime:
I doubt that the following statement is true on all operating systems:

Unless you link statically, you don't have to explicitly specify GMP, because the dependency of PARI on GMP is recorded in libpari.so.

There are certainly OSs which don't have shared/dynamic libraries...

But besides that their names/extensions vary, the above holds for all platforms supported by Sage, AFAIK^TM.

[jdemeyer]

comment:27

Replying to @nexttime:

interrupt.so still does not need GMP, but with your current patch, GMP gets linked to it.

It does need GMP since it uses PARI which needs GMP. It's true that the Cython code doesn't use anything of PARI or GMP, but the C code in implementation.c does.

[kiwifb]

comment:28

OK, you wanted me in on this.

fbissey@QCD-nzi3 ~ $ ldd -r /usr/lib64/python2.7/site-packages/sage/ext/interrupt/interrupt.so
        linux-vdso.so.1 (0x00007fffa4fff000)
        libpari-gmp-2.8.so.0 => /usr/lib64/libpari-gmp-2.8.so.0 (0x00007fa7e3b1e000)
        libpython2.7.so.1.0 => /usr/lib64/libpython2.7.so.1.0 (0x00007fa7e375e000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fa7e33c3000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fa7e30bf000)
        libgmp.so.10 => /usr/lib64/libgmp.so.10 (0x00007fa7e2e4e000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fa7e2c49000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fa7e2a2d000)
        libutil.so.1 => /lib64/libutil.so.1 (0x00007fa7e282a000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fa7e4564000)

but

fbissey@QCD-nzi3 ~ $ readelf -d /usr/lib64/python2.7/site-packages/sage/ext/interrupt/interrupt.so

Dynamic section at offset 0x9d28 contains 26 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libpari-gmp-2.8.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libpython2.7.so.1.0]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000c (INIT)               0x2968
 0x000000000000000d (FINI)               0x84b4
 0x0000000000000019 (INIT_ARRAY)         0x209d10
 0x000000000000001b (INIT_ARRAYSZ)       8 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x209d18
 0x000000000000001c (FINI_ARRAYSZ)       8 (bytes)
 0x000000006ffffef5 (GNU_HASH)           0x1c8
 0x0000000000000005 (STRTAB)             0xd90
 0x0000000000000006 (SYMTAB)             0x238
 0x000000000000000a (STRSZ)              1956 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000003 (PLTGOT)             0x20a000
 0x0000000000000002 (PLTRELSZ)           2016 (bytes)
 0x0000000000000014 (PLTREL)             RELA
 0x0000000000000017 (JMPREL)             0x2188
 0x0000000000000007 (RELA)               0x1678
 0x0000000000000008 (RELASZ)             2832 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffe (VERNEED)            0x1628
 0x000000006fffffff (VERNEEDNUM)         1
 0x000000006ffffff0 (VERSYM)             0x1534
 0x000000006ffffff9 (RELACOUNT)          87
 0x0000000000000000 (NULL)               0x0

So sage-on-gentoo is built with -Wl,--as-needed which discard unneeded libraries. Because interrupt.so doesn't directly use gmp it is not kept. For that to work libpari has to be properly linked to libgmp - by that I mean, libgmp has to be in the NEEDED section of libpari.

fbissey@QCD-nzi3 ~ $ readelf -d /usr/lib64/libpari-gmp-2.8.so.0.0.0 

Dynamic section at offset 0x5ca6d8 contains 28 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgmp.so.10]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so.2]
 0x000000000000000e (SONAME)             Library soname: [libpari-gmp-2.8.so.0]
 0x000000000000000c (INIT)               0x69900
 0x000000000000000d (FINI)               0x4fb194
 0x0000000000000019 (INIT_ARRAY)         0x7c98e8
 0x000000000000001b (INIT_ARRAYSZ)       8 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x7c98f0
 0x000000000000001c (FINI_ARRAYSZ)       8 (bytes)
 0x000000006ffffef5 (GNU_HASH)           0x1c8
 0x0000000000000005 (STRTAB)             0x26570
 0x0000000000000006 (SYMTAB)             0x8090
 0x000000000000000a (STRSZ)              57218 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000003 (PLTGOT)             0x7cb000
 0x0000000000000002 (PLTRELSZ)           85704 (bytes)
 0x0000000000000014 (PLTREL)             RELA
 0x0000000000000017 (JMPREL)             0x54a38
 0x0000000000000007 (RELA)               0x36e10
 0x0000000000000008 (RELASZ)             121896 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffe (VERNEED)            0x36d60
 0x000000006fffffff (VERNEEDNUM)         3
 0x000000006ffffff0 (VERSYM)             0x344f2
 0x000000006ffffff9 (RELACOUNT)          3899
 0x0000000000000000 (NULL)               0x0

Equivalently on OS X everything will be fine so long as all the install_names are properly set to the right value.

Yes it will break if your system only use static libraries. Static libraries don't know where to get their other needed bits - unless you bundle them in in the first place.

I don't know if cygwin is properly equipped to deal with something like a needed section or even if it exists on windows (I'll confess ignorance in that matter). I also don't know how things will go on BSD, they are using elf but some bits may depend on the libc implementation and of course ld.

Linking with gmp is not useful on OS X and linux but it won't hurt.

[kiwifb]

comment:29

I should say I haven't read the whole thread, and now that I see more of it. there are some points that can be made:

• it is of course legal to underlink a library
• only executable need to have all their symbols resolved

A case in point is vanilla gsl. libgsl needs a cblas library but it is not linked against cblas implementation (unless you do something). So there is no entry for a cblas in the elf table. When you build an application against libgsl you have to provide a cblas library whether your application use cblas function or not, to resolve the cblas symbols in libgsl.

If libpari was underlinked like that you would have to provide your libgmp/libmpir all the time.

Regarding your slides Gentoo applies "the chasing your tail method" (recompile on soname change). I think sage fits in that category too. I'll note that if you do it correctly a change in soname for libgmp will trigger a rebuild of libpari but not of interrupt.so and it shouldn't have too if libpari didn't change its soname there is no impact on interrupt.so.

If your handling of soname is not done properly or your are overlinking you are exposing yourself to excessive rebuilding. Making sure your handling of soname is right is the hard bit. Only now do we have interesting mechanism that will trigger rebuild on soname change in Gentoo and I won't pretend that every gentoo dev understand them properly.

[kiwifb]

comment:30

One more to be in rant mode: dev that thinks soname and install_name are not important or even ignorant of them should be sent in re-education before touching a library (I am looking at you Fredrick Johansson with your arb package - and now that arb has been made standard it can contribute to the brokenness on OS X 10.11).

[vbraun]

comment:31

Replying to @jdemeyer:

I doubt that the following statement is true on all operating systems:

Unless you link statically, you don't have to explicitly specify GMP, because the dependency of PARI on GMP is recorded in libpari.so.

It is (tautologically) true on every operating system where you can link shared libraries to other shared libraries.

I don't know of an OS where you can't link libraries to libraries, but I'd bet that some obscure platform did that back in the old days. Today certainly not.

[jdemeyer]

comment:32

Replying to @vbraun:

It is (tautologically) true on every operating system where you can link shared libraries to other shared libraries.

I don't understand why this would be tautologically true. It might be true whenever the linker is sufficiently clever, but I don't know if that's the fact on all operating systems. I have particular doubts about Cygwin, because I know that linker command lines often have to be changed for Cygwin.

[jpflori]

comment:34

We should get rid of decl.pxi.
And its several inclusions within other pxd files.

[jdemeyer]

comment:35

Replying to @jpflori:

We should get rid of decl.pxi.
And its several inclusions within other pxd files.

Did you mean to post this on #19590? But yes, of course we should.

[jpflori]

comment:36

Argh yes, I did intend to post to the NTL ticket.

[jpflori]

comment:37

I'll check the recursive dep on Cygwin when I have some time.
For sure on Cygwin you have to explicitely specify every thing that is needed at link time.
I don't remember how it goes when a linked lib itself needs another library but it might be the case you need to specify both in that case...
It sounds crazy but linking on Cygwin is.

[jdemeyer]

comment:38

This ticket has nothing to do with Cygwin, it's only about adding a dependency.

[vbraun]

comment:39

This is getting a bit too long, so let me try to summarize:

Replying to @jdemeyer:

This ticket has nothing to do with Cygwin, it's only about adding a dependency.

But the dependency is wrong (interrupt does not depend on gmp).

Cygwin doesn't really have shared libraries, it uses a static library hack (the Cygwin import library) at link time.

So the real question is should we

• Overlink on all platforms
• Artificially inject all libraries on Cygwin only (via LDFLAGS, say)

[jdemeyer]

comment:40

Replying to @vbraun:

So the real question is should we

• Overlink on all platforms
• Artificially inject all libraries on Cygwin only (via LDFLAGS, say)

Sage has always chosen the first option, so there are probably many overlinked modules in Sage.

So: can we please just focus on the bug that this ticket is about? If you want to discuss overlinking, please do it somewhere else.

[jdemeyer]

comment:41

Let me also summarize: this ticket is about adding a dependency of interrupt.pyx on PARI.

This ticket has absolutely nothing to do with linking and it does not change anything related to linking.

[vbraun]

comment:42

It seems that we have a branch for each approach already so we might just as well merge the one we want to pursue in the future.

Jean-Pierre, do you have any thoughts on Cygwin?

IMHO the "always overlink" approach is simpler and the cost of unnecessarily compiling an extension module is low (its just a single source file, no extra configure and recursive make).

[jdemeyer]

comment:43

Can we please stop this? This has NOTHING to do with linking, why is that so hard to understand?

[jdemeyer]

comment:44

Or maybe I should turn the question around: why do you think that this ticket is somehow related to linking?

[jpflori]

comment:45

I suggest to fix the dependency here and fix the overlinking issue elsewhere (e.g. using leif branch).

As far as Cygwin is concerned, the situation is quite good actually: you just need to have at hand libs directly used by your executable/library and feed them to the linker with appropriate flags as I just tested on a VM.
If these libraries need further libraries then everything is resolved at run time.
So in the linking ticket, we can drop the gmp library.

The main issue with cygwin is that the need above is mandatory: if you wanna use a gmp function (whatever it relies on, even further libraries) then you need to tell it to the linker whereas linux is more negligent about that and just bet that at runtime every undefined symbol will be resolved.
And many times people modified a random Sage cython file, use some gmp function (typically mpn_popcount don't ask me why :)) and forgott to make gmp a dependency.
Nowadays, thanks to Jeroen work, it should be less an issue as when people want to use mpn_popcount they have to cimport it and it will automagically tell the linker that gmp has to be linked in.

[jpflori]

Reviewer: Jean-Pierre Flori

[vbraun]

Changed branch from u/jdemeyer/fix_dependencies_on_pari to 5d0e4d3