Opened 6 years ago

Closed 5 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:

Status badges

Description (last modified by fbissey)

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: Changed 5 years ago by leif

So should we add -liml in LinBox's spkg-install?

Or patch LinBox in that way?

comment:2 in reply to: ↑ 1 Changed 5 years ago by strogdon

Replying to leif:

So should we add -liml in LinBox's spkg-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 5 years ago by leif

Third option:

Add

# distutils: libraries = iml

to src/sage/libs/linbox/linbox.pxd.

comment:4 in reply to: ↑ 1 Changed 5 years ago by mjo

Replying to leif:

So should we add -liml in LinBox's spkg-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: Changed 5 years ago by mjo

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 5 years ago by strogdon

Replying to mjo:

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.

I was just going to say that.

comment:7 Changed 5 years ago by fbissey

#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 5 years ago by fbissey

Is it still relevant now that the new linbox stack has been merged.

comment:9 Changed 5 years ago by strogdon

I'm now unable to reproduce any of the issues associated with this ticket. Things implemented:

  • export export LDFLAGS="-W1,--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.

Version 0, edited 5 years ago by strogdon (next)

comment:10 Changed 5 years ago by mjo

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: Changed 5 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 years ago by strogdon

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 5 years ago by mjo

  • 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 5 years ago by mjo

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by mjo

  • 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: Changed 5 years ago by 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

comment:17 in reply to: ↑ 16 Changed 5 years ago by mjo

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 5 years ago by fbissey

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 5 years ago by fbissey

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 5 years ago by mjo

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 5 years ago by strogdon

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: Changed 5 years ago by 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

comment:23 Changed 5 years ago by strogdon

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 5 years ago by fbissey

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: Changed 5 years ago by fbissey

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 5 years ago by mjo

Replying to fbissey:

It could be interesting to try adding -Wl,--no-allow-shlib-undefined as an additional LDFLAGS.

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: Changed 5 years ago by 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 to libgsl.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 5 years ago by fbissey

Hum, $(BLAS) is currently in gsl's dependency but is not in fact used.

comment:29 follow-up: Changed 5 years ago by fbissey

./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 5 years ago by strogdon

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 setting LIBS 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 5 years ago by fbissey

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 5 years ago by strogdon

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 to libgsl.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.

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 5 years ago by mjo

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: Changed 5 years ago by fbissey

@ 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: Changed 5 years ago by mjo

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 5 years ago by strogdon

Replying to mjo:

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.

ditto

or in common parlance

+1

comment:37 follow-up: Changed 5 years ago by 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.

comment:38 in reply to: ↑ 37 Changed 5 years ago by strogdon

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 5 years ago by fbissey

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 5 years ago by fbissey

  • Authors set to François Bissey
  • 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:

0486c05Make ligsl link to the current cblas library instead of leaving it underlinked.

comment:41 Changed 5 years ago by strogdon

  • 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 5 years ago by mjo

  • Status changed from needs_review to positive_review

Looks good, and I even remembered to run it this time. Thanks!

comment:43 Changed 5 years ago by mjo

  • Reviewers changed from Steven Trogdon to Steven Trogdon, Michael Orlitzky

comment:44 Changed 5 years ago by vbraun

  • Branch changed from u/fbissey/gsl_cblas_link to 0486c05e0d501e2428493c7db4314df26625f200
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.