cosinekitty / astronomy

Astronomy Engine: multi-language calculation of Sun, Moon, and planet positions. Predicts lunar phases, eclipses, transits, oppositions, conjunctions, equinoxes, solstices, rise/set times, and other events. Provides vector and angular coordinate transforms among equatorial, ecliptic, horizontal, and galactic orientations.
MIT License
496 stars 64 forks source link

Difference when built with -Ofast in Linux #245

Closed ebraminio closed 2 years ago

ebraminio commented 2 years ago

Apparently we are hitting something interesting in the C port,

Steps to reproduce:

  1. Save the following file as a.cc
    
    #include <stdio.h>

include "astronomy.h"

int main() { astro_time_t time = Astronomy_TimeFromDays(8301.6); astro_observer_t observer = { .latitude = 83, .longitude = -36, .height = .0 }; astro_search_result_t s = Astronomy_SearchRiseSet(BODY_SUN, observer, DIRECTION_SET, time, 1); printf("%d\n", s.status); return 0; }


2. Run
`gcc a.cc astronomy.c -lm -O3 && ./a.out`
3. Run
`gcc a.cc astronomy.c -lm -Ofast && ./a.out`

Expected:
Both should print 0

Actual:
`-O3` prints 0 but `-Ofast` prints 5.

Interestingly I don't see the issue in macOS with `-Ofast` or `-Ofastest`, I've seen the issue in four different Linux machines and it happens with both `clang` and `gcc` in Linux so perhaps not a compiler issue(!) but somehow platform dependent difference.
cosinekitty commented 2 years ago

My initial research led me to this gcc bug report about the -Ofast option. The C version of Astronomy Engine currently uses NaN as a sentinel value to represent an unknown value. For example, this check for whether sidereal time has yet been calculated in an instance of astro_time_t.

If I am understanding correctly, enabling -Ofast will cause isnan to always return false, meaning sidereal time will never be calculated. This would make it impossible to calculate anything related to the rotation of the Earth.

I will investigate more and report back on this thread later. But for now, I recommend avoiding this optimization because it is so aggressive that it will produce incorrect behavior in Astronomy Engine.

If I can confirm this optimization should not be used, my opinion is the best change to Astronomy Engine would be to detect the optimization and generate a compiler error, rather than allow invalid code to be produced.

cosinekitty commented 2 years ago

I was able to confirm my suspicions about isnan:

$ more nan.c
#include <stdio.h>
#include <math.h>

int main()
{
    double x = NAN;
    int f = isnan(x);
    printf("%d\n", f);
    return 0;
}
$ gcc -o nan nan.c && ./nan
1
$ gcc -o nan nan.c -Ofast && ./nan
0
cosinekitty commented 2 years ago

I made a change in fc21b4c0f17cd6d07f9bfbc1033c5e1286e8bdcf that will cause a compiler error in gcc if someone tries to use -Ofast to build the C version of Astronomy Engine. I recommend using -O3 for optimization, because that is what I use for all my unit tests and demo verification.