Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#18240 closed defect (fixed)

fix MPIR's compliance with c++

Reported by: dimpase Owned by:
Priority: major Milestone: sage-6.7
Component: packages: standard Keywords:
Cc: vbraun, leif, wbhart Merged in:
Authors: Dima Pasechnik Reviewers: Jakob Kroeker, Leif Leonhardy
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 6785ef2 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by 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.

Change History (32)

comment:1 Changed 7 years ago by dimpase

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

comment:2 Changed 7 years ago by dimpase

  • Description modified (diff)

comment:3 Changed 7 years ago by dimpase

  • Summary changed from fix MIPRs compliance with c++ to fix MIPR's compliance with c++

comment:4 Changed 7 years ago by dimpase

  • Summary changed from fix MIPR's compliance with c++ to fix MPIR's compliance with c++

comment:5 Changed 7 years ago by 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).

comment:6 Changed 7 years ago by dimpase

  • Cc vbraun added

comment:7 follow-up: Changed 7 years ago by leif

  • Cc leif wbhart added
  • Component changed from number theory to packages: standard

Is that known upstream?

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

comment:8 in reply to: ↑ 7 Changed 7 years ago by 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)?

comment:9 Changed 7 years ago by dimpase

  • Description modified (diff)

(fixed the link to sage-devel)

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

comment:10 follow-up: Changed 7 years ago by 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?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by 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!

comment:12 in reply to: ↑ 11 Changed 7 years ago by 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.

comment:13 Changed 7 years ago by 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.

comment:14 follow-ups: Changed 7 years ago by 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.

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

comment:16 follow-up: Changed 7 years ago by 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)

comment:17 in reply to: ↑ 15 Changed 7 years ago by 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.

comment:18 in reply to: ↑ 16 Changed 7 years ago by 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).

comment:19 in reply to: ↑ 14 Changed 7 years ago by 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.

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

comment:20 follow-up: Changed 7 years ago by 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)

comment:21 in reply to: ↑ 20 Changed 7 years ago by 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...

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

comment:23 follow-up: Changed 7 years ago by git

  • Commit changed from d358636766bf0abae69bb07f154053180bd14a8a to 933c5c587943027944e226bc3b3b80181f22e967

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

933c5c5merged patch files, renamed...

comment:24 in reply to: ↑ 23 Changed 7 years ago by dimpase

  • Status changed from needs_review to needs_work

Replying to git:

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

933c5c5merged patch files, renamed...

oops, I messed up.

comment:25 Changed 7 years ago by dimpase

New commits:

933c5c5merged patch files, renamed...

comment:26 Changed 7 years ago by git

  • Commit changed from 933c5c587943027944e226bc3b3b80181f22e967 to 6785ef2000d220f511fcf3e9830b05099500ef47

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

6785ef2restored wrongly removed file, and deleted one that should have gone

comment:27 Changed 7 years ago by dimpase

  • Status changed from needs_work to needs_review

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

comment:28 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by 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.)

comment:29 in reply to: ↑ 28 Changed 7 years ago by leif

  • Reviewers set to Jakob Kroeker, Leif Leonhardy

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

comment:30 Changed 7 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:31 Changed 7 years ago by vbraun

  • Branch changed from u/dimpase/18240 to 6785ef2000d220f511fcf3e9830b05099500ef47
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 Changed 7 years ago by dimpase

  • Commit 6785ef2000d220f511fcf3e9830b05099500ef47 deleted
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, in a later stable release.
Note: See TracTickets for help on using tickets.