bn_mp_set_double.c does not compile under MacOS plus #159
Comments
|
Apple! *sigh*
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
binary32 (FLT) can be done, of course, but a
Also no problem.
Yes, I was painfully made aware of it in #156 ;-) |
|
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. ;-)
|
Well, that can be helped https://stackoverflow.com/questions/54671503/necessary-c-compiler-flags-to-check-macos-old-and-new-for-ieee-754-compliance ;-) |
|
...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?
|
|
@buggywhip Doing runtime checking sounds like a good idea. We could use a small set of correct inputs/outputs as tests. |
if there's no hardware to test on, I'd exclude
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.
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. |
@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 |
|
The function should not protect against invalid data.
Agree.
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.
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. |
|
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. ;-)
|
|
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). |
|
@MasterDuke17 could you paste the failure output here? Compilation failure? |
|
Appveyor (windows): TravisCI (MacOS only, linux is fine): |
|
Yes ok, so the macro checks should be somehow adapted in order to recognize windows and macos. @MasterDuke17 is it possible that you prepare a patch? |
|
My knowledge of the C pre-processor, IEEE754, etc. is very limited, but I'll cobble together something cribbed from the above comments and StackOverflow and see if it works in our CI environments. |
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 libtom#159
|
MoarVM@db0d387 makes our CI happy. |
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
|
closed by #476 |
Under MacOS...
a simple
makereturns......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.
The text was updated successfully, but these errors were encountered: