Opened 6 years ago
Closed 6 years ago
#20646 closed defect (fixed)
gsl and linbox underlinked in sage-7.2
Reported by: | mjo | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | build | Keywords: | |
Cc: | Merged in: | ||
Authors: | François Bissey | Reviewers: | Steven Trogdon, Michael Orlitzky |
Report Upstream: | N/A | Work issues: | |
Branch: | 0486c05 (Commits, GitHub, GitLab) | Commit: | 0486c05e0d501e2428493c7db4314df26625f200 |
Dependencies: | Stopgaps: |
Description (last modified by )
When building sage-7.2 with LDFLAGS="--as-needed"
and the gold linker, the result is underlinked:
from sage.rings.complex_double import CDF ImportError: /home/mjo/src/sage.git/local/lib/libgsl.so.19: undefined symbol: cblas_sdsdot
This is because libgsl.so
itself is underlinked as per upstream design. This doesn't work well when building a python loadable module which needs all its symbols (apart from those provided by python) to be resolved.
The gold linker in particular as a very strict interpretation of --as-needed
and will not add library not needed directly by the object being linked. In this particular case sage.rings.complex_double needs gsl but doesn't use cblas and therefore cblas is not added to the list of needed libraries. libgsl.so itself need to import cblas.
The current branch provides a minimal non invasive fix to link libgsl.so to the current cblas.
Note, that gsl already depends on $(BLAS)
even so prior to this ticket it was not used.
Change History (44)
comment:1 follow-ups: ↓ 2 ↓ 4 Changed 6 years ago by
comment:2 in reply to: ↑ 1 Changed 6 years ago by
Replying to leif:
So should we add
-liml
in LinBox'sspkg-install
?Or patch LinBox in that way?
FWIW, Sage-on-Gentoo (see:https://github.com/cschwan/sage-on-gentoo/blob/master/sci-libs/linbox/linbox-1.3.2-r2.ebuild
) does this by appending -liml
to the LIBS
variable and not by patching.
comment:3 Changed 6 years ago by
Third option:
Add
# distutils: libraries = iml
to src/sage/libs/linbox/linbox.pxd
.
comment:4 in reply to: ↑ 1 Changed 6 years ago by
Replying to leif:
So should we add
-liml
in LinBox'sspkg-install
?
That's what I've been doing. But I noticed that the newer version of linbox (https://github.com/cschwan/sage-on-gentoo/tree/master/sci-libs/linbox) doesn't have the same "append-libs" hack, so if it's easier, an upgrade to linbox-1.4.1 probably fixes it as well.
comment:5 follow-up: ↓ 6 Changed 6 years ago by
On the one hand, it looks like updating linbox will NOT be easier (see ticket #17635); on the other, it looks like a lot of the work has already been done.
comment:6 in reply to: ↑ 5 Changed 6 years ago by
comment:7 Changed 6 years ago by
#17635 is basically ready, modulo a rebasing and fixing minor platforms (32bit). I'd like to add that libgsl
is underlinked upstream on purpose. In their opinion it is the responsibility of people downstream or writing their applications to provide a cblas
.
It is our choice to provide extra linking. In that context I am not sure the gentoo patch as it is the way to go.
The people in the gentoo science overlay (including me) have not drunk the whole Kool-aid of their blas infrastructure when it comes to gsl
. The reality is gsl
should be split in gslcblas
and libgsl
. Then libgsl
could just use pkg-config
to find cblas
. You still need to remove libgslcblas.la
everywhere but you avoid adding a custom macro.
comment:8 Changed 6 years ago by
Is it still relevant now that the new linbox
stack has been merged.
comment:9 Changed 6 years ago by
I'm now unable to reproduce any of the issues associated with this ticket. Things implemented:
- export export LDFLAGS="-Wl,--as-needed"
- rebuild (sage -f)
sagelib, gsl, linbox
(gsl
was probably not necessary)
sage: from sage.rings.complex_double import CDF
does not fail and rebuilding linbox
also rebuilds iml
and fflas_ffpack
. I don't know if using atlas
will be a problem.
comment:10 Changed 6 years ago by
The linbox issue should be gone, but I don't know about gsl. I guess I'll have to recompile from scratch.
comment:11 follow-up: ↓ 12 Changed 6 years ago by
Well libgsl.so
is still underlinked as upstream intended, but anything linking to libgsl should also have been properly linked to a cblas library.
comment:12 in reply to: ↑ 11 Changed 6 years ago by
Replying to fbissey:
Well
libgsl.so
is still underlinked as upstream intended, but anything linking to libgsl should also have been properly linked to a cblas library.
Yep, this is what I see. When there is linking with libgsl.so
there also appears linking to openblas
. Something like -lgsl -lm -lopenblas
. And when atlas
was used in say, 7.4.beta4
the linking would typically appear as -lgsl ... -lm -latlas -lcblas
. I suspect everything is now OK.
comment:13 Changed 6 years ago by
- Milestone changed from sage-7.3 to sage-duplicate/invalid/wontfix
- Status changed from new to needs_review
Yup, it's fixed. Thanks for all your work on this stuff.
comment:14 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-7.6
- Status changed from positive_review to needs_work
Oops, I just tried to run the sage that I built 5 days ago:
from sage.misc.all import * # takes a while File "/home/mjo/src/sage.git/local/lib/python2.7/site-packages/sage/misc/all.py", line 89, in <module> from .functional import (additive_order, File "/home/mjo/src/sage.git/local/lib/python2.7/site-packages/sage/misc/functional.py", line 32, in <module> from sage.rings.complex_double import CDF ImportError: /home/mjo/src/sage.git/local/lib/libgsl.so.19: undefined symbol: cblas_sdsdot
comment:16 follow-up: ↓ 17 Changed 6 years ago by
That's interesting. Can you post the output of
readelf -d /home/mjo/src/sage.git/local/lib/python2.7/site-packages/sage/rings/complex_double.so
comment:17 in reply to: ↑ 16 Changed 6 years ago by
Replying to fbissey:
That's interesting. Can you post the output of
readelf -d /home/mjo/src/sage.git/local/lib/python2.7/site-packages/sage/rings/complex_double.so
Sure:
Dynamic section at offset 0x48200 contains 34 entries: Tag Type Name/Value 0x0000000000000003 (PLTGOT) 0x49660 0x0000000000000002 (PLTRELSZ) 3672 (bytes) 0x0000000000000017 (JMPREL) 0x7aa0 0x0000000000000014 (PLTREL) RELA 0x0000000000000007 (RELA) 0x2d60 0x0000000000000008 (RELASZ) 19776 (bytes) 0x0000000000000009 (RELAENT) 24 (bytes) 0x000000006ffffff9 (RELACOUNT) 762 0x0000000000000006 (SYMTAB) 0x190 0x000000000000000b (SYMENT) 24 (bytes) 0x0000000000000005 (STRTAB) 0x15a0 0x000000000000000a (STRSZ) 5292 (bytes) 0x000000006ffffef5 (GNU_HASH) 0x2a50 0x0000000000000001 (NEEDED) Shared library: [libgsl.so.19] 0x0000000000000001 (NEEDED) Shared library: [libpari-gmp-2.8.so.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libpython2.7.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libpthread.so.0] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x000000000000000c (INIT) 0x88f8 0x000000000000000d (FINI) 0x40af0 0x000000000000001a (FINI_ARRAY) 0x5b4c8 0x000000000000001c (FINI_ARRAYSZ) 8 (bytes) 0x0000000000000019 (INIT_ARRAY) 0x5b4d0 0x000000000000001b (INIT_ARRAYSZ) 8 (bytes) 0x000000000000001d (RUNPATH) Library runpath: [/home/mjo/src/sage.git/local/lib] 0x000000000000001e (FLAGS) BIND_NOW 0x000000006ffffffb (FLAGS_1) Flags: NOW 0x000000006ffffff0 (VERSYM) 0x2b44 0x000000006ffffffc (VERDEF) 0x2cf0 0x000000006ffffffd (VERDEFNUM) 1 0x000000006ffffffe (VERNEED) 0x2d0c 0x000000006fffffff (VERNEEDNUM) 2 0x0000000000000000 (NULL) 0x0
comment:18 Changed 6 years ago by
Interesting my install in sage-on-gentoo shows
readelf -d /usr/lib64/python2.7/site-packages/sage/rings/complex_double.so Dynamic section at offset 0x46c00 contains 29 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libgsl.so.19] 0x0000000000000001 (NEEDED) Shared library: [libpari-gmp-2.8.so.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libopenblas_threads.so.0] 0x0000000000000001 (NEEDED) Shared library: [libpython2.7.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
So I have openblas here. It could be a library order problem. When complex_double.so
is linked, is -lopenblas
before -lgsl
? Since this is a shared library it is legal to underlink. May be you should try a build with -Wl,--no-undefined
to catch funny stuff.
comment:19 Changed 6 years ago by
Hum my own log for vanilla 7.4 show that the order is correct at least here
gcc -pthread -shared -L/home/fbissey/sandbox/git-fork/sage-7.4/local/lib -Wl,-rpath,/home/fbissey/sandbox/git-fork/sage-7.4/local/lib -L/home/fbissey/sandbox/git-fork/sage-7.4/local/lib -Wl,-rpath,/home/fbissey/sandbox/git-fork/sage-7.4/local/lib build/temp.linux-x86_64-2.7/home/fbissey/sandbox/git-fork/sage-7.4/src/build/cythonized/sage/rings/complex_double.o -L/home/fbissey/sandbox/git-fork/sage-7.4/local/lib -L/home/fbissey/sandbox/git-fork/sage-7.4/local/lib -lgsl -lpari -lgmp -lm -lopenblas -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/rings/complex_double.so -lpari -Ddummy
comment:20 Changed 6 years ago by
This is what I've got in my sagelib log:
gcc -pthread -shared -L/home/mjo/src/sage.git/local/lib -Wl,-rpath,/home/mjo/src/sage.git/local/lib -Wl,-O1 -Wl,--as-needed -L/home/mjo/src/sage.git/local/lib -Wl,-rpath,/home/mjo/src/sage.git/local/lib -Wl,-O1 -Wl,--as-needed -march=native -O2 -pipe build/temp.linux-x86_64-2.7/home/mjo/src/sage.git/src/build/cythonized/sage/rings/complex_double.o -L/home/mjo/src/sage.git/local/lib -L/home/mjo/src/sage.git/local/lib -lgsl -lpari -lgmp -lm -lopenblas -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/rings/complex_double.so -lpari -Ddummy
The difference that stands out to me is --as-needed
... won't it refuse to link OpenBLAS into the result if complex_double.so
fails to use its symbols directly?
comment:21 Changed 6 years ago by
I have
gcc -pthread -shared -L/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-rpath,/64bitdev/storage/sage-git_develop/sage/local/lib -L/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,-rpath,/64bitdev/storage/sage-git_develop/sage/local/lib -Wl,--as-needed build/temp.linux-x86_64-2.7/64bitdev/storage/sage-git_develop/sage/src/build/cythonized/sage/rings/complex_double.o -L/64bitdev/storage/sage-git_develop/sage/local/lib -L/64bitdev/storage/sage-git_develop/sage/local/lib -lgsl -lpari -lgmp -lm -lopenblas -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/rings/complex_double.so -lpari -Ddummy
but shouldn't the linking be with g++
?
comment:22 follow-up: ↓ 24 Changed 6 years ago by
I'm also using gold instead of the default BFD linker, if that turns out to matter. On gentoo the switch amounts to
$ sudo binutils-config --linker ld.gold
comment:23 Changed 6 years ago by
Rebuilt sagelib
here using LDFLAGS=-Wl,-O1 -Wl,--as-needed
instead of LDFLAGS=-Wl,--as-needed
, just in case; still no issue. I am using gentoo default BFD linker.
comment:24 in reply to: ↑ 22 Changed 6 years ago by
Replying to mjo:
I'm also using gold instead of the default BFD linker, if that turns out to matter. On gentoo the switch amounts to
$ sudo binutils-config --linker ld.gold
It usually does with matter regarding --as-needed
in my experience.
comment:25 follow-up: ↓ 26 Changed 6 years ago by
Somehow I had another comment that escaped posting :( The gold linker has a more strict interpretation of --as-needed
and in this case libgsl
would need to be linked to cblas
itself.
It could be interesting to try adding -Wl,--no-allow-shlib-undefined
as an additional LDFLAGS
.
comment:26 in reply to: ↑ 25 Changed 6 years ago by
Replying to fbissey:
It could be interesting to try adding
-Wl,--no-allow-shlib-undefined
as an additionalLDFLAGS
.
I didn't notice anything different, with a clean build of 7.5.beta1, it dies when it tries to build the docs at the end of compilation:
$ LDFLAGS="-Wl,-O1 -Wl,--as-needed -Wl,--no-allow-shlib-undefined" CFLAGS="${CFLAGS}" make -j4 ... [conway_polynomials-0.4.p0] Finished installing conway_polynomials-0.4.p0.spkg cd ../.. && sage-logger -p './sage --docbuild --no-pdf-links all html ' logs/dochtml.log [dochtml] /home/mjo/src/sage.git/local/bin/python: /home/mjo/src/sage.git/local/lib/libgsl.so.19: undefined symbol: cblas_sdsdot; 'sage_setup.docbuild' is a package and cannot be directly executed Makefile:1063: recipe for target 'doc-html' failed
I don't see anything unusual in that same spot in the sagelib build log either, but I don't really know what I'm looking for. I think the additional linker flag should have made it fail?
comment:27 follow-ups: ↓ 32 ↓ 33 Changed 6 years ago by
Failure to link would have been my minimal expectation.
So the option we have:
- link
libgsl.so
to the selected cblas (openblas or atlas) when it is built. - add a
-Wl,--no-as-needed
flag to all files linking tolibgsl.so
in the sage library.
The first one may be achieved by adding
LIBS=`pkg-config --libs-only-l cblas`
to the configure line of gsl
's spkg-install. I think.
The second one may be achievable by alteration of gsl.pc
by manually adding the flag we want. Possibly by patching gsl.pc.in
before building gsl
.
Pick your poison.
comment:28 Changed 6 years ago by
Hum, $(BLAS)
is currently in gsl
's dependency but is not in fact used.
comment:29 follow-up: ↓ 30 Changed 6 years ago by
./configure --prefix="$SAGE_LOCAL" --libdir="$SAGE_LOCAL/lib" \ LIBS="`pkg-config --libs-only-l cblas` -lm"
-lm
is needed for some reason. Possibly setting LIBS
interferes with it being added, libopenblas.so
is certainly properly linked to it.
comment:30 in reply to: ↑ 29 Changed 6 years ago by
Replying to fbissey:
./configure --prefix="$SAGE_LOCAL" --libdir="$SAGE_LOCAL/lib" \ LIBS="`pkg-config --libs-only-l cblas` -lm"
-lm
is needed for some reason. Possibly settingLIBS
interferes with it being added,libopenblas.so
is certainly properly linked to it.
Do you think the order of where -lm
appears is important? I always see -lgsl ... -lm -lopenblas ...
in the sagelib
log. I'm not sure where the order is set.
comment:31 Changed 6 years ago by
I don't think the order is important in that case, because openblas doesn't need it according to ldd and readelf. It certainly would be important if libopenblas wasn't linked to libm. Then it would have to be after. And even so, with gold, if the stuff you link doesn't use libm directly, it won't appear in the final shared object and you will be in the same situation as with gsl needing cblas.
comment:32 in reply to: ↑ 27 Changed 6 years ago by
Replying to fbissey:
Failure to link would have been my minimal expectation.
So the option we have:
- link
libgsl.so
to the selected cblas (openblas or atlas) when it is built.- add a
-Wl,--no-as-needed
flag to all files linking tolibgsl.so
in the sage library.The first one may be achieved by adding
LIBS=`pkg-config --libs-only-l cblas`to the configure line of
gsl
's spkg-install. I think.The second one may be achievable by alteration of
gsl.pc
by manually adding the flag we want. Possibly by patchinggsl.pc.in
before buildinggsl
.Pick your poison.
It seems to me that altering the gsl.pc
file is like a SoG build with USE=cblas-external
. And this approach will require a change in the configure of gsl
resulting in increased package maintenance on the Sage end. But doing it would remove undefined cblas
symbols in libgsl
.
comment:33 in reply to: ↑ 27 Changed 6 years ago by
Replying to fbissey:
Failure to link would have been my minimal expectation.
So the option we have:
- link
libgsl.so
to the selected cblas (openblas or atlas) when it is built.
When I first encountered this, I thought the fact that GSL wasn't linked to CBLAS was a bug, but then you pointed out that upstream leaves it that way on purpose. What's the benefit of doing so in Sage, where we know that everything is going to transitively link to the same CBLAS anyway?
Requiring consumers to fill in your missing symbols still seems backwards to me, but I don't want to judge it wrong too quickly.
comment:34 follow-up: ↓ 35 Changed 6 years ago by
@ Steve adding -lopenblas won't work. It is not the only thing we do.
@ mjo, underlinking is legal but no distro leave it way I think. The wheels come of for shared objects such as those used by python, bfd linker does ok but gold doesn't want to cater to that case I think. Not their goal. I'd rather link gsl in sage, the dependency is there already.
Send from my iPhone from school camp😀
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 6 years ago by
Replying to fbissey:
I'd rather link gsl in sage, the dependency is there already.
If there are no hidden pitfalls, then this is simple and unobtrusive and has my vote.
comment:36 in reply to: ↑ 35 Changed 6 years ago by
comment:37 follow-up: ↓ 38 Changed 6 years ago by
The way I outlined is indeed simple and not intrusive. Downside, libgslcblas.so
is probably unusable. Not sure we care that much, but there should be a warning somewhere.
readelf -d /home/fbissey/sandbox/git-fork/sage-7.4/local/lib/libgslcblas.so Dynamic section at offset 0x3ddd0 contains 28 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libopenblas.so.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x000000000000000e (SONAME) Library soname: [libgslcblas.so.0] 0x000000000000001d (RUNPATH) Library runpath: [/home/fbissey/sandbox/git-fork/sage-7.4/local/lib]
In concrete terms I am not sure which cblas you are getting when linking with it.
comment:38 in reply to: ↑ 37 Changed 6 years ago by
Replying to fbissey:
The way I outlined is indeed simple and not intrusive. Downside,
libgslcblas.so
is probably unusable. Not sure we care that much, but there should be a warning somewhere.readelf -d /home/fbissey/sandbox/git-fork/sage-7.4/local/lib/libgslcblas.so Dynamic section at offset 0x3ddd0 contains 28 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libopenblas.so.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x000000000000000e (SONAME) Library soname: [libgslcblas.so.0] 0x000000000000001d (RUNPATH) Library runpath: [/home/fbissey/sandbox/git-fork/sage-7.4/local/lib]In concrete terms I am not sure which cblas you are getting when linking with it.
With my default BFD linker and extra linker flags -Wl,-O1 -Wl,--as-needed
I have no linking to cblas
.
Dynamic section at offset 0x3cde0 contains 27 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x000000000000000e (SONAME) Library soname: [libgslcblas.so.0] 0x000000000000001d (RUNPATH) Library runpath: [/64bitdev/storage/sage-git_develop/sage/local/lib]
comment:39 Changed 6 years ago by
Probably a bad manipulation on my part then. It will be present for people not using --as-needed
though. I'll post a branch later. I may also revamp the description a bit.
comment:40 Changed 6 years ago by
- Branch set to u/fbissey/gsl_cblas_link
- Commit set to 0486c05e0d501e2428493c7db4314df26625f200
- Description modified (diff)
- Milestone changed from sage-7.6 to sage-7.5
- Status changed from needs_work to needs_review
At last someone can review this.
New commits:
0486c05 | Make ligsl link to the current cblas library instead of leaving it underlinked.
|
comment:41 Changed 6 years ago by
- Reviewers set to Steven Trogdon
I think this fixes things
readelf -d local/lib/libgsl.so Dynamic section at offset 0x23ab08 contains 28 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libopenblas.so.0] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x000000000000000e (SONAME) Library soname: [libgsl.so.19] 0x000000000000001d (RUNPATH) Library runpath: [/64bitdev/storage/sage-git_develop/sage/local/lib]
with --as-needed
added to LDFLAGS
. But I was wrong before, so @mjo (Michael) should probably push the button since I don't use the offending gold
linker.
comment:42 Changed 6 years ago by
- Status changed from needs_review to positive_review
Looks good, and I even remembered to run it this time. Thanks!
comment:43 Changed 6 years ago by
- Reviewers changed from Steven Trogdon to Steven Trogdon, Michael Orlitzky
comment:44 Changed 6 years ago by
- Branch changed from u/fbissey/gsl_cblas_link to 0486c05e0d501e2428493c7db4314df26625f200
- Resolution set to fixed
- Status changed from positive_review to closed
So should we add
-liml
in LinBox'sspkg-install
?Or patch LinBox in that way?