RFC:Removal of gmp_wrapper.c

[andreasnoack]

Inspired by librandom.jl I have tried to rewrite the gmp interface to avoid gmp_wrapper.c. I can't make it not work on OS X, Ubuntu 32 and 64 bit, but maybe I have missed some memory issues somewhere. I have introduced a limit to the size of the string that can be returned, but it should be easy to change.

See also: #1887 and #2190

[Keno]

LGTM

[aviks]

This causes a segfault in examples/bigfib.jl at the point of converting a large bigint to a string for display.

[ViralBShah]

Is there a way to figure out the string length adaptively - doubling the allocation every time it fails until it is successful?

[aviks]

For larger numbers, it takes order of 10 secs to convert to strings. Doing it adaptively is going to hugely increase the time.

There are gmp functions to write out the string to a file handle. That may be an option.

http://gmplib.org/manual/I_002fO-of-Integers.html#I_002fO-of-Integers

[andreasnoack]

@aviks Thank you for the example. It is a very big number. I think that sizeinbase can do the trick. I'll try to see if I can make it work.

[StefanKarpinski]

Maybe we should move the bigfib example into the tests?

[andreasnoack]

It works. I can add the bigfib to the tests as part of this request.

[aviks]

The example takes around 20 seconds, which might get annoying if you are trying to run the tests often (as you should :) )

[StefanKarpinski]

That's a fair point. Perhaps we need a somewhat smaller version of the test?

[andreasnoack]

I think it works now. The Fibonacci example runs and is included as test in a smaller version. The conversions to strings now use functions that are not warned against in the documentation. In order to get the the float conversion to work, I have changed to scientific notation and changed the tests accordingly. Is that okay?

[aviks]

Looks good to me.

[andreasnoack]

@JeffBezanson Thank you for the comments. I have updated the code.

[ViralBShah]

@andreasnoackjensen If you are ready with this one, please feel free to merge.

[ViralBShah]

While the test works for me if I run it with make test-bigint, it crashes in make testall on the mac.

[ViralBShah]

Ok - this was perhaps some transient stuff, since a couple of updates and rebuilding of sys.ji, it works fine for me.

[andreasnoack]

I cannot provoke it locally but Travis seem to segfault irregularly on the bigint test. Do you think it is a Travis thing or a BigInt thing?

[JeffBezanson]

Valgrind says:

==31938== Invalid read of size 8
==31938==    at 0x2BFCCC4F: __gmpn_copyi (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFC51D3: __gmpn_preinv_mu_div_qr (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFC53BC: mpn_mu_div_qr2 (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFABD32: __gmpn_tdiv_qr (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA63DF: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6419: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6419: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6951: __gmpn_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BF8F252: __gmpz_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFCEE03: __gmp_doprnt (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFD0297: __gmp_snprintf (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x1B5494B7: ???
==31938==  Address 0x14227f98 is 0 bytes after a block of size 92,184 alloc'd
==31938==    at 0x4C2A93D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31938==    by 0x2BF813A8: __gmp_default_allocate (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BF81C70: __gmp_tmp_reentrant_alloc (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFAC42C: __gmpn_tdiv_qr (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA63DF: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6419: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6419: mpn_dc_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFA6951: __gmpn_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BF8F252: __gmpz_get_str (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFCEE03: __gmp_doprnt (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x2BFD0297: __gmp_snprintf (in /home/jeff/src/julia2/julia/usr/lib/libgmp.so.10.1.0)
==31938==    by 0x1B5494B7: ???
==31938== 

This only happens for a sufficiently large bigfib; if you remove a zero it is clean.

[ViralBShah]

We should perhaps plan revert this one today, given that we have a pretty tight window, and we need the build to be in good condition.

@andreasnoackjensen Do you want to see if you can chase this one down or revert it today? This could live on a branch until it is figured out.

[andreasnoack]

I'll look into it and see if can get that string conversion right (or revert).

[JeffBezanson]

The build is consistently passing.

[nolta]

Does b92272a fix the problem?

[JeffBezanson]

I get the same valgrind output. It only happens the first time I do string(bigfib(10000000)) though.

[andreasnoack]

The segfault disappears if the string is assigned before return (I have added the zero again). Please have a look at af3e11d. Any ideas? I cannot get the segfault locally and hence not use valgrind.

[ViralBShah]

If this fixes the issue, it would be worth to pull this in before tagging 0.1. @JeffBezanson should probably try it out with valgrind again.

Also a heads up to @StefanKarpinski

[StefanKarpinski]

Huh?

[ViralBShah]

Since you are going to tag, this issue needs to be resolved - either the suggestion fixes things and is merged, or we need to bring back gmp_wrapper.c again. Doing neither of these and leaving things as they are is probably not a good thing.

[JeffBezanson]

I actually don't know whether the valgrind error is relevant; I've never seen the segfault on my system.

[ViralBShah]

I have never seen the segfault too - but it could happen on debian's build systems if it happened on travis. Hopefully, we should find out quickly if that happens, and do something about it.