Opened 3 years ago

Last modified 5 weeks ago

#27764 needs_info defect

configure threading in flint in sync with NTL

Reported by: dimpase Owned by:
Priority: major Milestone: sage-9.6
Component: build Keywords:
Cc: embray, fbissey, mkoeppe, dcoudert 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 mkoeppe)

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 (32)

comment:1 Changed 3 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/flint-no-tls
  • Commit set to 386dd3fd7899d9b861fa6cc8fbe0b56e62242ce6

New commits:

386dd3fdisable TLS in flint

comment:2 Changed 3 years ago by dimpase

  • Status changed from new to needs_review

comment:3 Changed 3 years ago by fbissey

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

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

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

comment:5 Changed 3 years ago by dimpase

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

comment:6 in reply to: ↑ 4 Changed 3 years ago by fbissey

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

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 3 years ago by dimpase

  • Summary changed from configure flint with --disable-tls to configure threading in flint in sync with NTL

comment:9 follow-up: Changed 3 years ago by dimpase

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

comment:10 in reply to: ↑ 9 Changed 3 years ago by fbissey

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 embray

  • Milestone changed from sage-8.8 to sage-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 fbissey

Needs rebasing.

comment:13 Changed 3 years ago by git

  • Commit changed from 386dd3fd7899d9b861fa6cc8fbe0b56e62242ce6 to 2945e3041ab2a4d766b3d0bd345087c2f9e6ee03

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 dimpase

OK, done.

comment:15 Changed 3 years ago by fbissey

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 dimpase

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 follow-up: Changed 3 years ago by fbissey

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

comment:18 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:19 Changed 23 months ago by dimpase

  • Cc mkoeppe added

comment:20 Changed 23 months ago by mkoeppe

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

comment:21 in reply to: ↑ 17 Changed 23 months ago by mkoeppe

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 23 months ago by mkoeppe

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

comment:23 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2
  • Status changed from needs_review to needs_info

comment:24 Changed 18 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:25 Changed 17 months ago by dimpase

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 17 months ago by mkoeppe

  • Cc dcoudert added
  • Description modified (diff)
  • Milestone changed from sage-9.3 to sage-9.2
  • Priority changed from major to blocker

comment:27 Changed 17 months ago by mkoeppe

  • Description modified (diff)

comment:28 Changed 16 months ago by dimpase

  • Milestone changed from sage-9.2 to sage-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 13 months ago by mkoeppe

  • Priority changed from blocker to major

comment:30 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-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 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

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

comment:32 Changed 5 weeks ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

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

Note: See TracTickets for help on using tickets.