Opened 4 years ago
Last modified 2 months ago
#27764 needs_info defect
configure threading in flint in sync with NTL
Reported by:  Dima Pasechnik  Owned by:  

Priority:  major  Milestone:  sage9.8 
Component:  build  Keywords:  
Cc:  Erik Bray, François Bissey, Matthias Köppe, David Coudert  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/dimpase/packages/flintnotls (Commits, GitHub, GitLab)  Commit:  2945e3041ab2a4d766b3d0bd345087c2f9e6ee03 
Dependencies:  Stopgaps: 
Description (last modified by )
By default, TLS (aka ThreadLocal Storage) is enabled in flint.
However, it contradicts to NTL configured without threads (NTL_THREADS=off
), and in particular nonGNU linkers might refuse to link flint
(actually happens on FreeBSD).
Also happens on macOS  see for example https://groups.google.com/d/msg/sagerelease/hobZzw74Rv0/VkAv7bG6DAAJ
See also:
Change History (34)
comment:1 Changed 4 years ago by
Authors:  → Dima Pasechnik 

Branch:  → u/dimpase/packages/flintnotls 
Commit:  → 386dd3fd7899d9b861fa6cc8fbe0b56e62242ce6 
comment:2 Changed 4 years ago by
Status:  new → needs_review 

comment:3 Changed 4 years ago by
By default the latest ntl
should use threads  when the compiler is C++11 capable. Is it disabled on some OS/archs?
comment:4 followup: 6 Changed 4 years ago by
this is what Sage does, it disables threads in NTL. https://github.com/sagemath/sage/blob/5ba5a5a40cd33901c5f4307d59aa68a4359b0430/build/pkgs/ntl/spkginstall#L103
comment:5 Changed 4 years ago by
an intelligent setup would sync this between dependencies automatically, but this seems to be not so easy.
comment:6 Changed 4 years ago by
Replying to dimpase:
this is what Sage does, it disables threads in NTL. https://github.com/sagemath/sage/blob/5ba5a5a40cd33901c5f4307d59aa68a4359b0430/build/pkgs/ntl/spkginstall#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 spkginstall.
comment:7 Changed 4 years ago by
On the other hand, flint's configuration system is unimpressive 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.
comment:8 Changed 4 years ago by
Summary:  configure flint with disabletls → configure threading in flint in sync with NTL 

comment:9 followup: 10 Changed 4 years ago by
flint’s interface to NTL is used in sagelib. So it cannot just be switched off.
comment:10 Changed 4 years ago by
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.
comment:11 Changed 3 years ago by
Milestone:  sage8.8 → sage8.9 

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).
comment:13 Changed 3 years ago by
Commit:  386dd3fd7899d9b861fa6cc8fbe0b56e62242ce6 → 2945e3041ab2a4d766b3d0bd345087c2f9e6ee03 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2945e30  disable TLS in flint

comment:15 Changed 3 years ago by
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
 enabling threads in NTL and letting flint do the same
 leaving threads disabled in NTL and applying this ticket
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?
comment:16 Changed 3 years ago by
I suppose enabling threads in NTL and flint, and all its dependencies e.g. arb, needs testing on all platforms we claim to support.
comment:17 followup: 21 Changed 3 years ago by
That's a bot job. Literally. We should make a threading branch and see how the patch bots likes it.
comment:18 Changed 3 years ago by
Milestone:  sage8.9 → sage9.1 

Ticket retargeted after milestone closed
comment:19 Changed 3 years ago by
Cc:  Matthias Köppe added 

comment:20 Changed 3 years ago by
comment:21 Changed 3 years ago by
comment:22 Changed 3 years ago by
For the present ticket  is there any platform other than FreeBSD where this has been observed to be a problem?
comment:23 Changed 3 years ago by
Milestone:  sage9.1 → sage9.2 

Status:  needs_review → needs_info 
comment:24 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:25 Changed 2 years ago by
yes, this happens on macOS 10.15 too, it seems
[sagelib9.2.beta9] clang++ bundle undefined dynamic_lookup isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk L/Users/dima/software/sagetracmirror/local/lib Wl,rpath,/Users/dima/software/sagetracmirror/local/lib build/temp.macosx10.15x86_643.7/build/cythonized/sage/matrix/matrix_integer_sparse.o L/usr/local/Cellar/openblas/0.3.10_1/lib L/Users/dima/software/sagetracmirror/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.macosx10.15x86_643.7/sage/matrix/matrix_integer_sparse.cpython37mdarwin.so lpari [sagelib9.2.beta9] ld: illegal thread local variable reference to regular symbol __ZN3NTL20ZZXFac_InitNumPrimesE for architecture x86_64
comment:26 Changed 2 years ago by
Cc:  David Coudert added 

Description:  modified (diff) 
Milestone:  sage9.3 → sage9.2 
Priority:  major → blocker 
comment:27 Changed 2 years ago by
Description:  modified (diff) 

comment:28 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

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.
comment:29 Changed 2 years ago by
Priority:  blocker → major 

comment:30 Changed 20 months ago by
Milestone:  sage9.3 → sage9.4 

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.
comment:31 Changed 17 months ago by
Milestone:  sage9.4 → sage9.5 

Setting a new milestone for this ticket based on a cursory review.
comment:32 Changed 11 months ago by
Milestone:  sage9.5 → sage9.6 

Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:33 Changed 8 months ago by
Milestone:  sage9.6 → sage9.7 

comment:34 Changed 2 months ago by
Milestone:  sage9.7 → sage9.8 

New commits:
disable TLS in flint