bn_mp_set_double.c does not compile under MacOS plus

[buggywhip]

Under MacOS...

$ gcc --version
Apple LLVM version 9.0.0 (clang-900.0.39.2)
Target: x86_64-apple-darwin16.7.0
Thread model: posix

a simple make returns...

   * cc bn_mp_set_double.o
bn_mp_set_double.c:55:6: warning: "mp_set_double implementation is only available on platforms with IEEE754 floating point format" [-W#warnings]
#    warning "mp_set_double implementation is only available on platforms with IEEE754 floating point format"
     ^
1 warning generated.

...resulting in no binary for mp_set_double(). (mp_get_double() compiles fine.)

---

Notes:

• LTM and LTC will not crash if mp_get_double() and mp_set_double() are not compiled. They are not called fm anywhere else in LTM or LTC. ...and only mp_set_double(), demo.c, and mpi.c are conditioned with #if defined(__STDC_IEC_559__) || defined(__GCC_IEC_559)

• Looking at the list of enabled defines in the attached file ( bn_mp_set_double.i.txt ), make note of those with FLT, DBL, and LDBL.

• It appears when Apple's defines __DBL_HAS_DENORM__, __DBL_HAS_INFINITY__, and __DBL_HAS_QUIET_NAN__ are true the processor supports IEEE 754/IEC 60559. Perhaps some combination of these 3 defines could be used to set __STDC_IEC_559__?

• LTM only implements get/set for DBL. Apple supports FLT and LDBL as well. So, for the sake of completeness, perhaps we should also implement mp_get|set_float() and mp_get|set_longdouble()?

• Finally, out of a concern for some misusing mp_get|set_double(), the doc should provide a strong caution about the possible loss of precision.

• FWIW, all Macs since 2006 have Intel processors with integrated IEEE-754 compliant FPUs. ...as do all PPC Macs. M680x0 Mac FPUs varies.

[czurnieden]

Apple! *sigh*

It appears when Apple's defines __DBL_HAS_DENORM__, __DBL_HAS_INFINITY__, and __DBL_HAS_QUIET_NAN__ are true the processor supports IEEE 754/IEC 60559. Perhaps some combination of these 3 defines could be used to set __STDC_IEC_559__?

That shouldn't be much of a problem.

According to https://sourceforge.net/p/predef/wiki/OperatingSystems/ the combination

#if ( (defined macintosh) || (defined  Macintosh) || ((defined __APPLE__) && ( defined __MACH__)) )

should suffice to detect an Apple OS (macros like __DBL_HAS_DENORM__ might have been defined by others, so it is better to check for MacOS first) or are you aware of anything different/additional/more exact?

macintosh and Macintosh are MacOS 9. Exclude from get|set_double?

LTM only implements get/set for DBL. Apple supports FLT and LDBL as well. So, for the sake of completeness, perhaps we should also implement mp_get|set_float() and mp_get|set_longdouble()?

binary32 (FLT) can be done, of course, but a long double has a slightly different format, so an implementation will need a bit more work and hence might take a bit longer.

Finally, out of a concern for some misusing mp_get|set_double(), the doc should provide a strong caution about the possible loss of precision.

Also no problem.

M680x0 Mac FPUs varies.

Yes, I was painfully made aware of it in #156 ;-)
And that didn't even touch the many other, uhm, peculiarities of the M6888[12].

[buggywhip]

It appears when Apple's defines __DBL_HAS_DENORM__, __DBL_HAS_INFINITY__, and __DBL_HAS_QUIET_NAN__ are true the processor supports IEEE 754/IEC 60559. Perhaps some combination of these 3 defines could be used to set __STDC_IEC_559__? That shouldn't be much of a problem. According to https://sourceforge.net/p/predef/wiki/OperatingSystems/ <https://sourceforge.net/p/predef/wiki/OperatingSystems/> the combination #if ( (defined macintosh) || (defined Macintosh) || ((defined __APPLE__) && ( defined __MACH__)) ) should suffice to detect an Apple OS (macros like __DBL_HAS_DENORM__ might have been defined by others, so it is better to check for MacOS first) or are you aware of anything different/additional/more exact?

My gcc configuration on this machine is only capable of compiling -arch i386 and/or -arch x86_64 (default if not specified), and both enable __APPLE__ and __MACH__. Neither macintosh or Macintosh get enabled. I cannot say what gets enabled for -arch ppc or -arch ppc64 or any -m680x0 unless I fire up one of my older PPC machines. Without some additional information I cannot say __APPLE__ and __MACH__ would be sufficient to identify only Intel Macs. I'd feel better if makefile_include.mk were edited to include something like: #if ( (defined __APPLE__) && (defined __MACH__) && ( (defined __DBL_HAS_DENORM__) && ( defined __DBL_HAS_INFINITY__) ) ) #define __STDC_IEC_559__ 1 #endif and omit (defined macintosh) || (defined Macintosh) until such time as we can verify. ...making a comment in the doc noting what is and is not supported.

macintosh and Macintosh are MacOS 9. Exclude from get|set_double?

So yes, for now anyway, I'd exclude. ...an opinion of one. ;-)

[czurnieden]

...an opinion of one. ;-)

Well, that can be helped https://stackoverflow.com/questions/54671503/necessary-c-compiler-flags-to-check-macos-old-and-new-for-ieee-754-compliance ;-)

[buggywhip]

...an opinion of one. ;-) Well, that can be helped https://stackoverflow.com/questions/54671503/necessary-c-compiler-flags-to-check-macos-old-and-new-for-ieee-754-compliance ;-)

Good find! Tx. Look also at https://stackoverflow.com/questions/752309/ensuring-c-doubles-are-64-bits/753018#753018 Alternatively, you can check the predefined constants __DBL_DIG__ (should be 15), __DBL_MANT_DIG__ (should be 53), __DBL_MAX_10_EXP__ (should be 308), __DBL_MAX_EXP__ (should be 1024), __DBL_MIN_10_EXP (should be -307), and __DBL_MIN_EXP__ (should be -1021). These should be available in all flavors of C and C++. Something is not right. We should be more concerned the data is in the right format than simply having a compliant FPU. The function does not do any FP calculations; it simply plays with the word's bits so nothing will fail if we were to run the function on a machine w/o a compliant FPU, or without a FPU for that matter. All that matters is that the function receive a properly formatted double. How that properly formatted double is created should not be our concern. Also, we do some runtime checking but is it sufficient? I'm leaning toward the function always being compiled and part of the library for ALL platforms and that we rely on runtime checking for properly formatted data. I want time to review the situation further. Unfortunately I have some deadlines and cannot get to it for a couple of days. Thots?

[minad]

@buggywhip Doing runtime checking sounds like a good idea. We could use a small set of correct inputs/outputs as tests.
Run the tests once and set a static variable.
In case the tests fail output a message at runtime or abort?
Or just return an error code...

[sjaeckel]

macintosh and Macintosh are MacOS 9. Exclude from get|set_double?

So yes, for now anyway, I'd exclude.

if there's no hardware to test on, I'd exclude

Doing runtime checking sounds like a good idea.

IMO runtime checking sounds like a bad idea as it brings complexity in our code where the toolchain fails to be set-up properly or something else is not 100% compatible (c.f. #156 ) and neither can nor should we fix all those problems on our side.

We could use a small set of correct inputs/outputs as tests.

IMO runtime checks are not "let's try stuff out to determine if it works and if our testcases work by hazard we decide to make this work all the time" but it should be "let's see what kind of CPU we have and read CPU flags and stuff" and if this CPU type doesn't support those operations we provide a fallback scenario.

[minad]

... but it should be "let's see what kind of CPU we have and read CPU flags and stuff"...

@sjaeckel I don't think that would help. Instead we could just continue tweaking the preprocessor checks. Functional runtime checks would avoid adjusting those checks to every platform and compiler. However you are right that it would increase complexity and it is difficult to chose the right set of test inputs.

Since we are discussing those matters for quite some time now - I would really like to use a simpler solution. What about only activating mp_set_double on platforms we know it works? E.g. checking for x86_64 etc. We should also compile the tests only on those platforms. As time goes by more platforms will be whitelisted...

Edit: I suggest #if defined(__x86_64__) || defined(_M_X64) || defined(_M_AMD64) || defined(__i386__) || defined(_M_X86) || defined(__aarch64__) || defined(__arm__) for now.

[buggywhip]

1. Testing for processor and/or FPU type seems pointless because there is nothing to protect. Neither mp_get_double() or mp_set_double() performs FP operations so nothing will blow up. Only the data itself is at issue.

2. Conditioning based on the type and presence of a compliant FPU only increases the likelihood the double will be correctly formed; it cannot guarantee it. What if the double was taken fm a binary file created elsewhere?

3. Not yet mentioned, but there are base 2 and base 10 FP formats to consider. I want to confirm how IEEE 754 handles this but cannot right now. ...hard deadlines, sorry.

4. As previously mentioned, I'm leaning to not conditioning either, but if we condition mp_get_double(), consider conditioning both mp_get|set_double() so as to avoid confusion when one is compiled and the other not.

5. A long shot (but I need time)... One dirt-simple runtime test would be for mp_set_double() to call mp_get_double() and compare the results. It might be the only test required and it should work on all platforms regardless of the type or even presence of a FPU? ...although it might be preferential for a FPU to do the comparison. ???

6. Stepping back for a moment, functions in LTC and LTM do not call either mp_get_double() or mp_set_double() and because there are potential precision issues, I'm struggling with the use case behind these functions in the first place. Why do we even need/want them? Any good use cases?

[minad]

2. Conditioning based on the type and presence of a compliant FPU **only increases the likelihood** the double will be correctly formed; it cannot guarantee it.  What if the double was taken fm a binary file created elsewhere?

The function should not protect against invalid data.

4. As previously mentioned, I'm leaning to not conditioning either, but **if** we condition mp_get_double(), consider conditioning both mp_get|set_double() so as to avoid confusion when one is compiled and the other not.

Agree.

5. A long shot (but I need time)...  One dirt-simple runtime test would be for mp_set_double() to call mp_get_double() and compare the results.   It might be the only test required and it should work on all platforms regardless of the type or even presence of a FPU?  ...although it might be preferential for a FPU to do the comparison.  ???

This would not work only if the transformation would round trip, which is generally not the case. However we are probably not going for runtime tests according to @sjaeckel.

6. Stepping back for a moment, functions in LTC and LTM do not call either mp_get_double() or mp_set_double() and because there are potential precision issues, I'm struggling with the use case behind these functions in the first place.  Why do we even need/want them?  Any good use cases?

LTM is a library. The functions might not be used internally but from the outside world. I wrote the patch adding the functions and I need them. GMP also has similar functions. I think these functions are pretty standard and their existence is expected. Look for old issues in the bugtracker. If you don't want the functions available in your build, you can tweak LTM not to include them via macro trickery.

[buggywhip]

The function should not protect against invalid data.

Then perhaps we need neither conditional compilation or runtime testing and leave it the the app to do its sanity checks? It would make this long discussion kinda moot. ;-)

[MasterDuke17]

I just ran into this with MoarVM. The upgrade to LTM v1.2.0 was just merged, so now I'm trying to take advantage of some of its new capabilities. One of those is mp_(get|set)_double(), but I get build failures in our CI environments (Travis and Appveyor).

[minad]

@MasterDuke17 could you paste the failure output here? Compilation failure?