sagemath / sage

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

mpir-3.0.0 does not compile on skylake chips on OSX #24085

Closed AndrewMathas closed 6 years ago

AndrewMathas commented 6 years ago

As discussed on sage-devel, mpir does not build on Skylake chips. As reported by Bill Hart,

We know what the problem is now, but don't have a working patch to fix it. It's the JMPENT macro in mpn/x86_64/x86_64-defs.m4 that doesn't work on 64 bit OSX. Details of the issue can be found here.

In the end, the fix was to edit jump labels in a (large) number of MPIR's asm files, and change the .section type of jumptables, while leaving JMPENT macros intact. The changes are way too big for Sage patches (and done against the master rather than against meanwhile a bit outdated 3.0.0 version), thus a new tarball.

Tarball: http://sage.ugent.be/www/jdemeyer/sage/mpir-3.0.0-644faf502c56f97d9accd301965fc57d6ec70868.tar.bz2

Upstream: Completely fixed; Fix reported upstream

CC: @wbhart @kiwifb

Component: packages: standard

Keywords: skylake, mpir

Author: Dima Pasechnik, Jeroen Demeyer

Branch: c2384a7

Reviewer: David Roe, Andrew Mathas, Dima Pasechnik

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

roed314 commented 6 years ago
comment:42

The commit passes all tests in

./configure CC=cc --prefix=/tmp/blah && make -j8 && make -j8 check

Wonderful!

Now I'm trying the Sage fix.

dimpase commented 6 years ago

Changed branch from u/dimpase/skylakefix to none

dimpase commented 6 years ago

Changed commit from ad77588 to none

dimpase commented 6 years ago
comment:43

OK, great.

Oops, sorry, there is no Sage patch up yet. I was waiting for a confirmation that this fix works. Now I need to make a patch against mpir 3.0.0. Please bear with me.

roed314 commented 6 years ago
comment:44

No problem! Thanks for your work on this; debugging assembler without access to the architecture+OS sounds hard....

I could also have noticed that there was no new patch, but I'd just woken up. :-)

dimpase commented 6 years ago

Changed upstream from Workaround found; Bug reported upstream. to Completely fixed; Fix reported upstream

dimpase commented 6 years ago
comment:45

Just asked upstream for a new release - packaging all these changes since 3.0.0 is a pain.

dimpase commented 6 years ago

Branch: u/dimpase/skylakefix

dimpase commented 6 years ago

Commit: 7592b4a

dimpase commented 6 years ago
comment:46

get the non-official tarball from http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1.tar.bz2 Please test.

The upstream promises a release, but it's not instant.


New commits:

7592b4afix for #24085 packaged as an update with non-official tarball
roed314 commented 6 years ago
comment:47

I downloaded a fresh sage-8.1.rc3, downloaded the mpir tarball and put it in upstream, deleted mpir-3.0.0.tar.bz2, applied the branch on this ticket and then ran make. It resulted in the following error:

sage-logger -p 'sage-spkg mpir-3.0.1.p0' '/Users/roed/sage/sage-8.1.rc3/logs/pkgs/mpir-3.0.1.p0.log'
[mpir-3.0.1.p0] Found local metadata for mpir-3.0.1.p0
[mpir-3.0.1.p0] Using cached file /Users/roed/sage/sage-8.1.rc3/upstream/mpir-3.0.1.tar.bz2
[mpir-3.0.1.p0] mpir-3.0.1.p0
[mpir-3.0.1.p0] ====================================================
[mpir-3.0.1.p0] Setting up build directory for mpir-3.0.1.p0
[mpir-3.0.1.p0] Finished extraction
[mpir-3.0.1.p0] No patch files found in ../patches
[mpir-3.0.1.p0] ****************************************************
[mpir-3.0.1.p0] Host system:
[mpir-3.0.1.p0] Darwin Davids-MBP.home.int.hackorp.com 16.7.0 Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64
[mpir-3.0.1.p0] ****************************************************
[mpir-3.0.1.p0] C compiler: gcc
[mpir-3.0.1.p0] C compiler version:
[mpir-3.0.1.p0] Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
[mpir-3.0.1.p0] Apple LLVM version 9.0.0 (clang-900.0.37)
[mpir-3.0.1.p0] Target: x86_64-apple-darwin16.7.0
[mpir-3.0.1.p0] Thread model: posix
[mpir-3.0.1.p0] InstalledDir: /Library/Developer/CommandLineTools/usr/bin
[mpir-3.0.1.p0] ****************************************************
[mpir-3.0.1.p0] Machine type (default): x86_64-apple-darwin16.7.0
[mpir-3.0.1.p0] Machine type (mpir): skylakeavx-apple-darwin16.7.0
[mpir-3.0.1.p0] Building a 64-bit version of MPIR, which is the default.
[mpir-3.0.1.p0] Building a reduced version of MPIR to bootstrap GCC.
[mpir-3.0.1.p0] MPIR will later get rebuilt (with the C++ interface and static libraries
[mpir-3.0.1.p0] enabled) using the new compiler.
[mpir-3.0.1.p0] ------------------------------------------------------------------------
[mpir-3.0.1.p0] Configuring MPIR with empty CFLAGS to determine the defaults:
[mpir-3.0.1.p0] configure: error: cannot find install-sh, install.sh, or shtool in "." "./.." "./../.."
[mpir-3.0.1.p0] Error configuring MPIR (with CFLAGS unset).
[mpir-3.0.1.p0] Consult /Users/roed/sage/sage-8.1.rc3/local/var/tmp/sage/build/mpir-3.0.1.p0/src/config.log for for details.
[mpir-3.0.1.p0] 
[mpir-3.0.1.p0] real    0m0.695s
[mpir-3.0.1.p0] user    0m0.240s
[mpir-3.0.1.p0] sys 0m0.343s
[mpir-3.0.1.p0] ************************************************************************
[mpir-3.0.1.p0] Error installing package mpir-3.0.1.p0
[mpir-3.0.1.p0] ************************************************************************
[mpir-3.0.1.p0] Please email sage-devel (http://groups.google.com/group/sage-devel)
[mpir-3.0.1.p0] explaining the problem and including the log file
[mpir-3.0.1.p0]   /Users/roed/sage/sage-8.1.rc3/logs/pkgs/mpir-3.0.1.p0.log
[mpir-3.0.1.p0] Describe your computer, operating system, etc.
[mpir-3.0.1.p0] If you want to try to fix the problem yourself, *don't* just cd to
[mpir-3.0.1.p0] /Users/roed/sage/sage-8.1.rc3/local/var/tmp/sage/build/mpir-3.0.1.p0 and type 'make' or whatever is appropriate.
[mpir-3.0.1.p0] Instead, the following commands setup all environment variables
[mpir-3.0.1.p0] correctly and load a subshell for you to debug the error:
[mpir-3.0.1.p0]   (cd '/Users/roed/sage/sage-8.1.rc3/local/var/tmp/sage/build/mpir-3.0.1.p0' && '/Users/roed/sage/sage-8.1.rc3/sage' --sh)
[mpir-3.0.1.p0] When you are done debugging, you can type "exit" to leave the subshell.
[mpir-3.0.1.p0] ************************************************************************
make[2]: *** [/Users/roed/sage/sage-8.1.rc3/local/var/lib/sage/installed/mpir-3.0.1.p0] Error 1
make[1]: *** [all-toolchain] Error 2

I've posted local/var/tmp/sage/build/mpir-3.0.1.p0/src/config.log at http://www.pitt.edu/~roed/config_24085_3.log

wbhart commented 6 years ago
comment:48

I think I should clarify that there is no "upstream". I am happy to advise on what needs to be done and upload tarballs to the website, but MPIR is a community supported project now. I am of course merging patches, and my repository is still the official repository, but that is the limit of my contribution, unless I personally need something myself.

wbhart commented 6 years ago
comment:49

I'm not sure how the tarball was created, but in general you can't make a tarball from the MPIR repository without running make dist, since it does some post changes. And make dist should only be run if all the paperwork is up-to-date, version numbers have been updated and the .so versioning is taken care of.

dimpase commented 6 years ago
comment:50

Replying to @roed314:

I downloaded a fresh sage-8.1.rc3, downloaded the mpir tarball and put it in upstream, deleted mpir-3.0.0.tar.bz2, applied the branch on this ticket and then ran make.

IMHO you might need to run ./configure before running make. It could be that the install procedure got confused about the package versions...

The tarball worked for me on an installation of Sage, where I dropped it to upstream/, pulled the branch, and ran ./sage -f mpir followed by make...

dimpase commented 6 years ago
comment:51

Replying to @wbhart:

I'm not sure how the tarball was created, but in general you can't make a tarball from the MPIR repository without running make dist, since it does some post changes. And make dist should only be run if all the paperwork is up-to-date, version numbers have been updated and the .so versioning is taken care of.

Hereby, in regard of allegations of fragrant tarball paperwork forgery, I invoke the 5th.

Whereas, there is no direct connection between Sage package numbers and versions of the library. Thus the tarball will pretend to be mpir 3.0.0, I think...

roed314 commented 6 years ago
comment:52

Replying to @dimpase:

Replying to @roed314:

I downloaded a fresh sage-8.1.rc3, downloaded the mpir tarball and put it in upstream, deleted mpir-3.0.0.tar.bz2, applied the branch on this ticket and then ran make.

IMHO you might need to run ./configure before running make. It could be that the install procedure got confused about the package versions...

I tried ./configure && make, with no success.

The tarball worked for me on an installation of Sage, where I dropped it to upstream/, pulled the branch, and ran ./sage -f mpir followed by make...

I didn't pull anything, but I wouldn't expect that to be necessary given that you made a tarball with the changes. I'm not sure how to proceed....

dimpase commented 6 years ago
comment:53

Replying to @roed314:

Replying to @dimpase:

Replying to @roed314:

I downloaded a fresh sage-8.1.rc3, downloaded the mpir tarball and put it in upstream, deleted mpir-3.0.0.tar.bz2, applied the branch on this ticket and then ran make.

IMHO you might need to run ./configure before running make. It could be that the install procedure got confused about the package versions...

I tried ./configure && make, with no success.

The tarball worked for me on an installation of Sage, where I dropped it to upstream/, pulled the branch, and ran ./sage -f mpir followed by make...

I didn't pull anything, but I wouldn't expect that to be necessary given that you made a tarball with the changes. I'm not sure how to proceed....

You do need to pull the branch u/dimpase/skylakefix. It is very short:

diff --git a/build/pkgs/mpir/checksums.ini b/build/pkgs/mpir/checksums.ini
index 185e5ca..d2acf76 100644
--- a/build/pkgs/mpir/checksums.ini
+++ b/build/pkgs/mpir/checksums.ini
@@ -1,4 +1,4 @@
 tarball=mpir-VERSION.tar.bz2
-sha1=8300b351b730891dfb9cb1e411a5ca39eee73fda
-md5=4e5d16676e0cd6773f43bbbeb5cb0016
-cksum=2173521258
+sha1=1f7081c2f4a83fb4e9e3453bbafb6aa18f1c5d5a
+md5=308f4a981c0a2e796aa41ff3ac805b81
+cksum=2473237225
diff --git a/build/pkgs/mpir/package-version.txt b/build/pkgs/mpir/package-version.txt
index e3e3689..a7590f2 100644
--- a/build/pkgs/mpir/package-version.txt
+++ b/build/pkgs/mpir/package-version.txt
@@ -1 +1 @@
-3.0.0.p0
+3.0.1.p0

There are no patches to MPIR in the branch, they are indeed packaged in the tarball.

Tarballs are matched by package-version.txt, which gives you VERSION in tarball=mpir-VERSION.tar.bz2, and checked for authenticity using sha1 and md5.

roed314 commented 6 years ago
comment:54

Replying to @dimpase:

Replying to @roed314:

Replying to @dimpase:

Replying to @roed314:

I downloaded a fresh sage-8.1.rc3, downloaded the mpir tarball and put it in upstream, deleted mpir-3.0.0.tar.bz2, applied the branch on this ticket and then ran make.

IMHO you might need to run ./configure before running make. It could be that the install procedure got confused about the package versions...

I tried ./configure && make, with no success.

The tarball worked for me on an installation of Sage, where I dropped it to upstream/, pulled the branch, and ran ./sage -f mpir followed by make...

I didn't pull anything, but I wouldn't expect that to be necessary given that you made a tarball with the changes. I'm not sure how to proceed....

You do need to pull the branch u/dimpase/skylakefix.

Oh, I misunderstood you: I thought you meant pull from the mpir repo. I did pull the branch on this ticket, so I'm using the correct tarball.

There are no patches to MPIR in the branch, they are indeed packaged in the tarball.

Tarballs are matched by package-version.txt, which gives you VERSION in tarball=mpir-VERSION.tar.bz2, and checked for authenticity using sha1 and md5.

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

Branch pushed to git repo; I updated commit sha1. New commits:

782e287tarball updated
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 7592b4a to 782e287

dimpase commented 6 years ago
comment:56

I've updated the tarball and the branch, hopefully it works now...


New commits:

782e287tarball updated
roed314 commented 6 years ago
comment:57

It's looking good: the build has made it to compiling the Sage library. I'm going to run tests, but need to go to sleep now. Assuming tests pass, positive review from me (probably in about 8 hours).

roed314 commented 6 years ago
comment:58

So, the build finished, but Sage doesn't start. The error that it gives doesn't seem particularly related to mpir though....

I've put the crash report at http://www.pitt.edu/~roed/crash_report_24085.txt in case anyone wants to take a look. I'm going to sleep now; I'll try to take a look tomorrow.

dimpase commented 6 years ago
comment:59

at the bottom of your log one sees certain looking dodgy ~/.venvburrito/ directory reference

/Users/roed/.venvburrito/lib/python2.7/site-packages/six.pyc in with_metaclass(meta=<type 'sage.misc.classcall_metaclass.ClasscallMetaclass'>, *bases=())

I would have moved it out of the way and tried to start Sage again. If you still have issues like this afterwards, I'd have moved ~/.sage/ too.

roed314 commented 6 years ago
comment:60

Thanks. I had to use Homebrew to install autotools; I think that's where the ~/.venvburrito came from. I've moved it, and now tests are running.

dimpase commented 6 years ago
comment:61

on my museum (7.y.o.) MacBookAir I get a timeout in one test:

sage -t --long --warn-long 1000.9 src/sage/rings/padics/padic_generic_element.pyx
...
sage: R = Zp(17,10^6) ## line 2626 ##
sage: a = 17 * R.random_element() ## line 2627 ##
sage: b = a.exp()    # should be rather fast ## line 2628 ##
------------------------------------------------------------------------
0   signals.so                          0x000000010995b2d8 print_backtrace + 40
1   ???                                 0x98c0d8bd7ba6be0e 0x0 + 11007035797628435982
------------------------------------------------------------------------

**********************************************************************
----------------------------------------------------------------------
sage -t --long --warn-long 1000.9 src/sage/rings/padics/padic_generic_element.pyx  # Timed out
Total time for all tests: 1800.1 seconds
...

If I try this b=a.exp() at the Sage prompt it keeps running seemingly forever. Perhaps knowing concrete values (not random a) to test would help here...

If I use 10^5 instead of 10^6 in the definition of R, it still seems to return in about 10 sec.

AndrewMathas commented 6 years ago
comment:62

Thanks for this! Once I moved mpir-3.0.1.tar.bz2 into upstream sage built without any problem (macbook pro with a skylake chip running 10.12.6). I'm currently running the tests and so far they have all passed, up to src/sage/plot/plot3d/shapes.pyx but it's time for me to crash...

AndrewMathas commented 6 years ago
comment:63

Oh, and for me the tests pass with sage -t --long --warn-long 1000.9 src/sage/rings/padics/padic_generic_element.pyx but I get the same problems with:

sage: R = Zp(17,10^6) ## line 2626 ##
sage: a = 17 * R.random_element() ## line 2627 ##
sage: b = a.exp()    # should be rather fast ## line 2628 ##
roed314 commented 6 years ago

Reviewer: David Roe

roed314 commented 6 years ago
comment:64

All tests pass on my machine. Given that 8.1 is about to be released and this change makes Sage buildable on a Skylake Mac, I think it's reasonable to try to get it in and worry about any potential slowdowns on another ticket.

I'm not seeing issues with src/sage/rings/padics/padic_generic_element.pyx, but note that changes in mpir could affect that code. The code that's eventually getting called is the padicexp function in src/sage/rings/padics/transcendental.c.

dimpase commented 6 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,8 @@
 > on 64 bit OSX. Details of the issue can be found
 > [here](https://gmplib.org/list-archives/gmp-bugs/2012-December/002836.html).

-the branch patches the relevant m4 macros. It is for testing purposes only - a proper fix should make it conditional for OSX.
+In the end, the fix was to edit jump labels in a (large) number of MPIR's asm files, and change the `.section` type of jumptables, while leaving `JMPENT` macros intact. The changes are way too big for Sage patches (and done against the master rather than against meanwhile a bit outdated 3.0.0 version), thus a new tarball.   
+
+The tarball: http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1.tar.bz2
+
+
dimpase commented 6 years ago

Changed reviewer from David Roe to David Roe, Andrew Mathas

jdemeyer commented 6 years ago
comment:66

Could you better document where you obtained this tarball? I can't find it on http://mpir.org/downloads.html

jdemeyer commented 6 years ago
comment:67

See also [comment:23]

roed314 commented 6 years ago
comment:68

There's not an official release: Dima made a tarball which is available at ​http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1.tar.bz2

dimpase commented 6 years ago
comment:69

The tarball has been made from https://github.com/dimpase/mpir/commit/65bb82fdf65765e6edfbbbf8edb47c726413b8da (now merged in https://github.com/wbhart/mpir/pull/234) by following (already outdated, as Billa says) https://github.com/wbhart/mpir/commit/1b3d5976292c6308c3f121c5ea2c406da0324bc8 and running make dist.

Bill is planning a new release, I gather.

jdemeyer commented 6 years ago
comment:70

Replying to @dimpase:

The tarball has been made from https://github.com/dimpase/mpir/commit/65bb82fdf65765e6edfbbbf8edb47c726413b8da (now merged in https://github.com/wbhart/mpir/pull/234) by following (already outdated, as Billa says) https://github.com/wbhart/mpir/commit/1b3d5976292c6308c3f121c5ea2c406da0324bc8 and running make dist.

  1. What does "made from X by following Y mean"?

  2. Can you please document this in SPKG.txt or somewhere where people will find it?

jdemeyer commented 6 years ago
comment:71
  1. Do not call it mpir-3.0.1. Call it mpir-3.0.0-dima or mpir-3.0.0-1b3d5976292c6308c3f121c5ea2c406da0324bc8 or whatever.
jdemeyer commented 6 years ago
comment:72

Replying to @roed314:

There's not an official release: Dima made a tarball which is available at ​http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1.tar.bz2

Fine for me, but how you expect people to know this?

I don't mind custom tarballs, as long as it is absolutely clear what is going on.

dimpase commented 6 years ago
comment:73

MPIR used to have a convoluted way to make releases, involving modifying version numbers some files, running autoconf, editing generated configure, running configure, and then running make dist to make a tarball. And the said tarball needs to be pruned for heaps of MSVC-specific config files, that take 10 times more space than the code...

Riding trains Oxford-London-Paris, might be unable to do much before evening.

jdemeyer commented 6 years ago
comment:74

Replying to @dimpase:

MPIR used to have a convoluted way to make releases, involving modifying version numbers some files, running autoconf, editing generated configure, running configure, and then running make dist to make a tarball. And the said tarball needs to be pruned for heaps of MSVC-specific config files, that take 10 times more space than the code...

Then are you sure that simply adding a patch isn't simpler?

dimpase commented 6 years ago
comment:75

Replying to @jdemeyer:

Replying to @dimpase:

MPIR used to have a convoluted way to make releases, involving modifying version numbers some files, running autoconf, editing generated configure, running configure, and then running make dist to make a tarball. And the said tarball needs to be pruned for heaps of MSVC-specific config files, that take 10 times more space than the code...

Then are you sure that simply adding a patch isn't simpler?

because my patches are not against mpir 3.0.0, they are against last week's master. And there is a hundred of commits in between, and also 3.0.0 is a different branch, in fact. I tried created a patch against 3.0.0, but it would have been magabytes in size and a lot of time to create.

dimpase commented 6 years ago
comment:76

anyhow, I can rename the tarball and the corr. different version in the package-version, but don't ask me to do tarball creation from scratch again.

jdemeyer commented 6 years ago
comment:77

Replying to @dimpase:

anyhow, I can rename the tarball and the corr. different version in the package-version, but don't ask me to do tarball creation from scratch again.

I am asking you to document what you did, not to do it again.

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

Changed commit from 782e287 to e9a44b8

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

Branch pushed to git repo; I updated commit sha1. New commits:

e9a44b8renamed tarball 3.0.1prerelease, info on building it added to SPKG.txt
dimpase commented 6 years ago
comment:79

I've renamed the tarball.


New commits:

e9a44b8renamed tarball 3.0.1prerelease, info on building it added to SPKG.txt
dimpase commented 6 years ago

Description changed:

--- 
+++ 
@@ -7,6 +7,6 @@

 In the end, the fix was to edit jump labels in a (large) number of MPIR's asm files, and change the `.section` type of jumptables, while leaving `JMPENT` macros intact. The changes are way too big for Sage patches (and done against the master rather than against meanwhile a bit outdated 3.0.0 version), thus a new tarball.   

-The tarball: http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1.tar.bz2
+The tarball: http://users.ox.ac.uk/~coml0531/sage/mpir-3.0.1prerelease.tar.bz2
jdemeyer commented 6 years ago
comment:80

3.0.1prerelease is built from https://github.com/wbhart/mpir/pull/234

Can you instead refer to an exact commit on the git repo?

following the (outdated) procedure outlined in https://github.com/wbhart/mpir/commit/1b3d5976292c6308c3f121c5ea2c406da0324bc8

That's also not clear. How does this commit outline a procedure?

dimpase commented 6 years ago
comment:82

Perhaps you have not had a look at the commit I mention? There are files to edit, scripts to run, and it is outdated. I am not going to spend time describing details- I am sure anyone with an idea on how mpir tarballs are made will manage to reproduce this.