Opened 3 years ago

Closed 3 years ago

#24705 closed defect (wontfix)

Do not hardcode the C++ stdlib in setup.py

Reported by: rws Owned by:
Priority: blocker Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: fbissey, SimonKing, jdemeyer Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/do_not_hardcode_the_c___stdlib_in_setup_py (Commits, GitHub, GitLab) Commit: 0e4016e97ff33a4ca0197e0242716c420a24685b
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

sage build with the following flags still uses libstdc++ for some cythonized so files:

export CC="clang"
export CXX="clang++ --stdlib=libc++"
export CXXFLAGS="$CXXFLAGS --stdlib=libc++"
export LDFLAGS="$LDFLAGS -lc++ --stdlib=libc++"
> ldd /home/ralf/sage/local/lib/python2.7/site-packages/sage/libs/ntl/ntl_ZZ.so
	linux-vdso.so.1 (0x00007ffc8f3f1000)
	libntl.so.33 => /home/ralf/sage/local/lib/libntl.so.33 (0x00007f823f6d7000)
	libgmp.so.23 => /home/ralf/sage/local/lib/libgmp.so.23 (0x00007f823f448000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f823f14b000)
	libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x00007f823edc2000)
	libpython2.7.so.1.0 => /home/ralf/sage/local/lib/libpython2.7.so.1.0 (0x00007f823e9d8000)
	libpari-gmp-2.10.so.0 => /home/ralf/sage/local/lib/libpari-gmp-2.10.so.0 (0x00007f823def9000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f823dce2000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f823dac5000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f823d724000)
	libgf2x.so.1 => /home/ralf/sage/local/lib/libgf2x.so.1 (0x00007f823d501000)
	libc++.so.1 => /usr/lib64/libc++.so.1 (0x00007f823ff05000)
	libc++abi.so.1 => /usr/lib64/libc++abi.so.1 (0x00007f823fec2000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f823fdd6000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f823d2fd000)
	libutil.so.1 => /lib64/libutil.so.1 (0x00007f823d0fa000)

Attachments (3)

ntl-10.3.0.log (114.7 KB) - added by rws 3 years ago.
sagelib-8.2.beta5.log (914.3 KB) - added by SimonKing 3 years ago.
giac-1.4.9.45.p1.log (22.6 KB) - added by SimonKing 3 years ago.

Download all attachments as: .zip

Change History (36)

Changed 3 years ago by rws

comment:1 Changed 3 years ago by rws

sagelib is the culprit. It has libstdc++ hardcoded:

../logs/pkgs/sagelib-8.2.beta5.log:[212/474] [211/474] clang++ -pthread -shared -L/home/ralf/sage/local/lib -Wl,-rpath,/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -Wl,-rpath,/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib build/temp.linux-x86_64-2.7/build/cythonized/sage/libs/ntl/ntl_ZZ_pX.o -L/home/ralf/sage/local/lib -L/home/ralf/sage/local/lib -lntl -lgmp -lm -lstdc++ -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/libs/ntl/ntl_ZZ_pX.so -lpari

comment:2 Changed 3 years ago by rws

  • Summary changed from NTL build with --stdlib=libc++ still uses libstdc++ for ntl_ZZ.so to sagelib build with --stdlib=libc++ still uses libstdc++ for ntl_ZZ.so

comment:3 follow-up: Changed 3 years ago by rws

  • Description modified (diff)
  • Summary changed from sagelib build with --stdlib=libc++ still uses libstdc++ for ntl_ZZ.so to Do not hardcode the C++ stdlib in setup.py

This is the relevant part of src/setup.py:

        # Libraries: add stdc++ if needed and sort them
        libs = kwds.get('libraries', [])
        if cplusplus:
            libs = libs + ['stdc++']

comment:4 follow-up: Changed 3 years ago by rws

It seems best to add "--stdlib=libc++"(else the libstdc++ is linked) only if it is already in CFLAGS. See also #24705.

Testing this manually works, except for element_givaro.pyx which still gets compiled without the "--stdlib=libc++" directive.

comment:5 Changed 3 years ago by rws

Never mind, I got it compiled, yeah, now testing.

comment:6 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:7 Changed 3 years ago by SimonKing

  • Cc SimonKing added

comment:8 in reply to: ↑ 4 Changed 3 years ago by SimonKing

Replying to rws:

It seems best to add "--stdlib=libc++"(else the libstdc++ is linked) only if it is already in CFLAGS. See also #24705.

Do you mean #24707?

comment:9 Changed 3 years ago by jdemeyer

  • Cc jdemeyer added
  • Component changed from packages: standard to build

comment:10 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by SimonKing

Replying to rws:

This is the relevant part of src/setup.py:

        # Libraries: add stdc++ if needed and sort them
        libs = kwds.get('libraries', [])
        if cplusplus:
            libs = libs + ['stdc++']

Sorry, I don't get it: What would you replace it with, so that the sage library will not use libstdc++ if --stdlib=libc++ is provided?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by rws

Replying to SimonKing:

Replying to rws:

This is the relevant part of src/setup.py:

        # Libraries: add stdc++ if needed and sort them
        libs = kwds.get('libraries', [])
        if cplusplus:
            libs = libs + ['stdc++']

Sorry, I don't get it: What would you replace it with, so that the sage library will not use libstdc++ if --stdlib=libc++ is provided?

At the moment we're still experimenting with flags, see https://trac.sagemath.org/ticket/24707#comment:9. But I already think that the line above needs to be changed to libs = libs + ['c++'] in order to get -lc++ in the link command.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by SimonKing

Replying to rws:

At the moment we're still experimenting with flags, see https://trac.sagemath.org/ticket/24707#comment:9. But I already think that the line above needs to be changed to libs = libs + ['c++'] in order to get -lc++ in the link command.

But certainly not unconditionally. Or do we want to build with -lc++ when using gcc?

comment:13 in reply to: ↑ 12 Changed 3 years ago by rws

Replying to SimonKing:

But certainly not unconditionally. Or do we want to build with -lc++ when using gcc?

Of course (EDIT: not).

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

comment:14 Changed 3 years ago by rws

  • Branch set to u/rws/do_not_hardcode_the_c___stdlib_in_setup_py

comment:15 Changed 3 years ago by rws

  • Commit set to 0e4016e97ff33a4ca0197e0242716c420a24685b

This still doesn't catch all ways to smuggle -lstdc++ in the cython file linking stage of sagelib.


New commits:

0e4016e24705: Do not hardcode the C++ stdlib in setup.py

comment:16 Changed 3 years ago by rws

  • Description modified (diff)

Changed 3 years ago by SimonKing

comment:17 Changed 3 years ago by SimonKing

I did

export CC="clang"
export CXX="clang++ --stdlib=libc++
export CXXFLAGS="$CXXFLAGS -I/usr/include/libcxxabi/ --stdlib=libc++"
export LDFLAGS="$LDFLAGS -lc++ --stdlib=libc++"
make build

The build process goes pretty far, however it hangs (for an hour or so) in sagelib. See the attached log.

comment:18 Changed 3 years ago by fbissey

I am a bit late to this party - sorry. I don't have this problem at all with my clang configured to use libc++ by default. Is it possible to have readelf -d /home/ralf/sage/local/lib/python2.7/site-packages/sage/libs/ntl/ntl_ZZ.so on the object initially posted?

comment:19 follow-up: Changed 3 years ago by rws

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

So this ticket is wontfix. Use these flags:

export CC="clang"
export CXX="clang++"
export CLANG_DEFAULT_CXX_STDLIB="libc++"

comment:20 Changed 3 years ago by rws

  • Status changed from needs_review to positive_review

comment:21 in reply to: ↑ 19 ; follow-up: Changed 3 years ago by SimonKing

  • Status changed from positive_review to needs_info

Replying to rws:

So this ticket is wontfix. Use these flags:

export CC="clang"
export CXX="clang++"
export CLANG_DEFAULT_CXX_STDLIB="libc++"

Do you claim that this is enough to override what is written in setup.py???

Recall that Sage's setup.py forces usage of libstdc++ and not libc++.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 3 years ago by SimonKing

Replying to SimonKing:

Do you claim that this is enough to override what is written in setup.py???

Please let me first test whether the given flags are enough to let the branch from #24710 build on ubuntu. Or do you even claim that with your flags scipy will build without modifications?

comment:23 Changed 3 years ago by fbissey

setup.py adds -lstdc++ to the flags used for linking but that can still be discarded by the compiler driver used. I can build sage with clang without altering setup.py so you should be able to. I am glad that Ralph found that environment variable.

comment:24 in reply to: ↑ 22 Changed 3 years ago by rws

Replying to SimonKing:

Replying to SimonKing:

Do you claim that this is enough to override what is written in setup.py???

Please let me first test whether the given flags are enough to let the branch from #24710 build on ubuntu. Or do you even claim that with your flags scipy will build without modifications?

I do not make such claims without testing first.

Changed 3 years ago by SimonKing

comment:25 follow-up: Changed 3 years ago by SimonKing

I just tried with the branch from #24710 and with the flags stated in comment:19. But giac fails to build, see attachment:giac-1.4.9.45.p1.log

comment:26 in reply to: ↑ 25 ; follow-up: Changed 3 years ago by fbissey

Replying to SimonKing:

I just tried with the branch from #24710 and with the flags stated in comment:19. But giac fails to build, see attachment:giac-1.4.9.45.p1.log

It looks like you need #24696

comment:27 in reply to: ↑ 26 Changed 3 years ago by SimonKing

Replying to fbissey:

Replying to SimonKing:

I just tried with the branch from #24710 and with the flags stated in comment:19. But giac fails to build, see attachment:giac-1.4.9.45.p1.log

It looks like you need #24696

OK, trying again with #24710 and #24696

comment:28 Changed 3 years ago by SimonKing

  • Status changed from needs_info to positive_review

Hooray, it builds now! I didn't run tests though. Anyway, to summarize what I did on ubuntu:

  • install clang, clang-dev, libc++abi-dev
  • checkout/merge #24710 and #24696
  • export CC="clang"
    export CXX="clang++"
    export CLANG_DEFAULT_CXX_STDLIB="libc++"
    
  • make

So, your finding out about CLANG_DEFAULT_CXX_STDLIB was great!

Side-remarks:

  • I did not have the impression that building Sage with clang was faster than with gcc.
  • I will now do some small timings to see about the performance of the code generated by clang vs. gcc.
Last edited 3 years ago by SimonKing (previous) (diff)

comment:29 Changed 3 years ago by SimonKing

I just realise that using #24710 was supposed to be not needed (it was closed as invalid).

Anyway, a data point concerning timings, after installing meataxe:

  • With gcc
    sage: M = random_matrix(GF(9,'x'), 5000)
    sage: type(M)
    <type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
    sage: N = random_matrix(GF(9,'x'), 5000)
    sage: %timeit _=M-N
    10 loops, best of 3: 20.5 ms per loop
    sage: %timeit _=M*N
    1 loop, best of 3: 46.5 s per loop
    
  • With clang:
    sage: M = random_matrix(GF(9,'x'), 5000)
    sage: type(M)
    <type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
    sage: N = random_matrix(GF(9,'x'), 5000)
    sage: %timeit _=M-N
    10 loops, best of 3: 20.5 ms per loop
    sage: %timeit _=M*N
    1 loop, best of 3: 53.2 s per loop
    

Since meataxe is heavily used in the computations that I really care about (group cohomology), I am not so happy about a slow-down of almost 15%. So, probably I won't use clang in my computations. Nonetheless, it is good for portability that clang can also be used on ubuntu.

comment:30 Changed 3 years ago by SimonKing

I get several segfaults when testing sage.homology. Is that to be expected?

comment:31 follow-up: Changed 3 years ago by jdemeyer

Should this ticket be closed?

comment:32 in reply to: ↑ 31 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Should this ticket be closed?

I think so. Certainly the test failure should be for a different ticket.

comment:33 Changed 3 years ago by jdemeyer

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.