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: sage-9.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/flint-no-tls (Commits, GitHub, GitLab) Commit: 2945e3041ab2a4d766b3d0bd345087c2f9e6ee03
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

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
  • #30388 homebrew: Add flint

Change History (34)

comment:1 Changed 4 years ago by Dima Pasechnik

Authors: Dima Pasechnik
Branch: u/dimpase/packages/flint-no-tls
Commit: 386dd3fd7899d9b861fa6cc8fbe0b56e62242ce6

New commits:

386dd3fdisable TLS in flint

comment:2 Changed 4 years ago by Dima Pasechnik

Status: newneeds_review

comment:3 Changed 4 years ago by François Bissey

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

comment:4 Changed 4 years ago by Dima Pasechnik

Last edited 4 years ago by Dima Pasechnik (previous) (diff)

comment:5 Changed 4 years ago by Dima Pasechnik

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

comment:6 in reply to:  4 Changed 4 years ago by François Bissey

Replying to dimpase:

this is what Sage does, it disables threads in NTL. https://github.com/sagemath/sage/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.

comment:7 Changed 4 years ago by François Bissey

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.

comment:8 Changed 4 years ago by Dima Pasechnik

Summary: configure flint with --disable-tlsconfigure threading in flint in sync with NTL

comment:9 Changed 4 years ago by Dima Pasechnik

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

comment:10 in reply to:  9 Changed 4 years ago by François Bissey

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 Erik Bray

Milestone: sage-8.8sage-8.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:12 Changed 3 years ago by François Bissey

Needs rebasing.

comment:13 Changed 3 years ago by git

Commit: 386dd3fd7899d9b861fa6cc8fbe0b56e62242ce62945e3041ab2a4d766b3d0bd345087c2f9e6ee03

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

2945e30disable TLS in flint

comment:14 Changed 3 years ago by Dima Pasechnik

OK, done.

comment:15 Changed 3 years ago by François Bissey

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 Dima Pasechnik

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 Changed 3 years ago by François Bissey

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 Erik Bray

Milestone: sage-8.9sage-9.1

Ticket retargeted after milestone closed

comment:19 Changed 3 years ago by Dima Pasechnik

Cc: Matthias Köppe added

comment:20 Changed 3 years ago by Matthias Köppe

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

comment:21 in reply to:  17 Changed 3 years ago by Matthias Köppe

Replying to fbissey:

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.

comment:22 Changed 3 years ago by Matthias Köppe

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 Matthias Köppe

Milestone: sage-9.1sage-9.2
Status: needs_reviewneeds_info

comment:24 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:25 Changed 2 years ago by Dima Pasechnik

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

comment:26 Changed 2 years ago by Matthias Köppe

Cc: David Coudert added
Description: modified (diff)
Milestone: sage-9.3sage-9.2
Priority: majorblocker

comment:27 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:28 Changed 2 years ago by Dima Pasechnik

Milestone: sage-9.2sage-9.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 Matthias Köppe

Priority: blockermajor

comment:30 Changed 20 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.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 Matthias Köppe

Milestone: sage-9.4sage-9.5

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

comment:32 Changed 11 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

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

comment:33 Changed 8 months ago by Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:34 Changed 2 months ago by Matthias Köppe

Milestone: sage-9.7sage-9.8
Note: See TracTickets for help on using tickets.