fix MPIR's compliance with c++

[dimpase]

with gcc 4.9.2, we hit this bug:

Header changes The 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.

Upstream: Fixed upstream, in a later stable release.

CC: @vbraun @nexttime @wbhart

Component: packages: standard

Author: Dima Pasechnik

Branch: 6785ef2

Reviewer: Jakob Kroeker, Leif Leonhardy

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

[dimpase]

Branch: u/dimpase/18240

[dimpase]

Commit: d358636

[dimpase]

Author: Dima Pasechnik

[dimpase]

Description changed:

--- 
+++ 
@@ -10,4 +10,4 @@

 This has hit here: #18198, see also [sage-devel thread](https://gcc.gnu.org/gcc-4.9/porting_to.html)

-The workaround suggested appears to work. A patch follows shortly.
+The workaround suggested by the gcc people appears to work. 

[dimpase]

comment:5

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

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:7

Is that known upstream?

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

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:8

Replying to @nexttime:

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]

Description changed:

--- 
+++ 
@@ -8,6 +8,6 @@
    using ::max_align_t;

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

The workaround suggested by the gcc people appears to work.

[dimpase]

comment:9

(fixed the link to sage-devel)

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

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:10

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]

comment:11

Replying to @nexttime:

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!

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:12

Replying to @dimpase:

Replying to @nexttime:

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.

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:13

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.

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:14

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]

comment:15

Replying to @nexttime:

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]

comment:16

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)

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:17

Replying to @dimpase:

Replying to @nexttime:

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.

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:18

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]

comment:19

Replying to @nexttime:

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.

[ea1d0bf8-c27a-4548-8cb7-de0b1d02441a]

comment:20

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]

comment:21

Replying to @sagetrac-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...

[ea1d0bf8-c27a-4548-8cb7-de0b1d02441a]

comment:22

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

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

933c5c5

merged patch files, renamed...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d358636 to 933c5c5

[dimpase]

comment:24

Replying to @sagetrac-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...

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6785ef2

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 933c5c5 to 6785ef2

[dimpase]

comment:27

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

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:28

Replying to @sagetrac-jakobkroeker:

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

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:29

Replying to @nexttime:

Replying to @sagetrac-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. ;-) )

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

Reviewer: Jakob Kroeker, Leif Leonhardy

[vbraun]

Changed branch from u/dimpase/18240 to 6785ef2

[dimpase]

Changed commit from 6785ef2 to none

[dimpase]

comment:32

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

[dimpase]

Changed upstream from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.