sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 478 forks source link

configure threading in flint in sync with NTL #27764

Open dimpase opened 5 years ago

dimpase commented 5 years ago

By default, TLS (aka Thread-Local Storage) is enabled in flint. However, it contradicts to NTL configured without threads (NTL_THREADS=off), and in particular non-GNU linkers might refuse to link flint (actually happens on FreeBSD).

Also happens on macOS -- see for example https://groups.google.com/d/msg/sage-release/hobZzw74Rv0/VkAv7bG6DAAJ

See also:

CC: @embray @kiwifb @mkoeppe @dcoudert

Component: build

Author: Dima Pasechnik

Branch/Commit: u/dimpase/packages/flint-no-tls @ 2945e30

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

dimpase commented 5 years ago

Branch: u/dimpase/packages/flint-no-tls

dimpase commented 5 years ago

New commits:

386dd3fdisable TLS in flint
dimpase commented 5 years ago

Author: Dima Pasechnik

dimpase commented 5 years ago

Commit: 386dd3f

kiwifb commented 5 years ago
comment:3

By default the latest ntl should use threads - when the compiler is C++11 capable. Is it disabled on some OS/archs?

dimpase commented 5 years ago
comment:4

this is what Sage does, it disables threads in NTL. https://github.com/sagemath/sage-prod/blob/5ba5a5a40cd33901c5f4307d59aa68a4359b0430/build/pkgs/ntl/spkg-install#L103

dimpase commented 5 years ago
comment:5

an intelligent setup would sync this between dependencies automatically, but this seems to be not so easy.

kiwifb commented 5 years ago
comment:6

Replying to @dimpase:

this is what Sage does, it disables threads in NTL. https://github.com/sagemath/sage-prod/blob/5ba5a5a40cd33901c5f4307d59aa68a4359b0430/build/pkgs/ntl/spkg-install#L103

Well, we shouldn't anymore. I should have taken care of this during the ntl upgrade and things about C++11 standard. Unless there is a compelling reason to do so, I would recommend removing NTL_THREADS=off from ntl's spkg-install.

kiwifb commented 5 years ago
comment:7

On the other hand, flint's configuration system is un-impressive to me. And flint uses C++ for a grand total of compiling one file for the ntl interface. Which is optional. I have no idea if this is useful for anything.

dimpase commented 5 years ago
comment:9

flint’s interface to NTL is used in sagelib. So it cannot just be switched off.

kiwifb commented 5 years ago
comment:10

Replying to @dimpase:

flint’s interface to NTL is used in sagelib. So it cannot just be switched off.

OK. That make something clearer to me.

embray commented 5 years ago
comment:11

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

kiwifb commented 5 years ago
comment:12

Needs rebasing.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 386dd3f to 2945e30

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

2945e30disable TLS in flint
dimpase commented 5 years ago
comment:14

OK, done.

kiwifb commented 5 years ago
comment:15

OK now that I see again what was done and that I remember the conversation let's move on a bit. There are two options

Is there any reason or platforms preventing us from enabling threads in NTL? We historically disabled threads in NTL because it relied on C++11 and we weren't on board with it for all packages and that was causing problems on downstream packages where c++11 wasn't on. This should be solved now. So is there another reason we cannot enable threads in NTL?

dimpase commented 5 years ago
comment:16

I suppose enabling threads in NTL and flint, and all its dependencies e.g. arb, needs testing on all platforms we claim to support.

kiwifb commented 5 years ago
comment:17

That's a bot job. Literally. We should make a threading branch and see how the patch bots likes it.

embray commented 4 years ago
comment:18

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago
comment:20

Also related: build errors with Singular if NTL is threaded (see #29339/#29104).

mkoeppe commented 4 years ago
comment:21

Replying to @kiwifb:

We should make a threading branch and see how the patch bots likes it.

I have created ticket #29340 for this purpose. If someone wants to work on this (this requires fixing the problem with SIngular...), I'll be happy to test it using the new GitHub CI infrastructure.

mkoeppe commented 4 years ago
comment:22

For the present ticket -- is there any platform other than FreeBSD where this has been observed to be a problem?

dimpase commented 4 years ago
comment:25

yes, this happens on macOS 10.15 too, it seems

[sagelib-9.2.beta9] clang++ -bundle -undefined dynamic_lookup -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -L/Users/dima/software/sagetrac-mirror/local/lib -Wl,-rpath,/Users/dima/software/sagetrac-mirror/local/lib build/temp.macosx-10.15-x86_64-3.7/build/cythonized/sage/matrix/matrix_integer_sparse.o -L/usr/local/Cellar/openblas/0.3.10_1/lib -L/Users/dima/software/sagetrac-mirror/local/lib -L/usr/local/lib -L/usr/local/opt/openssl@1.1/lib -L/usr/local/opt/sqlite/lib -llinbox -lntl -liml -lfflas -lffpack -lgivaro -lflint -lmpfr -lgmp -lgmpxx -lopenblas -o build/lib.macosx-10.15-x86_64-3.7/sage/matrix/matrix_integer_sparse.cpython-37m-darwin.so -lpari
[sagelib-9.2.beta9] ld: illegal thread local variable reference to regular symbol __ZN3NTL20ZZXFac_InitNumPrimesE for architecture x86_64
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,13 @@
 By default, TLS (aka Thread-Local Storage) is enabled in flint.
 However, it contradicts to NTL configured without threads (`NTL_THREADS=off`), and in particular non-GNU linkers might refuse to link flint
 (actually happens on FreeBSD). 
+
+Also happens on macOS -- see for example https://groups.google.com/d/msg/sage-release/hobZzw74Rv0/VkAv7bG6DAAJ
+
+
+
+See also:
+- #29339 Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS
+- #29267 enabling thread safety for NTL
+
+
mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -9,5 +9,5 @@
 See also:
 - #29339 Fix NTL spkg-configure.m4 so it rejects NTLs built with NTL_THREADS
 - #29267 enabling thread safety for NTL
+- #30388 homebrew: Add flint

-
dimpase commented 4 years ago
comment:28

from what I recall - I was able to build and test Sage 9.2.beta12 on Homebrew macOS 10.15.6 using Homebrew's Flint and NTL, which do come with threads enabled. (Unfortunately the machine is bricked now, so I can't verify). Moving this to 9.3.

mkoeppe commented 3 years ago
comment:30

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

mkoeppe commented 3 years ago
comment:31

Setting a new milestone for this ticket based on a cursory review.

mkoeppe commented 2 years ago
comment:32

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.