Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24085 closed defect (fixed)

mpir-3.0.0 does not compile on skylake chips on OSX

Reported by: andrew.mathas Owned by:
Priority: critical Milestone: sage-8.1
Component: packages: standard Keywords: skylake, mpir
Cc: wbhart, fbissey Merged in:
Authors: Dima Pasechnik, Jeroen Demeyer Reviewers: David Roe, Andrew Mathas, Dima Pasechnik
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: c2384a7 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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

Attachments (2)

config.log (171.1 KB) - added by andrew.mathas 4 years ago.
Log file for failed build
01.patch (1.6 KB) - added by dimpase 4 years ago.
the tarball vs the MPIR master

Download all attachments as: .zip

Change History (127)

comment:1 Changed 4 years ago by dimpase

  • Priority changed from critical to blocker

making this a blocker - we do want skylake in, no?

comment:2 Changed 4 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/skylakefix
  • Commit set to ee995509f1c5046a008ad332c8f757f1371c1c19
  • Status changed from new to needs_review

New commits:

ee99550skylake fix

comment:3 Changed 4 years ago by slelievre

  • Description modified (diff)

(Minor typo fixes and reformatting of ticket description.)

comment:4 Changed 4 years ago by dimpase

  • Description modified (diff)

the actual state of the ticket reflected in the descr. now.

comment:5 Changed 4 years ago by roed

When I pull this change to a build that failed before, I get

[mpir-3.0.0.p0] libtool: link: /usr/bin/gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libmpir.23.dylib  .libs/assert.o .libs/compat.o .libs/errno.o .libs/extract-dbl.o .libs/invalid.o .libs/memory.o .li
[mpir-3.0.0.p0] ld: in section __DATA,__const reloc 0: X86_64_RELOC_SUBTRACTOR must have r_extern=1 file 'mpn/.libs/addmul_1.o' for architecture x86_64
[mpir-3.0.0.p0] clang: error: linker command failed with exit code 1 (use -v to see invocation)

Full log posted at http://www.pitt.edu/~roed/mpir-24085.log.

I'll try starting from a fresh sage-8.1.rc2.

comment:6 Changed 4 years ago by roed

Very similar looking error when trying with a fresh Sage. Log posted at http://www.pitt.edu/~roed/mpir-24085_2.log

comment:7 Changed 4 years ago by dimpase

  • Cc wbhart added
  • Status changed from needs_review to needs_work

comment:8 Changed 4 years ago by dimpase

  • Description modified (diff)

Well, I merely presented in a patch form what was reported by Andrew to be a working workaround. Without access to the corresponding OS/arch combo, I can't do much.

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

  • Branch u/dimpase/skylakefix deleted
  • Commit ee995509f1c5046a008ad332c8f757f1371c1c19 deleted

David, could you post the output of MPIR's ./config.guess for your system?

I'm thinking about a better way to fix this: detect skylake+OSX combo and pass a different (working) arch to the MPIR build system.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by roed

Replying to dimpase:

David, could you post the output of MPIR's ./config.guess for your system?

It's at http://www.pitt.edu/~roed/config.guess

comment:11 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by dimpase

Replying to roed:

Replying to dimpase:

David, could you post the output of MPIR's ./config.guess for your system?

It's at http://www.pitt.edu/~roed/config.guess

this looks like a script. I meant: cd $SAGE_ROOT/upstream; tar xf mpir-3.0.0.tar.gz; cd mpir-3.0.0; ./config.guess

e.g. on my laptop I get the output skylakeavx-unknown-linux-gnu

comment:12 in reply to: ↑ 11 Changed 4 years ago by roed

Replying to dimpase:

Replying to roed:

Replying to dimpase:

David, could you post the output of MPIR's ./config.guess for your system?

It's at http://www.pitt.edu/~roed/config.guess

this looks like a script. I meant: cd $SAGE_ROOT/upstream; tar xf mpir-3.0.0.tar.gz; cd mpir-3.0.0; ./config.guess

Ah, thanks.

e.g. on my laptop I get the output skylakeavx-unknown-linux-gnu

I get skylakeavx-apple-darwin16.7.0

comment:13 Changed 4 years ago by alexjbest

So I also don't have the set up to test this, but it looks like deleting the files in x86_64w is the wrong place. the 64w folder is for windows code and wouldn't be used by osx? Perhaps deleting the files just in x86_64 will work. (If we look at David's log it does config.status: linking mpn/x86_64/skylake/avx/addmul_1.asm to mpn/addmul_1.asm so osx is using the x86_64 code)

comment:14 Changed 4 years ago by roed

Deleting the files in x86_64 or x86_64w? There's also x86w....

comment:15 Changed 4 years ago by alexjbest

x86_64 is 64bit *nix abi, x86_64w is 64bit windows, x86(w) is older 32-bit stuff for linux (resp. windows) . mpn/x86_64/ is what's being linked on your machine.

comment:16 Changed 4 years ago by roed

I tried

cd upstream/mpir-3.0.0/mpn/x86_64
rm -rf *
cd $SAGE_ROOT && make

I still get

[mpir-3.0.0.p1] libtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libmpir.23.dylib  .libs/assert.o .libs/compat.o .libs/errno.o .libs/extract-dbl.o .libs/invalid.o .libs/memory.o .libs/mp_bpi
[mpir-3.0.0.p1] ld: in section __DATA,__const reloc 0: X86_64_RELOC_SUBTRACTOR must have r_extern=1 file 'mpn/.libs/addmul_1.o' for architecture x86_64
[mpir-3.0.0.p1] clang: error: linker command failed with exit code 1 (use -v to see invocation)

Let me know if you have any other ideas....

comment:17 Changed 4 years ago by alexjbest

Sorry I was being lazy you don't want to remove all the x86_64 code just the 3 from before just from the right folder: mpir-3.0.0/mpn/x86_64/skylake/avx/addmul_1.asm mpir-3.0.0/mpn/x86_64/skylake/avx/mul_basecase.asm mpir-3.0.0/mpn/x86_64/skylake/avx/sqr_basecase.asm also you need to re-run ./configure after this, is sage make doing that?

Last edited 4 years ago by alexjbest (previous) (diff)

comment:18 Changed 4 years ago by roed

I downloaded a fresh copy of sage and deleted the files. At first I didn't see your comment about ./configure, but after getting an error, I ran ./configure && make, to no avail:

[mpir-3.0.0.p0] /bin/sh ./libtool  --tag=CC   --mode=link gcc  -m64 -O2 -march=skylake -mtune=skylake  -g    -version-info 23:3:0 -L/Users/roed/sage/sage-8.1.rc2/local/lib -Wl,-rpath,/Users/roed/sage/sage-8.1.rc2/ls
[mpir-3.0.0.p0] libtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/libmpir.23.dylib  .libs/assert.o .libs/compat.o .libs/errno.o .libs/extract-dbl.o .libs/invalid.o .libs/memory.o .libs/mp_bps
[mpir-3.0.0.p0] ld: in section __DATA,__const reloc 0: X86_64_RELOC_SUBTRACTOR must have r_extern=1 file 'mpn/.libs/addmul_1.o' for architecture x86_64
[mpir-3.0.0.p0] clang: error: linker command failed with exit code 1 (use -v to see invocation)

comment:19 Changed 4 years ago by dimpase

Following a tip from Bill, I am able to reproduce the issue on a non-skylake OSX 10.12 machine (with XCode 9). To do this (in MPIR source dir), do

./configure --host=skylakeavx-apple-darwin16.7.0
make

and get the familiar

ld: in section __DATA,__const reloc 0: X86_64_RELOC_SUBTRACTOR must have r_extern=1 file 'mpn/.libs/addmul_1.o' for architecture x86_64

(This should also work on Travis CI, if you are so inclined (although in my experience Travis is quite slow on OSX).

Now the plan is to lift the fix for the responsible m4 off GMP...

comment:20 Changed 4 years ago by dimpase

I am trying the following patch, basically taken from GMP

diff --git a/mpn/x86_64/x86_64-defs.m4 b/mpn/x86_64/x86_64-defs.m4
index 120efc7..0f65e55 100644
--- a/mpn/x86_64/x86_64-defs.m4
+++ b/mpn/x86_64/x86_64-defs.m4
@@ -129,9 +129,10 @@ dnl  Usage: JMPENT(targlabel,tablabel)
 
 define(`JMPENT',`dnl
 ifdef(`PIC',
-    `.long  $1-$2'
+        `.set   $1_tmp, $1-$2
+        .long   $1_tmp'
 ,
-    `.quad  $1'
+        `.quad  $1'
 )')
 
 dnl  Usage: call_mcount
@@ -666,7 +667,7 @@ define(`R8',
                 $1,`%r14',`%r14b',
                 $1,`%r15',`%r15b')')
 
-define(`JUMPTABSECT', `.section .data.rel.ro.local,"aw",@progbits')
+define(`JUMPTABSECT', `.text')
 
 
 dnl  Usage: JMPENT(targlabel,tablabel)

but it does not work, neither with the native (clang) toolchain, nor with gcc-7.2. (Same error with addmul_1.o as above). Could it be something to do with internal vs non-internal assembly?

comment:21 Changed 4 years ago by dimpase

  • Branch set to u/dimpase/skylakefix
  • Commit set to ad775881d806020c405d59fdb9671e8557fd9b50
  • Status changed from needs_work to needs_review

Please try this on the "real" skylake. It builds on the fake one (i.e. with --host=skylakeavx-apple-darwin16.7.0).

In the previous attempt I missed that JMPENT and JUMPTABSECT are defined twice...


New commits:

ad77588testing-only - for OSX skylake

comment:22 Changed 4 years ago by dimpase

  • Description modified (diff)
  • Summary changed from mpir-3.0.0 does not compile on skylake chips to mpir-3.0.0 does not compile on skylake chips on OSX

comment:23 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

The ticket says "Workaround found; Bug reported upstream." but where is the upstream report?

And please add a link to this ticket in the patch file.

comment:24 Changed 4 years ago by roed

I'm past the mpir compilation: looking good so far. I'll report once it's done and I've run tests.

comment:25 Changed 4 years ago by roed

I'm getting an error building gcc:

[gcc-7.2.0] /Users/roed/sage/sage-8.1.rc2/local/var/tmp/sage/build/gcc-7.2.0/gcc-build/./gcc/xgcc -B/Users/roed/sage/sage-8.1.rc2/local/var/tmp/sage/build/gcc-7.2.0/gcc-build/./gcc/ -nostdinc -x c /dev/null -S -o /d
[gcc-7.2.0] <built-in>: internal compiler error: Bus error: 10
[gcc-7.2.0] libbacktrace could not find executable to open
[gcc-7.2.0] Please submit a full bug report,
[gcc-7.2.0] with preprocessed source if appropriate.
[gcc-7.2.0] See <https://gcc.gnu.org/bugs/> for instructions.
[gcc-7.2.0] make[6]: *** [s-selftest] Error 1
[gcc-7.2.0] rm gcc.pod
[gcc-7.2.0] make[5]: *** [all-stage1-gcc] Error 2
[gcc-7.2.0] make[4]: *** [stage1-bubble] Error 2
[gcc-7.2.0] make[3]: *** [all] Error 2

Full log at http://www.pitt.edu/~roed/gcc_24085.log

comment:26 Changed 4 years ago by dimpase

OK, now I see that on my machine MPIR with this patch fails standalone tests. :-(

That's probably why gcc can't be built (mpir is used ) Let me try few variations to see if I can make it work.

comment:27 Changed 4 years ago by dimpase

In fact, it's linking errors that I get; the MPIR tests fail with the following:

$ less tests/t-constants.log 
dyld: Symbol not found: .Ll0m4_tmp
  Referenced from: /Volumes/Sage/sage/upstream/mpir-3.0.0/.libs/libmpir.23.dylib
  Expected in: flat namespace
 in /Volumes/Sage/sage/upstream/mpir-3.0.0/.libs/libmpir.23.dylib

comment:28 Changed 4 years ago by dimpase

  • Cc fbissey added

Right, the problem turns out to be that some assembler code in MPIR does not work with the new version of the JMPENT macro. Updating it still leads to two errors in MPIR testsuite. See https://github.com/wbhart/mpir/issues/232

It would be great if someone can tell how to write an m4 macro that is conditional on the OS type (we only need to change JMPENT on OSX).

comment:29 follow-up: Changed 4 years ago by vbraun

This ticket doesn't look like it'll be ready any time soon, skip it for 8.1?

comment:30 in reply to: ↑ 29 ; follow-up: Changed 4 years ago by dimpase

Replying to vbraun:

This ticket doesn't look like it'll be ready any time soon, skip it for 8.1?

I hope to have a working version by the end of today... But it's up to you, I think Apple should not slow Sage down (without paying us, for sure :-))

comment:31 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by roed

Replying to dimpase:

Replying to vbraun:

This ticket doesn't look like it'll be ready any time soon, skip it for 8.1?

I hope to have a working version by the end of today... But it's up to you, I think Apple should not slow Sage down (without paying us, for sure :-))

I can't help with the fix itself, but I'm happy to keep helping with testing.

comment:32 in reply to: ↑ 31 Changed 4 years ago by dimpase

Replying to roed:

Replying to dimpase:

Replying to vbraun:

This ticket doesn't look like it'll be ready any time soon, skip it for 8.1?

I hope to have a working version by the end of today... But it's up to you, I think Apple should not slow Sage down (without paying us, for sure :-))

I can't help with the fix itself, but I'm happy to keep helping with testing.

Could you try out (the master of) https://github.com/dimpase/mpir.git ? that is,

git clone https://github.com/dimpase/mpir.git
cd mpir
./autogen.sh
./configure CC=cc --prefix=/tmp/blah && make -j8 && make -j8 check

(assuming you have yasm installed) This fails 2 tests on non-skylake, perhaps it would work on skylake?

comment:33 Changed 4 years ago by andrew.mathas

I'm running xcode Version 9.0 (9A235) on on mscox 10.12.16. I got the following errors:

PASS: t-addadd_n
PASS: t-addsub_n
PASS: t-aors_1
../../test-driver: line 107: 94805 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 94803 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_bdiv_q
PASS: t-asmtype
FAIL: t-dc_bdiv_q_n
../../test-driver: line 107: 94861 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 94862 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 94863 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_div_q
FAIL: t-dc_bdiv_qr
FAIL: t-dc_bdiv_qr_n
../../test-driver: line 107: 94918 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 94920 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 94919 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_div_qr_n
FAIL: t-dc_divappr_q
FAIL: t-dc_div_qr
PASS: t-divrem_1
PASS: t-divebyff
PASS: t-divebyfobm1
../../test-driver: line 107: 95028 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 95027 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-gcdext
FAIL: t-fat
PASS: t-get_d
PASS: t-instrument
../../test-driver: line 107: 95085 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-hgcd
../../test-driver: line 107: 95115 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_div_q
../../test-driver: line 107: 95141 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 95142 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_div_qr
FAIL: t-inv_div_qr_n
../../test-driver: line 107: 95174 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_divappr_q
../../test-driver: line 107: 95199 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 95198 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-invert
FAIL: t-inv_divappr_q_n
PASS: t-iord_u
../../test-driver: line 107: 95255 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-matrix22
PASS: t-mp_bases
../../test-driver: line 107: 95307 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mullow_basecase
../../test-driver: line 107: 95326 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mullowhigh
../../test-driver: line 107: 95364 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmod_2expm1
PASS: t-lorrshift1
../../test-driver: line 107: 95383 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmod_2expp1
../../test-driver: line 107: 95345 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmid
PASS: t-perfsqr
../../test-driver: line 107: 95441 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-redc_1
PASS: t-neg
../../test-driver: line 107: 95455 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_bdiv_q
../../test-driver: line 107: 95479 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_bdiv_qr
../../test-driver: line 107: 95495 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_div_q
../../test-driver: line 107: 95499 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_div_qr
../../test-driver: line 107: 95536 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_divappr_q
PASS: t-scan
../../test-driver: line 107: 95558 Segmentation fault: 11  "$@" > $log_file 2>&1
FAIL: t-sizeinbase
PASS: t-subadd_n
../../test-driver: line 107: 95609 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-tdiv_q
../../test-driver: line 107: 95614 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-tdiv_qr
PASS: st_fat
PASS: st_instrument

Happy to upload the log but I couldn't find it...

comment:34 Changed 4 years ago by dimpase

Could we get the ./configure log?

comment:35 Changed 4 years ago by roed

I get similar errors; my log is at http://www.pitt.edu/~roed/config_24085.log

Changed 4 years ago by andrew.mathas

Log file for failed build

comment:36 Changed 4 years ago by dimpase

Please try the latest updates: do git pull in the mpir git repo, then make distclean followed by ./autogen.sh etc.

comment:37 Changed 4 years ago by roed

Still similar looking failures. I've uploaded the new log to http://www.pitt.edu/~roed/config_24085_2.log

comment:38 Changed 4 years ago by roed

The test output is

/Library/Developer/CommandLineTools/usr/bin/make  check-TESTS
../../test-driver: line 107: 31065 Bus error: 10           "$@" > $log_file 2>&1
PASS: t-asmtype
FAIL: t-dc_bdiv_q
PASS: t-aors_1
../../test-driver: line 107: 31068 Bus error: 10           "$@" > $log_file 2>&1
PASS: t-addsub_n
FAIL: t-dc_bdiv_q_n
../../test-driver: line 107: 31067 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 31069 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_bdiv_qr
FAIL: t-dc_bdiv_qr_n
PASS: t-addadd_n
../../test-driver: line 107: 31217 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 31214 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 31218 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_div_qr
FAIL: t-dc_div_qr_n
FAIL: t-dc_divappr_q
../../test-driver: line 107: 31215 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-dc_div_q
PASS: t-divebyff
PASS: t-divrem_1
../../test-driver: line 107: 31225 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-fat
../../test-driver: line 107: 31352 Bus error: 10           "$@" > $log_file 2>&1
PASS: t-instrument
../../test-driver: line 107: 31350 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 31346 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_div_q
FAIL: t-hgcd
FAIL: t-gcdext
PASS: t-get_d
PASS: t-divebyfobm1
../../test-driver: line 107: 31423 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_div_qr
../../test-driver: line 107: 31442 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_div_qr_n
../../test-driver: line 107: 31480 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-inv_divappr_q
../../test-driver: line 107: 31484 Bus error: 10           "$@" > $log_file 2>&1
../../test-driver: line 107: 31482 Bus error: 10           "$@" > $log_file 2>&1
PASS: t-iord_u
FAIL: t-invert
FAIL: t-inv_divappr_q_n
../../test-driver: line 107: 31489 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-matrix22
PASS: t-mp_bases
../../test-driver: line 107: 31591 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mullow_basecase
../../test-driver: line 107: 31628 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mullowhigh
../../test-driver: line 107: 31632 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmod_2expm1
PASS: t-perfsqr
../../test-driver: line 107: 31709 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-redc_1
../../test-driver: line 107: 31730 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_bdiv_q
../../test-driver: line 107: 31750 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_bdiv_qr
../../test-driver: line 107: 31762 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_div_q
../../test-driver: line 107: 31781 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_div_qr
../../test-driver: line 107: 31806 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-sb_divappr_q
PASS: t-scan
PASS: t-neg
../../test-driver: line 107: 31839 Segmentation fault: 11  "$@" > $log_file 2>&1
FAIL: t-sizeinbase
PASS: t-subadd_n
PASS: st_fat
PASS: st_instrument
../../test-driver: line 107: 31881 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-tdiv_q
../../test-driver: line 107: 31907 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-tdiv_qr
../../test-driver: line 107: 31631 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmid
../../test-driver: line 107: 31933 Bus error: 10           "$@" > $log_file 2>&1
FAIL: t-mulmod_2expp1
PASS: t-lorrshift1
============================================================================
Testsuite summary for MPIR 3.0.0
============================================================================
# TOTAL: 50
# PASS:  18
# SKIP:  0
# XFAIL: 0
# FAIL:  32
# XPASS: 0
# ERROR: 0
============================================================================
See tests/mpn/test-suite.log
Please report to http://groups.google.co.uk/group/mpir-devel/
============================================================================
make[5]: *** [test-suite.log] Error 1
make[4]: *** [check-TESTS] Error 2
make[3]: *** [check-am] Error 2
make[2]: *** [check-recursive] Error 1
make[1]: *** [check-recursive] Error 1
make: *** [check] Error 2

comment:39 Changed 4 years ago by dimpase

  • Priority changed from blocker to critical
  • Status changed from needs_info to needs_work

OK, I give up on trying to fix the assembler code there. This needs debugging on an actual box.

comment:40 Changed 4 years ago by dimpase

OK, please try the latest https://github.com/dimpase/mpir

this commit should hopefully suffice.

comment:41 Changed 4 years ago by dimpase

  • Status changed from needs_work to needs_review

comment:42 Changed 4 years ago by roed

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.

comment:43 Changed 4 years ago by dimpase

  • Branch u/dimpase/skylakefix deleted
  • Commit ad775881d806020c405d59fdb9671e8557fd9b50 deleted

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.

comment:44 Changed 4 years ago by roed

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. :-)

comment:45 Changed 4 years ago by dimpase

  • Report Upstream changed from Workaround found; Bug reported upstream. to Completely fixed; Fix reported upstream

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

comment:46 Changed 4 years ago by dimpase

  • Branch set to u/dimpase/skylakefix
  • Commit set to 7592b4af53bb912f0d4f3fc5c6d270e9050892b7

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

comment:47 follow-up: Changed 4 years ago by roed

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

comment:48 Changed 4 years ago by wbhart

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.

comment:49 follow-up: Changed 4 years ago by 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.

comment:50 in reply to: ↑ 47 ; follow-up: Changed 4 years ago by dimpase

Replying to roed:

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...

comment:51 in reply to: ↑ 49 Changed 4 years ago by dimpase

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...

comment:52 in reply to: ↑ 50 ; follow-up: Changed 4 years ago by roed

Replying to dimpase:

Replying to roed:

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....

comment:53 in reply to: ↑ 52 ; follow-up: Changed 4 years ago by dimpase

Replying to roed:

Replying to dimpase:

Replying to roed:

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.

comment:54 in reply to: ↑ 53 Changed 4 years ago by roed

Replying to dimpase:

Replying to roed:

Replying to dimpase:

Replying to roed:

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.

comment:55 Changed 4 years ago by git

  • Commit changed from 7592b4af53bb912f0d4f3fc5c6d270e9050892b7 to 782e287f65f4ff4adae5fa89032924b8610adbeb

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

782e287tarball updated

comment:56 Changed 4 years ago by dimpase

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


New commits:

782e287tarball updated

comment:57 Changed 4 years ago by roed

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).

comment:58 Changed 4 years ago by roed

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.

comment:59 Changed 4 years ago by dimpase

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.

comment:60 Changed 4 years ago by roed

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.

comment:61 Changed 4 years ago by dimpase

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.

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

comment:62 Changed 4 years ago by andrew.mathas

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...

comment:63 Changed 4 years ago by andrew.mathas

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 ##

comment:64 Changed 4 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

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.

comment:65 Changed 4 years ago by dimpase

  • Description modified (diff)
  • Reviewers changed from David Roe to David Roe, Andrew Mathas

comment:66 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_info

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

comment:67 Changed 4 years ago by jdemeyer

See also 23

comment:68 follow-ups: Changed 4 years ago by roed

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

comment:69 follow-up: Changed 4 years ago by 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.

Bill is planning a new release, I gather.

comment:70 in reply to: ↑ 69 Changed 4 years ago by jdemeyer

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"?
  1. Can you please document this in SPKG.txt or somewhere where people will find it?

comment:71 in reply to: ↑ 68 Changed 4 years ago by jdemeyer

  1. Do not call it mpir-3.0.1. Call it mpir-3.0.0-dima or mpir-3.0.0-1b3d5976292c6308c3f121c5ea2c406da0324bc8 or whatever.
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:72 in reply to: ↑ 68 Changed 4 years ago by jdemeyer

Replying to roed:

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.

comment:73 follow-up: Changed 4 years ago by 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...

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

comment:74 in reply to: ↑ 73 ; follow-up: Changed 4 years ago by 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?

comment:75 in reply to: ↑ 74 ; follow-up: Changed 4 years ago by dimpase

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.

comment:76 follow-up: Changed 4 years ago by 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.

comment:77 in reply to: ↑ 76 Changed 4 years ago by jdemeyer

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.

comment:78 Changed 4 years ago by git

  • Commit changed from 782e287f65f4ff4adae5fa89032924b8610adbeb to e9a44b84638822a36ba143f20978cf9f585afe3b

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

e9a44b8renamed tarball 3.0.1prerelease, info on building it added to SPKG.txt

comment:79 Changed 4 years ago by dimpase

  • Description modified (diff)
  • Status changed from needs_info to needs_review

I've renamed the tarball.


New commits:

e9a44b8renamed tarball 3.0.1prerelease, info on building it added to SPKG.txt

comment:80 Changed 4 years ago by jdemeyer

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?

comment:81 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:82 follow-ups: Changed 4 years ago by dimpase

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.

comment:83 Changed 4 years ago by dimpase

  • Status changed from needs_work to needs_review

comment:84 follow-up: Changed 4 years ago by vbraun

Release or not release, that is the question ;-)

I'm hesitant to ship some half-baked tarball. On the plus side, it does pass on the buildbot....

comment:85 Changed 4 years ago by dimpase

I have followed the master tarball baker's instructions, as close as I could, short of writing my own cookbook, that is. If you really insist I could write a hacky spkg-src, although I would rather not at this point.

comment:86 in reply to: ↑ 84 Changed 4 years ago by roed

Replying to vbraun:

Release or not release, that is the question ;-)

I'm hesitant to ship some half-baked tarball. On the plus side, it does pass on the buildbot....

I understand the hesitancy, especially since this change includes a bunch of additional changes to mpir between 3.0.0 and master. But Skylake + OS X seems like a fairly common combination: it would be good to be able to build Sage again on my machine.

comment:87 in reply to: ↑ 82 Changed 4 years ago by jdemeyer

Replying to dimpase:

I am sure anyone with an idea on how mpir tarballs are made will manage to reproduce this.

That's essentially a tautology. This docs are not for the people "with an idea on how mpir tarballs are made". They are for the people without such an idea.

comment:88 in reply to: ↑ 82 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

Perhaps you have not had a look at the commit I mention? There are files to edit

You mean the files edited on that commit? Most of that seems to be related to changing the version number. That shouldn't be needed here.

scripts to run

You mean autogen.sh? But Bill says (in a comment on that commit) that this might not work...

and it is outdated.

In that case, it can serve even less as documentation.

comment:89 in reply to: ↑ 75 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

I tried created a patch against 3.0.0, but it would have been magabytes in size and a lot of time to create.

Why would running git diff commit1 commit2 take a lot of time?

And why would it be so large? Are there any auto-generated files in the git repo?

comment:90 in reply to: ↑ 89 ; follow-up: Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

I tried created a patch against 3.0.0, but it would have been magabytes in size and a lot of time to create.

Why would running git diff commit1 commit2 take a lot of time?

because many patches created this way do not apply to the 3.0.0 branch. And there was about 80 patches to look at. I have better things to do than manually deal with 80 non-applying patches.

comment:91 Changed 4 years ago by dimpase

And they are big, big, as may of them involve big changes to assembler code, or just new assembler code, not in 3.0.0.

Even my changes to the assembler code run into many thousands of lines (they are almost all about renaming jumplabels, though).

comment:92 in reply to: ↑ 90 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

because many patches created this way do not apply to the 3.0.0 branch.

If you diff against the 3.0.0 branch, they will surely apply.

comment:93 in reply to: ↑ 88 ; follow-up: Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Perhaps you have not had a look at the commit I mention? There are files to edit

You mean the files edited on that commit? Most of that seems to be related to changing the version number. That shouldn't be needed here.

I don't understand. We need to bump up version numbers of the package, and of the libraries built.

scripts to run

You mean autogen.sh? But Bill says (in a comment on that commit) that this might not work...

Oh Holy Jesus, I have done a job. It is a unique one-off job, why do you want a full documentation of the process? Nobody won't need to redo it, ever. A new way of making MPIR releases is underway.

and it is outdated.

In that case, it can serve even less as documentation.

You do not need this documentation, noone needs it. See above.

comment:94 in reply to: ↑ 92 ; follow-up: Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

because many patches created this way do not apply to the 3.0.0 branch.

If you diff against the 3.0.0 branch, they will surely apply.

Whatever. You will not want to review these megabytes.

What is the problem? I do solemnly swear that I do not work for any 3-letter orgs, and I have not sneaked in anything dodgy.

comment:95 in reply to: ↑ 94 Changed 4 years ago by jdemeyer

Replying to dimpase:

What is the problem?

I see at least these issues:

(A) you could have done some wrong in creating the tarball, subtly breaking it somehow. Usually, when I review a ticket creating a custom tarball, then checking the tarball distribution process is part of the review because things can go wrong there.

(B) distros will not be happy to package this unofficial version of MPIR (see ODK D3.10). If the changes only involve Skylake support, then there is no issue here. However, you seem to imply that there are a lot of other unrelated changes too.

comment:96 Changed 4 years ago by fbissey

Distros use gmp not mpir in the general case. Otherwise I may have complained too. Sometimes it is hard to isolate the changes you want, so long as nothing else but skylake om OS X is affected (in that they now build) then this is neutral.

If it changes doctests and behavior, then sure it is not neutral and things become very hard. But it is not the case here I believe.

comment:97 Changed 4 years ago by dimpase

The diff between the MPIR 3.0.0 and the current master is about 700K in size, or which about 50% is various Windows-only stuff and can be ignored, but the rest is still many times bigger than any other Sage patch (and commit by commit it is much bigger, still). Further untangling this to make patch smaller is not something we should spend time on, I think.

This makes (B) pretty much unfixable. As to (A), well, as Volker says, buildbots are happy with the tarball. I attach the git diff between the MPIR master and the tarball (this is naturally only the git-tracked files that show in that very small patch; one naturally can get the rest by running ./autogen)

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

Changed 4 years ago by dimpase

the tarball vs the MPIR master

comment:98 follow-up: Changed 4 years ago by git

  • Commit changed from e9a44b84638822a36ba143f20978cf9f585afe3b to 9764442aa3bddc303a32ccc808c80c96a3130bc4

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

9764442simplified detailed instruction to produce tarball

comment:99 in reply to: ↑ 93 Changed 4 years ago by jdemeyer

Replying to dimpase:

You mean autogen.sh? But Bill says (in a comment on that commit) that this might not work...

Oh Holy Jesus, I have done a job. It is a unique one-off job, why do you want a full documentation of the process?

Why so offensive? You say that you did X and Bill isn't sure whether X will work. Given that the official maintainer of MPIR questions your procedure, I feel that I am right to ask this question.

comment:100 follow-up: Changed 4 years ago by jdemeyer

Anyway, let me try to recreate the tarball (without the patch, we don't need that).

comment:101 Changed 4 years ago by jdemeyer

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:102 in reply to: ↑ 100 ; follow-ups: Changed 4 years ago by dimpase

Replying to jdemeyer:

Anyway, let me try to recreate the tarball (without the patch, we don't need that).

Why? Surely distributions need libraries' versions to be bumped up.

comment:103 Changed 4 years ago by jdemeyer

  • Branch changed from u/dimpase/skylakefix to u/jdemeyer/skylakefix

comment:104 Changed 4 years ago by jdemeyer

  • Commit changed from 9764442aa3bddc303a32ccc808c80c96a3130bc4 to c2384a70ba09c3acb8e8f2b948cba0ea2985a5b1
  • Description modified (diff)
  • Status changed from needs_work to needs_review

New commits:

c2384a7Updated tarball, add spkg-src and fix instructions

comment:105 in reply to: ↑ 98 Changed 4 years ago by jdemeyer

Replying to git:

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

9764442simplified detailed instruction to produce tarball

That wasn't so hard. I put those instructions in a spkg-src script and created a new tarball using that script.

Please review.

comment:106 in reply to: ↑ 102 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

Why? Surely distributions need libraries' versions to be bumped up.

This is answered adequately in 96

comment:107 follow-ups: Changed 4 years ago by dimpase

Please also remove mpir.net/ from the tarball.

comment:108 in reply to: ↑ 106 Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Why? Surely distributions need libraries' versions to be bumped up.

This is answered adequately in 96

not really - we are shipping 350K of diffs to MPIR 3.0.0, not a little skylake/OSX-only fix.

comment:109 in reply to: ↑ 107 Changed 4 years ago by jdemeyer

Replying to dimpase:

Please also remove mpir.net/ from the tarball.

That wasn't in your instructions :-)

comment:110 in reply to: ↑ 107 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

Please also remove mpir.net/ from the tarball.

I don't know what you mean... there is no such file/directory in the tarball.

comment:111 in reply to: ↑ 110 Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Please also remove mpir.net/ from the tarball.

I don't know what you mean... there is no such file/directory in the tarball.

OK, sorry - I actually did it the other way around: removed directories and edited the dist target of the makefile and ran make dist only then. I didn't notice they don't package mpir.net.

comment:112 follow-up: Changed 4 years ago by jdemeyer

Well, I tried to follow your instructions as literally as possible (apart from changing the version number)...

comment:113 in reply to: ↑ 112 Changed 4 years ago by dimpase

Replying to jdemeyer:

Well, I tried to follow your instructions as literally as possible (apart from changing the version number)...

I can only say https://github.com/wbhart/mpir/issues/237

comment:114 in reply to: ↑ 102 ; follow-up: Changed 4 years ago by jdemeyer

Replying to dimpase:

Why? Surely distributions need libraries' versions to be bumped up.

Distributions are not going to ship this tarball anyway. So it is simply not relevant.

comment:115 in reply to: ↑ 114 ; follow-up: Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Why? Surely distributions need libraries' versions to be bumped up.

Distributions are not going to ship this tarball anyway. So it is simply not relevant.

I think it is relevant, as it's simply misleading not to bump up versions in this case.

comment:116 in reply to: ↑ 115 Changed 4 years ago by jdemeyer

Replying to dimpase:

I think it is relevant, as it's simply misleading not to bump up versions in this case.

We typically do not change .so version numbers if we patch. And the difference between adding a patch or using a new tarball is purely technical. So if we don't change versions for patches, we shouldn't change the version number for this either.

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

this is not a typical case. we basically do a prerelease of 3.0.1 here. the difference is too big to be a patch.

comment:118 Changed 4 years ago by wbhart

I think I should have been clearer about two things: Firstly, I (Bill) am not performing all the functions of an official maintainer any more. I am merely accepting pull requests on GitHub, uploading tarballs to the website and answering questions as the community takes over the project. Secondly, I did not promise to "do" a new release. I said that I think a new release is the best way forward and that I am happy to advise on how to accomplish that.

More specifically, I think a patch release to fix some of the issues with the previous release is in order here. This includes all updates to the Windows build files that Brian Gladman has made, the inclusion of mpir.net, which was accidentally left out of the last tarball. And there have also been some additional patches merged to fix various reported issues, which I accepted since the last release.

The new release should be called 3.0.1. There is no reason to go through an alpha/beta cycle for a patch release, so long as everything is passing on Travis/Appveyor? and the bugs that are meant to be fixed are confirmed to be fixed (which appears to be the case).

The steps remaining for a release are: checking that all FSF copyright notices were updated correctly by Dima (i.e. review), update the Changelog, NEWS, Authors, change the version and .so numbers, make the tarballs, test them on at least one Linux machine and ask Brian to test them on a Windows box, and at least make a brief announcement on mpir-devel about the release and what it is for. I will put the tarballs up on the website, update the website and accept any PR's needed.

We cannot do a full (minor) release, since Flint is broken on Windows due to changes that were made to the interface in the last release, and this needs fixing. Also, Jens Nurmann and David Cleaver were working on Broadwell support. I think I have some patches from them that would need to go into a minor release to support Broadwell, but this includes a nontrivial amount of work, since the patches would need to be tested and profiled to check they are faster than what was there (Jens did not have access to a Broadwell machine). Also, there is a slowdown reported by someone that Jens Nurmann tried to fix, but the original person who reported it produced a better fix. I don't know the status of this, since I have not heard anything about it since. But those are the current blockers for a new minor release of MPIR, which is why I would advocate doing a patch release for Sage (update to 3.0.1) instead of a minor release (update to 3.1.0).

Last edited 4 years ago by wbhart (previous) (diff)

comment:119 Changed 4 years ago by dimpase

OK, I am actually OK with Jeroen's version, as he's certainly more stubborn than me. We probably should arrange for another way to resolve this between us, like riding an wielertoerist version of Ronde van Vlaanderen and see who wins ;-) The only complaint is that the tarball name is way too long, the longest in upstream/. For all practical purposes it would be fine to shorten the commit string in PACKAGEVERSION to the first 7 characters.

comment:120 in reply to: ↑ 117 Changed 4 years ago by jdemeyer

Replying to dimpase:

this is not a typical case. we basically do a prerelease of 3.0.1 here.

I don't think that we are in a position to call something a prerelease of 3.0.1. Who knows how that version will look like or whether there will even be such a version.

comment:121 Changed 4 years ago by vbraun

So are we good?

If Bill wants to release a 3.0.1 in the future then we shouldn't produce a 3.0.1 ourselves, this one should be obvious.

comment:122 Changed 4 years ago by dimpase

  • Reviewers changed from David Roe, Andrew Mathas to David Roe, Andrew Mathas, Dima Pasechnik
  • Status changed from needs_review to positive_review

OK, let it have the longest tarball name in Sage history :-)

comment:123 Changed 4 years ago by dimpase

Volker, is this merged?

comment:124 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/skylakefix to c2384a70ba09c3acb8e8f2b948cba0ea2985a5b1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:125 Changed 4 years ago by vbraun

  • Commit c2384a70ba09c3acb8e8f2b948cba0ea2985a5b1 deleted

yes, its all good

Note: See TracTickets for help on using tickets.