Opened 5 years ago

Closed 5 years ago

#19280 closed defect (fixed)

MPIR gives incorrect result on 32-bit machines

Reported by: cheuberg Owned by:
Priority: critical Milestone: sage-6.10
Component: packages: standard Keywords: mpir
Cc: fredrik.johansson, wbhart, leif Merged in:
Authors: Clemens Heuberger Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: f780291 (Commits) Commit: f780291ccb0363c69411dfe723de795f2c75093b
Dependencies: Stopgaps:

Description (last modified by cheuberg)

In #18546, a bug in mpir only affecting 32 bit machines was described.

 #include "mpir.h"

int main()
{

    mpz_t one, x, w;
    mpz_init(one);
    mpz_init(x);
    mpz_init(w);
    mpz_set_str(one, "62165404551223330269422781018352605012557018849668464680057997111644937126566671941632", 10);
    mpz_set_str(x, "39623752663112484341451587580", 10);

    mpz_tdiv_q(w, one, x);
    gmp_printf("%Zd\n", w);

}

outputs 1568892406419848332919986945754866320479099155002496784035 instead of the correct 1568892403497558507879962225846103176600476845510570267609.

Change History (25)

comment:1 Changed 5 years ago by cheuberg

I'll try to implement the patch proposed in #18546, but compilation on my 32 bit platform is slow ...

comment:2 Changed 5 years ago by cheuberg

  • Branch set to u/cheuberg/mpir-bug

comment:3 Changed 5 years ago by cheuberg

  • Authors set to Clemens Heuberger
  • Commit set to b5049f672d39e857cded703259322be0bb99817f
  • Status changed from new to needs_review

Here is a patch that adds a test to mpir's test suite and then patches all gmp-mparam.h files for the x86 architecture as proposed in #18546.

I hope that one day, the problem will be solved in mpir and we can remove these patches.

For testing, you might want to first check out commit f1b4dff and run sage -i -f -c mpir (or sage -p -c mpir in more recent versions) to see the test failing on x86. Then checking out this branch fully, the problem should disappear.


New commits:

f1b4dffTrac #19280: add test to mpir's test suite
b5049f6Trac #19280: add fix to all x86 gmp-mparam

comment:4 Changed 5 years ago by git

  • Commit changed from b5049f672d39e857cded703259322be0bb99817f to 0d48fb57205cb16d58380c9b022c39eb354c3a58

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

be00675Trac #19280: increase patch level
0d48fb5Trac #19280: Add header to patch files

comment:5 follow-up: Changed 5 years ago by jdemeyer

Instead of patching all those tuning files, why not just

  • gmp-impl.h

    diff -ru mpir-2.7.0/gmp-impl.h mpir-2.7.0-fixed/gmp-impl.h
    old new  
    20402040#endif
    20412041
    20422042#ifndef SB_DIVAPPR_Q_SMALL_THRESHOLD
    2043 #define SB_DIVAPPR_Q_SMALL_THRESHOLD 11
     2043#define SB_DIVAPPR_Q_SMALL_THRESHOLD 0
    20442044#endif
    20452045
    20462046#ifndef SB_DIV_QR_SMALL_THRESHOLD
    2047 #define SB_DIV_QR_SMALL_THRESHOLD 11
     2047#define SB_DIV_QR_SMALL_THRESHOLD 0
    20482048#endif
    20492049
    20502050#ifndef DC_DIVAPPR_Q_THRESHOLD

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by cheuberg

Replying to jdemeyer:

Instead of patching all those tuning files, why not just

  • gmp-impl.h

    diff -ru mpir-2.7.0/gmp-impl.h mpir-2.7.0-fixed/gmp-impl.h
    old new  
    20402040#endif
    20412041
    20422042#ifndef SB_DIVAPPR_Q_SMALL_THRESHOLD
    2043 #define SB_DIVAPPR_Q_SMALL_THRESHOLD 11
     2043#define SB_DIVAPPR_Q_SMALL_THRESHOLD 0
    20442044#endif
    20452045
    20462046#ifndef SB_DIV_QR_SMALL_THRESHOLD
    2047 #define SB_DIV_QR_SMALL_THRESHOLD 11
     2047#define SB_DIV_QR_SMALL_THRESHOLD 0
    20482048#endif
    20492049
    20502050#ifndef DC_DIVAPPR_Q_THRESHOLD

Are we sure that we want to change the parameter on all architectures?

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

You could easily guard the default threshold to be chosen with in addition __i386__ (or some GMP/MPIR equivalent).

comment:8 in reply to: ↑ 7 Changed 5 years ago by cheuberg

Replying to leif:

You could easily guard the default threshold to be chosen with in addition __i386__ (or some GMP/MPIR equivalent).

Could you help me finding the GMP/MPIR equivalent? Thanks.

comment:9 follow-up: Changed 5 years ago by leif

Not ad hoc, otherwise I would have been more precise... ;-)

FWIW, __i386__ is slightly compiler-dependent, so would suffice for our purpose (i.e., GCC).

comment:10 in reply to: ↑ 9 Changed 5 years ago by leif

Replying to leif:

Not ad hoc, otherwise I would have been more precise... ;-)

FWIW, __i386__ is slightly compiler-dependent, so would suffice for our purpose (i.e., GCC).

Since most of the architecture-specific stuff is managed via different files and symlinks to them created by configure, there's no straight-forward "MPIR equivalent".

We could use defined(__i386__) || defined(__i386) (the latter is used by at least M$), and probably to be triple-safe also check whether GMP_LIMB_BITS is really 32 (I vaguely remember someTM ill compiler used to define both __i386__ and __x86_64__ on x86_64, but I'm not sure).

I don't think it really matters as long as our fix works for the architectures supported by Sage, as we're not going to submit the patch upstream.

comment:11 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to cheuberg:

Are we sure that we want to change the parameter on all architectures?

Are you sure that you don't want to change the parameter on all architectures? Why do you think that this bug affects only i386?

comment:12 follow-up: Changed 5 years ago by cheuberg

I don't know anything about it, I just followed the recommendation for a "temporary workaround" by William Hart made in #18546.

The particular problem caught by chance in #18546 was present only on i386.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by jdemeyer

Replying to cheuberg:

I don't know anything about it, I just followed the recommendation for a "temporary workaround" by William Hart made in #18546.

Exactly, and I saw no indication at all in William Hart's comments that this bug happens only on i386. His post talks about "32 bit machines" in general.

I like simple solutions, so I would just apply the workaround in all cases. If you want to, you could only apply it on 32 bit systems.

comment:14 in reply to: ↑ 13 Changed 5 years ago by leif

Replying to jdemeyer:

Replying to cheuberg:

I don't know anything about it, I just followed the recommendation for a "temporary workaround" by William Hart made in #18546.

Exactly, and I saw no indication at all in William Hart's comments that this bug happens only on i386. His post talks about "32 bit machines" in general.

Can you reproduce it on moufang, say?


I like simple solutions, so I would just apply the workaround in all cases.

Then you might also want to change the ticket's title... B)

comment:15 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from MPIR gives incorrect result on i386 to MPIR gives incorrect result on 32-bit machines

comment:16 Changed 5 years ago by leif

The example in the description itself has a bug. ;-)

Either use

gmp_printf("%Zd / %Zd = %Zd\n", one, x, w);

or remove one and x.

comment:17 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Upstream answered about 5:

Changing the default value to 0 for those two values is probably a reasonable thing to do. Please do that.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by leif

Replying to jdemeyer:

Upstream answered about 5:

Changing the default value to 0 for those two values is probably a reasonable thing to do. Please do that.

Meaning what? #undef and re#define unconditionally instead? (Only) if GMP_LIMB_BITS==32?

Last edited 5 years ago by leif (previous) (diff)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 5 years ago by jdemeyer

Replying to leif:

Replying to jdemeyer:

Upstream answered about 5:

Changing the default value to 0 for those two values is probably a reasonable thing to do. Please do that.

It's about the default value, so no #undef needed. Just apply 5.

comment:20 Changed 5 years ago by git

  • Commit changed from 0d48fb57205cb16d58380c9b022c39eb354c3a58 to f780291ccb0363c69411dfe723de795f2c75093b

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

0f4e373Revert "Trac #19280: add fix to all x86 gmp-mparam"
f780291Trac #19280: Use patch proposed in http://trac.sagemath.org/ticket/19280#comment:5

comment:21 in reply to: ↑ 19 Changed 5 years ago by cheuberg

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Replying to leif:

Replying to jdemeyer:

Upstream answered about 5:

Changing the default value to 0 for those two values is probably a reasonable thing to do. Please do that.

It's about the default value, so no #undef needed. Just apply 5.

Done. Thank you for your enquiries and comments.

comment:22 Changed 5 years ago by cheuberg

  • Description modified (diff)

comment:23 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-6.9 to sage-6.10
  • Reviewers set to Jeroen Demeyer

I am going to test this + arb on a 32-bit machine.

comment:24 Changed 5 years ago by jdemeyer

  • Priority changed from major to critical
  • Status changed from needs_review to positive_review

comment:25 Changed 5 years ago by vbraun

  • Branch changed from u/cheuberg/mpir-bug to f780291ccb0363c69411dfe723de795f2c75093b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.