fix MPIR's compliance with c++

[dimpase]

with gcc 4.9.2, we hit this bug:

​Header <cstddef> changes The <cstddef> header was updated for C++11 support and this breaks some libraries which misuse macros meant for internal use by GCC only. For instance with GMP versions up to 5.1.3, you may see:

/usr/include/c++/4.9.0/cstddef:51:11: error: ‘::max_align_t’ has not been declared
   using ::max_align_t;

This has hit here: #18198, see also ​sage-devel thread

The workaround suggested by the gcc people appears to work.

[dimpase]

needless to say, after pulling in the change, one has to sage -f mpir (ideally followed up by make - but this will rebuild half of Sage for nothing, apart from an extra check).

[leif]

Is that known upstream?

(I recall we had related discussions, but I don't know whether there's already a similar fix.)

[leif]

Replying to leif:

Is that known upstream?

Ooops, saw the "reported upstream" too late.

But isn't there already an upstream patch (or at least an issue on github)?

[dimpase]

(fixed the link to sage-devel)

my understanding is that upstream is not in hurry to fix this properly, as it's not easy.

[leif]

Hmmm, I at least haven't found any reply from Bill on mpir-devel, nor any related (open or closed) issue on github (wbhart/mpir).

So you should probably send some fix upstream as well (and/or open a new issue on github).

("Developers acknowledge bug" -- I'm not sure upstream is really aware of it such that it'll get fixed in the next release. Bill's pretty busy and there are lots of things he wanted or has to do for the next release IIRC.)

As mpirxx.h includes mpir.h, is it really necessary to touch the former as well?

[dimpase]

Replying to leif:

Hmmm, I at least haven't found any reply from Bill on mpir-devel, nor any related (open or closed) issue on github (wbhart/mpir).

there is a post: ​https://groups.google.com/d/msg/mpir-devel/78Hb2-sGrjQ/1VohfbjBmgUJ (I suppose that no denial means an acknowledgement)

I just opened an issue on github, too: ​https://github.com/wbhart/mpir/issues/153

So you should probably send some fix upstream as well (and/or open a new issue on github).

("Developers acknowledge bug" -- I'm not sure upstream is really aware of it such that it'll get fixed in the next release. Bill's pretty busy and there are lots of things he wanted or has to do for the next release IIRC.)

As mpirxx.h includes mpir.h, is it really necessary to touch the former as well?

the former need to be touched as <cstddef> must be the very 1st include!

[leif]

Replying to dimpase:

Replying to leif:

Hmmm, I at least haven't found any reply from Bill on mpir-devel, nor any related (open or closed) issue on github (wbhart/mpir).

there is a post: ​https://groups.google.com/d/msg/mpir-devel/78Hb2-sGrjQ/1VohfbjBmgUJ (I suppose that no denial means an acknowledgement)

:-) So this ticket will set itself to "positive review" after a while as well...

I just opened an issue on github, too: ​https://github.com/wbhart/mpir/issues/153

Ok.

As mpirxx.h includes mpir.h, is it really necessary to touch the former as well?

the former need to be touched as <cstddef> must be the very 1st include!

Ok, the real problem is that mpirxx.h includes mpir.h too late, but it's not the purpose of this ticket to optimize the order of inclusions in MPIR's headers.

Still, I'd remove the /* for size_t */ when including cstddef. Otherwise looks ok to me, but I can't test this right now.

[leif]

P.S.: I'd also like to see the patches to both files in a single patch with the issue in its filename, especially since SPKG.txt apparently no longer gets updated.

[leif]

Oh, this is weird:

While with the patches to MPIR 4ti2-1.6.2 now "finds" GMP (i.e., MPIR), it doesn't build for me (on Sage 6.6) with GCC 4.8.4 nor 4.9.2, but 5.0.1 (RC1).

In the failing builds, GCC complains about

../../src/groebner/VectorArrayAPI.h:144:1: error: redefinition of 'static void _4ti2_::VectorArrayAPI::convert(const T1&, T2&) [with T1 = __gmp_expr<__mpz_struct [1], __mpz_struct [1]>; T2 = long int]'
 VectorArrayAPI::convert(const mpz_class& v1, _4ti2_int64_t& v2)
 ^
../../src/groebner/VectorArrayAPI.h:130:1: error: 'static void _4ti2_::VectorArrayAPI::convert(const T1&, T2&) [with T1 = __gmp_expr<__mpz_struct [1], __mpz_struct [1]>; T2 = long int]' previously declared here
 VectorArrayAPI::convert(const mpz_class& v1, long int& v2)
 ^

multiple times, where apparently only the last is fatal (= not ignored through Make rules); after the first error other things get built and installed.

This might be related to MPIR as well, but I'm not sure, and I guess you also tested with GCC 4.9.2, so I'm a bit puzzled.

[dimpase]

Replying to leif:

Oh, this is weird:

While with the patches to MPIR 4ti2-1.6.2 now "finds" GMP (i.e., MPIR), it doesn't build for me (on Sage 6.6) with GCC 4.8.4 nor 4.9.2, but 5.0.1 (RC1).

sorry, what is the compiler you are using?

[vbraun]

The patch/patches should be documented in the header (free-form text before the first chunk). Not in the filesystem metadata, and not in other files (like SPKG.txt)

[leif]

Replying to dimpase:

Replying to leif:

Oh, this is weird:

While with the patches to MPIR 4ti2-1.6.2 now "finds" GMP (i.e., MPIR), it doesn't build for me (on Sage 6.6) with GCC 4.8.4 nor 4.9.2, but 5.0.1 (RC1).

sorry, what is the compiler you are using?

FSF GCC 4.8.4, FSF GCC 4.9.2, and the first release candidate of FSF GCC 5.0.1, i.e., no versions from distros / all vanilla.

[leif]

Replying to vbraun:

Not in the filesystem metadata, and not in other files (like SPKG.txt)

Well, a "verbose" filename in addition is also helpful; which files get patched can easily be extracted from the patch, and other patches may modify the same file(s).

[dimpase]

Replying to leif:

Oh, this is weird:

While with the patches to MPIR 4ti2-1.6.2 now "finds" GMP (i.e., MPIR), it doesn't build for me (on Sage 6.6) with GCC 4.8.4 nor 4.9.2, but 5.0.1 (RC1).

In the failing builds, GCC complains about

../../src/groebner/VectorArrayAPI.h:144:1: error: redefinition of 'static void _4ti2_::VectorArrayAPI::convert(const T1&, T2&) [with T1 = __gmp_expr<__mpz_struct [1], __mpz_struct [1]>; T2 = long int]'
 VectorArrayAPI::convert(const mpz_class& v1, _4ti2_int64_t& v2)
 ^
../../src/groebner/VectorArrayAPI.h:130:1: error: 'static void _4ti2_::VectorArrayAPI::convert(const T1&, T2&) [with T1 = __gmp_expr<__mpz_struct [1], __mpz_struct [1]>; T2 = long int]' previously declared here
 VectorArrayAPI::convert(const mpz_class& v1, long int& v2)
 ^

multiple times, where apparently only the last is fatal (= not ignored through Make rules); after the first error other things get built and installed.

This might be related to MPIR as well, but I'm not sure, and I guess you also tested with GCC 4.9.2, so I'm a bit puzzled.

I don't see any errors in my logs/pkg/4ti2.log with Sage's gcc 4.9.2 (neither on several versions of Linux x64 or i386, nor on OSX 10.10.3). Unless you can explain how to reproduce your problem, I don't think I can do anything with this.

[jakobkroeker]

I don't think either that this is connected with MPIR

offtopic: I see the same redefinition error as above when trying to build recent Macaulay2 on Fedora21 64 bit

It must be the case that '_4ti2_HAVE_MPZ_INT64_CONVERSION' for whatever reasons is not defined (and '_4ti2_int64_t' is probably the same as a 'long int' on a 64 bit, causing the redefinition error)

[dimpase]

Replying to jakobkroeker:

I don't think either that this is connected with MPIR

"either"? Which comment are you replying to? It's not clear to me, sorry...

[jakobkroeker]

"either"? Which comment are you replying to? It's not clear to me, sorry...

I just misreaded comment 19, it is probably too late for me, sorry. Drop 'either'.

@leif it seems that the HAVE_MPZ_INT64_CONVERSION check by the configure script fails in case g++ is used without enabling c++ 2011 standard. When using g++ with '-std=c++11' flag (or similar), the HAVE_MPZ_INT64_CONVERSION check should succeed and define '_4ti2_HAVE_MPZ_INT64_CONVERSION'. Please check if that is indeed the case.

[git]

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

​933c5c5

merged patch files, renamed...

[dimpase]

Replying to git:

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

​933c5c5

merged patch files, renamed...

oops, I messed up.

[dimpase]

New commits:

​933c5c5

merged patch files, renamed...

[git]

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

​6785ef2

restored wrongly removed file, and deleted one that should have gone

[dimpase]

fixed the error in the previous commit. Ready for review now.

[leif]

Replying to jakobkroeker:

@leif it seems that the HAVE_MPZ_INT64_CONVERSION check by the configure script fails in case g++ is used without enabling c++ 2011 standard. When using g++ with '-std=c++11' flag (or similar), the HAVE_MPZ_INT64_CONVERSION check should succeed and define '_4ti2_HAVE_MPZ_INT64_CONVERSION'. Please check if that is indeed the case.

FWIW, without setting any CXXFLAGS (just from the logs),
with GCC 4.8:

...
checking whether C++ compiler accepts -std=c++0x... yes
checking whether C++ compiler accepts -ftrapv... yes
checking whether -ftrapv actually seems to work for int... no
checking whether -ftrapv actually seems to work for long long... no
...
checking whether we can use the int32_t and int64_t types... yes
checking whether we can convert between int32_t and mpz_class... no
checking whether we can convert between int64_t and mpz_class... no
...

With GCC 4.9:

...
checking whether C++ compiler accepts -std=c++0x... yes
checking whether C++ compiler accepts -ftrapv... yes
checking whether -ftrapv actually seems to work for int... no
checking whether -ftrapv actually seems to work for long long... no
...
checking whether we can use the int32_t and int64_t types... yes
checking whether we can convert between int32_t and mpz_class... no
checking whether we can convert between int64_t and mpz_class... no
...

With GCC 5.1:

...
checking whether C++ compiler accepts -std=c++0x... yes
checking whether C++ compiler accepts -ftrapv... yes
checking whether -ftrapv actually seems to work for int... yes
checking whether -ftrapv actually seems to work for long long... no
...
checking whether we can use the int32_t and int64_t types... yes
checking whether we can convert between int32_t and mpz_class... yes
checking whether we can convert between int64_t and mpz_class... yes
...

In all cases, the C++ sources are compiled with -std=c++0x by the way, and

$ for v in 4.4 4.8 4.9 5.0 5.1; do echo -n "g++ $v:  "; g++-$v -E -dM -x c++ -std=c++0x /dev/null | grep __cplusplus; done
g++ 4.4:  #define __cplusplus 1
g++ 4.8:  #define __cplusplus 201103L
g++ 4.9:  #define __cplusplus 201103L
g++ 5.0:  #define __cplusplus 201103L
g++ 5.1:  #define __cplusplus 201103L

so there doesn't seem to be any difference (in case the version is checked in the source files).

Haven't digged deeper yet. (Should take a look at the config.logs, and configure.)

[leif]

Replying to leif:

Replying to jakobkroeker:

it seems that the HAVE_MPZ_INT64_CONVERSION check by the configure script fails in case g++ is used without enabling c++ 2011 standard. When using g++ with '-std=c++11' flag (or similar), the HAVE_MPZ_INT64_CONVERSION check should succeed and define '_4ti2_HAVE_MPZ_INT64_CONVERSION'.

As posted, they fail, but for a different reason... (see below)

Haven't digged deeper yet. (Should take a look at the config.logs, and configure.)

Ok, sorry, my bad: I had only rebuilt MPIR with GCC 5.1, and so 4ti2's conversion checks failed with the earlier versions (4.8.4 and 4.9.2) due to a rather unrelated linker error.

Dima, if nobody objects, I think you can set the ticket to positive review. (Although "GCC 4.9 work-around" stills seems a bit misleading, as it's not a compiler bug. ;-) )

[dimpase]

please see an update on ​https://github.com/wbhart/mpir/issues/153