-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build+test bn_mp_set_double.c on more platforms #476
Conversation
Not all platforms/environments/architectures that support enough of IEEE 754 for the purposes of mp_set_double() actually support enough to legitimately define __STDC_IEC_559__, so only relying on that is too strict. Fixes #159
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx
mp_set_double.c
Outdated
@@ -3,7 +3,7 @@ | |||
/* LibTomMath, multiple-precision integer library -- Tom St Denis */ | |||
/* SPDX-License-Identifier: Unlicense */ | |||
|
|||
#if defined(__STDC_IEC_559__) || defined(__GCC_IEC_559) | |||
#if defined(__STDC_IEC_559__) || defined(__GCC_IEC_559) || defined(__x86_64__) || defined(_M_X64) || defined(_M_AMD64) || defined(__i386__) || defined(_M_X86) || defined(__aarch64__) || defined(__arm__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe have this in tommath_private.h
instead?
#if defined(__STDC_IEC_559__) || defined(__GCC_IEC_559) || defined(__x86_64__) || defined(_M_X64) || defined(_M_AMD64) || defined(__i386__) || defined(_M_X86) || defined(__aarch64__) || defined(__arm__)
#define MP_HAS_SET_DOUBLE
and check for MP_HAS_SET_DOUBLE
?
after fixing the build this should be fine... |
Why is the build suddenly failing? |
I guess build30866717.log:335 says it all ;-) |
I don't get divide by zero errors, but |
(Ow, that's my part) Cannot repeat. |
Adding some fprintfs:
gives
|
Mmh…interesting. It seems as if the calculation for powers of two failed here. (Or more? Could you do me a favour and comment out the exit-on-failure parts and print the actual radix, too, please?)
Oh, not me? Phew! ;-) But lame jokes aside: that's a really curious one. An uninitialized value somewhere (it worked the second time with the same number and base)? Compiler optimization gone too wild? Can you try a different compiler/optimization? I know I'm a bit demanding here, but as I cannot repeat it, I'm restricted to poking around in the hope to get bitten at some point and be able to analyse the venom. Mmh…interesting, indeed. |
@czurnieden thanks for looking into this! Uninitialized values should be caught by the compiler statically and if not, then by valgrind. I find it weird that this problem suddenly came up. Maybe git bisect could help? |
Well, I have to, it's (mainly) my code!
Yepp, most likely, but as I said: I'm poking in the dark here, cannot assume anything and static/dynamic analysing can fail, too.
We haven't changed much in the last couple of commits, so yes, that could be helpful. @MasterDuke17 please find a commit in develop that worked for you. You don't have to try all, just guess a date and choose a commit or two before that date and use |
Hm, I just tried again with both |
And no complaints from valgrind after building with each compiler. |
Ow, you're killing me! ;-) Nuh, doesn't really matter, shit happens. |
@MasterDuke17 can you please do a |
Works just fine with gcc and clang, no errors. |
And with MSVC? |
Dunno, I don't have a windows machine to test on. |
75e7cb6
to
68ce8c1
Compare
68ce8c1
to
aa02bc6
Compare
aa02bc6
to
c1cf807
Compare
Reported by: Sébastien Santoro <[email protected]> Obtained from: libtom/libtommath#476
any ideas about when this change can be released out (did not see in 1.3.0 release tarball) |
Yeah, I missed this change :-\ Give a few days for other issues to maybe pop up and I'll make a 1.3.1. |
@chenrui333 can you please check whether https://github.com/libtom/libtommath/releases/tag/v1.3.1-rc1 fixes this for you? |
Not all platforms/environments/architectures that support enough of
IEEE 754 for the purposes of mp_set_double() actually support enough
to legitimately define STDC_IEC_559, so only relying on that is
too strict. Fixes #159