-
Notifications
You must be signed in to change notification settings - Fork 453
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
Fix an uninitialized variable in GeographicLib. #1173
base: master
Are you sure you want to change the base?
Conversation
This avoids the tests to fail because an FPE can be randomly triggered.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1173 +/- ##
==========================================
- Coverage 25.12% 24.96% -0.17%
==========================================
Files 170 170
Lines 18337 19283 +946
==========================================
+ Hits 4608 4814 +206
- Misses 13729 14469 +740 ☔ View full report in Codecov by Sentry. |
@bcoconni I'm refreshing my memory regarding the original issue and trying to understand the issue with setting So first off in terms of Python versus JSBSim exe, it looks to me like in both cases for MSC which I'm using to test, that the set of enabled floating point exceptions are the same? jsbsim/python/fpectl/fpectlmodule.cpp Lines 142 to 148 in 0f00b80
Lines 288 to 294 in 0f00b80
Testing a number of scripts I hit a breakpoint here: jsbsim/src/GeographicLib/Geodesic.cpp Line 406 in 0f00b80
Originally, using a debug build and without I then initialized
And checking what jsbsim/src/GeographicLib/Math.cpp Lines 260 to 270 in 0f00b80
So when you say:
Are you seeing examples of I'm assuming when we were seeing this intermittent bug it was because But if we're confident that all modern (?) compilers/platforms have a quiet NaN then wouldn't it be better to go with |
Yes, correct.
In the case of the Ubuntu distribution (which is the one used by the GitHub runners), By chance, this condition was systematically fulfilled at some point of the development of my
No, I have assumed that multiplying a NaN by a number would raise an invalid FPE. And you are entirely right: my assumption is wrong and setting
Agreed, I guess that compilers are allocating
Yes, that's a good point. Added to the fact that I was incorrectly assuming that using |
Fixes the issue #475.
Unlike the option that has been chosen by the GeographicLib project (see geographiclib/geographiclib#33 (comment)), the variablem12x
is set to zero, instead ofMath::NaN
, to avoid raising floating point exceptions.A new floating point exception flag
FE_OVERFLOW
has also been added toJSBSim.cpp
because it is the exception that is triggered by this bug andJSBSim.cpp
was not catching it on Linux. This setting helped debugging the issue while running the scriptc3101.xml
on an Ubuntu distribution.UPDATE: The variable
m12x
is initialized toMath::NaN
as in the GeographicLib project. See the discussion below for details.