fix MPIR's compliance with c++ #18240

dimpase · 2015-04-17T14:12:32Z

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

The text was updated successfully, but these errors were encountered:

dimpase · 2015-04-17T18:10:24Z

Branch: u/dimpase/18240

dimpase · 2015-04-17T18:10:24Z

Commit: d358636

dimpase · 2015-04-17T18:10:24Z

Author: Dima Pasechnik

dimpase · 2015-04-17T18:16:04Z

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

nexttime · 2015-04-19T15:35:52Z

comment:7

Is that known upstream?

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

nexttime · 2015-04-19T15:38:17Z

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 · 2015-04-19T18:27:19Z

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.

nexttime · 2015-04-19T18:42:43Z

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 · 2015-04-19T18:48:13Z

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: wbhart/mpir#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!

nexttime · 2015-04-19T19:37:01Z

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: wbhart/mpir#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.

nexttime · 2015-04-19T20:38:09Z

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.

nexttime · 2015-04-19T21:45:57Z

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 · 2015-04-19T23:05:29Z

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 · 2015-04-19T23:14:45Z

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)

nexttime · 2015-04-19T23:15:36Z

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.

nexttime · 2015-04-19T23:20:13Z

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 · 2015-04-20T07:19:32Z

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.

sagetrac-jakobkroeker · 2015-04-24T23:43:01Z

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 · 2015-04-25T00:18:36Z

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

sagetrac-jakobkroeker · 2015-04-25T00:31:41Z

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.

sagetrac-git · 2015-04-25T00:36:56Z

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

`933c5c5`	`merged patch files, renamed...`

sagetrac-git · 2015-04-25T00:36:56Z

Changed commit from d358636 to 933c5c5

dimpase · 2015-04-25T00:46:29Z

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 · 2015-04-25T05:54:17Z

New commits:

`933c5c5`	`merged patch files, renamed...`

sagetrac-git · 2015-04-25T06:04:13Z

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

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

sagetrac-git · 2015-04-25T06:04:13Z

Changed commit from 933c5c5 to 6785ef2

dimpase · 2015-04-25T06:08:44Z

comment:27

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

nexttime · 2015-04-25T15:34:16Z

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

nexttime · 2015-04-25T16:46:50Z

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

nexttime · 2015-04-25T16:46:50Z

Reviewer: Jakob Kroeker, Leif Leonhardy

vbraun · 2015-04-26T02:21:48Z

Changed branch from u/dimpase/18240 to 6785ef2

dimpase · 2015-06-10T09:20:28Z

Changed commit from 6785ef2 to none

dimpase · 2015-06-10T09:20:28Z

comment:32

please see an update on wbhart/mpir#153

dimpase · 2015-06-10T09:20:28Z

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

dimpase added this to the sage-6.7 milestone Apr 17, 2015

dimpase added c: number theory labels Apr 17, 2015

dimpase added the s: needs review label Apr 17, 2015

This comment has been minimized.

Sign in to view

dimpase changed the title ~~fix MIPRs compliance with c++~~ fix MIPR's compliance with c++ Apr 17, 2015

dimpase changed the title ~~fix MIPR's compliance with c++~~ fix MPIR's compliance with c++ Apr 17, 2015

nexttime mannequin added c: packages: standard and removed c: number theory labels Apr 19, 2015

This comment has been minimized.

Sign in to view

dimpase added s: needs work and removed s: needs review labels Apr 25, 2015

dimpase added s: needs review and removed s: needs work labels Apr 25, 2015

dimpase added s: positive review and removed s: needs review labels Apr 25, 2015

vbraun removed the s: positive review label Apr 26, 2015

vbraun closed this as completed in 5b5a4c0 Apr 26, 2015

This was referenced May 19, 2015

4ti2 spkg fails to find gmp #18198

Closed

Upgrade 4ti2 package to version 1.6.3 #18324

Closed

fix MPIR's compliance with c++ #18240

fix MPIR's compliance with c++ #18240

Comments

dimpase commented Apr 17, 2015

dimpase commented Apr 17, 2015

dimpase commented Apr 17, 2015

dimpase commented Apr 17, 2015

This comment has been minimized.

dimpase commented Apr 17, 2015

nexttime mannequin commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

This comment has been minimized.

dimpase commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

dimpase commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

dimpase commented Apr 19, 2015

vbraun commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

nexttime mannequin commented Apr 19, 2015

dimpase commented Apr 20, 2015

sagetrac-jakobkroeker mannequin commented Apr 24, 2015

dimpase commented Apr 25, 2015

sagetrac-jakobkroeker mannequin commented Apr 25, 2015

sagetrac-git mannequin commented Apr 25, 2015

sagetrac-git mannequin commented Apr 25, 2015

dimpase commented Apr 25, 2015

dimpase commented Apr 25, 2015

sagetrac-git mannequin commented Apr 25, 2015

sagetrac-git mannequin commented Apr 25, 2015

dimpase commented Apr 25, 2015

nexttime mannequin commented Apr 25, 2015

nexttime mannequin commented Apr 25, 2015

nexttime mannequin commented Apr 25, 2015

vbraun commented Apr 26, 2015

dimpase commented Jun 10, 2015

dimpase commented Jun 10, 2015

dimpase commented Jun 10, 2015