Silent failure of locale dependent functions

[bcoconni]

I'm submitting a ...

• bug report
• feature request
• support request => Please do not submit support request here, see note at the top of this template.

Describe the issue
As reported by @vranki in the discussion #773, JSBSim can fail reading data if the locale is not set to "C". Functions such as atof() may expect the decimal separator to be a comma , instead of a period . depending on the locale setting.

What is the current behavior?
When the separator does not match the locale setting, data are incorrectly parsed and no error is reported by JSBSim resulting in an unexpected behavior of the flight model.

What is the expected behavior?
JSBSim should report an error when encountering an incorrect syntax while parsing a number.

[bcoconni]

In some places the usage of atof() raises an error when the string does not match the syntax of a number:

jsbsim/src/input_output/FGXMLElement.cpp

Lines 294 to 302 in 9cb22d7

	double number=0;
	if (is_number(trim(attribute)))
	number = atof(attribute.c_str());
	else {
	std::stringstream s;
	s << ReadFrom() << "Expecting numeric attribute value, but got: " << attribute;
	cerr << s.str() << endl;
	throw invalid_argument(s.str());
	}

But in some other places, the invalid number errors are silently ignored by JSBSim:

jsbsim/src/models/flight_control/FGSwitch.cpp

Lines 90 to 94 in 9cb22d7

	if (delay > 0 && is_number(value)) { // If there is a delay, initialize the
	for (unsigned int i=0; i<delay-1; i++) { // delay buffer to the default value
	output_array[i] = atof(value.c_str()); // for the switch if that value is a number.
	}
	}

And in some other places, values are read without checking if the syntax is legal:

jsbsim/src/math/FGFunction.cpp

Lines 601 to 606 in 9cb22d7

	string mean_attr = element->GetAttributeValue("mean");
	string stddev_attr = element->GetAttributeValue("stddev");
	if (!mean_attr.empty())
	mean = atof(mean_attr.c_str());
	if (!stddev_attr.empty())
	stddev = atof(stddev_attr.c_str());

Finally, there is also the case where the data is evaluated differently depending on whether or not JSBSim is interpreting the input data as a number:

jsbsim/src/models/propulsion/FGTurbine.cpp

Lines 488 to 494 in 9cb22d7

	if (tsfcElement) {
	string value = tsfcElement->GetDataLine();
	if (is_number(value))
	TSFC = std::make_unique<FGSimplifiedTSFC>(this, atof(value.c_str()));
	else
	TSFC = std::make_unique<FGFunction>(FDMExec, tsfcElement, to_string(EngineNumber));
	}

[bcoconni]

In addition to adding new error messages, I am wondering if we should force the locale to "C" before calling atof() then restore the locale afterwards ?

Something like:

char* current_locale = setlocale(LC_NUMERIC, "C");
double value = atof(number_string);
setlocale(LC_NUMERIC, current_locale);

I'd guess the performance impact is not a problem since these functions are called only once at the initialization of JSBSim while parsing the XML files of the flight model.

Opinions ?

[seanmcleod]

Yep, I guess one option is as you say to change the locale and change it back whenever we call atof(), so along your lines we replace all our calls to atof() with your suggestion.

double atof_locale_c(const char* string)
{
   char* current_locale = setlocale(LC_NUMERIC, "C");
   double value = atof(number_string);
   setlocale(LC_NUMERIC, current_locale);
   return value;
}

However when looking up some info on atof() I noticed that there is a version that takes a locale argument, _atof_l().

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/atof-atof-l-wtof-wtof-l?view=msvc-170

For Visual C++ it's _atof_l() which typically signifies it's not part of the official C/C++ standard, but I see other references to it without the underscore. So we may need an ifdef to cater for all supported compilers/std libraries.

https://www.codecogs.com/library/computing/c/stdlib.h/atof.php?alias=atof_l

Using this we could create a numeric C locale once and replace all our atof() calls with atof_l() passing this locale.

[seanmcleod]

Oh, something else I noticed when looking at JSBSim's is_number() function is that it is stricter than atof() in that it doesn't allow leading or trailing whitespace while atof() does.

[bcoconni]

Yep, I guess one option is as you say to change the locale and change it back whenever we call atof(), so along your lines we replace all our calls to atof() with your suggestion.

double atof_locale_c(const char* string)
{
   char* current_locale = setlocale(LC_NUMERIC, "C");
   double value = atof(number_string);
   setlocale(LC_NUMERIC, current_locale);
   return value;
}

Yes and we'd need to check errno as atof() sets errno to ERANGE in some cases. Following the spirit of Element::GetAttributeValueAsNumber(), if errno == ERANGE then we should throw.

However when looking up some info on atof() I noticed that there is a version that takes a locale argument, _atof_l().

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/atof-atof-l-wtof-wtof-l?view=msvc-170

For Visual C++ it's _atof_l() which typically signifies it's not part of the official C/C++ standard, but I see other references to it without the underscore. So we may need an ifdef to cater for all supported compilers/std libraries.

https://www.codecogs.com/library/computing/c/stdlib.h/atof.php?alias=atof_l

Using this we could create a numeric C locale once and replace all our atof() calls with atof_l() passing this locale.

Well, I'd say the function atof_locale_c is not that much complicated to deserve being replaced by a non standard library function. Especially if the latter needs some ifdef to be used.

On my side, I have also found strtod which unlike atof has no undefined behavior when the converted value is out of the range of representable values by a double.

https://cplusplus.com/reference/cstdlib/strtod/

The result of the function strtod is still locale dependent however, so we'd still need the locale conversion code as in atof_locale_c.

[bcoconni]

Oh, something else I noticed when looking at JSBSim's is_number() function is that it is stricter than atof() in that it doesn't allow leading or trailing whitespace while atof() does.

That's a good point: the correct usage should be is_number(trim(value)) as is done in Element::GetAttributeValueAsNumber() but has been omitted in the other calls to atof().

[bcoconni]

There is also a question that remains open: is atof() the only function in JSBSim that is locale dependent ?

[seanmcleod]

In terms of strtod yep I noticed some documentation for atof mentioned that atof is, or sort of depecrated and you should rather use strtod.

However I noticed at the time that there is also a strod_l version which takes a locale argument.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strtod-strtod-l-wcstod-wcstod-l?view=msvc-170

The MSVC CRT has them with an underscore which typically means it's not part of the C/C++ standard, but a number of Linux references have the atof_l and strtod_l without a leading underscore. So not sure if MSVC is behind the standard in terms of these functions or whether Linux etc. just don't bother with the leading underscore.

Switching the global locale each time before and after calling atof\strod just seems more heavy handed, but the other issue to consider is that it's setting the global locale for the process, so if there is another thread executing in the process then we could be messing with it's expected locale since it appears to be global as opposed to thread local.

Actually it's looks like at least with MSVC there is a _configthreadlocale function in which you can choose whether it's global or thread local, although again it looks non-standard.

The function _configthreadlocale is used to control whether setlocale affects the locale of all threads in a program or only the locale of the calling thread.

[seanmcleod]

There is also a question that remains open: is atof() the only function in JSBSim that is locale dependent ?

At least, there may be others, printf is also locale dependent, so when outputting data to the user we need to decide whether to respect the user's chosen locale, or whether we consistent in terms of the C locale.

[seanmcleod]

The other option I came across for converting text to floating point values is the C++ function std::from_chars

https://en.cppreference.com/w/cpp/utility/from_chars

Although it's only included from the C++17 standard onwards. I thought I remember a reference in the JSBSim readme.md mentioning the minimum C++ standard that we support, seem to remember it might only be C++14? Or only C++11? But I can't find it in readme.md.

Basically it's locale independent, basically assumes/applies the C locale standard for parsing floating point numbers.

Unlike other parsing functions in C++ and C libraries, std::from_chars is locale-independent, non-allocating, and non-throwing.

Floating-point parsers: Expects the pattern identical to the one used by std::strtod in the default ("C") locale, except that

[seanmcleod]

Talking of the potential set of functions that are locale specific I came across this list when looking up setlocale.

Also note the comment about undefined behavior with multiple threads.

https://en.cppreference.com/w/cpp/locale/setlocale

Because setlocale modifies global state which affects execution of locale-dependent functions, it is undefined behavior to call it from one thread, while another thread is executing any of the following functions: std::fprintf, std::isprint, std::iswdigit, std::localeconv, std::tolower, std::fscanf, std::ispunct, std::iswgraph, std::mblen, std::toupper, std::isalnum, std::isspace, std::iswlower, std::mbstowcs, std::towlower, std::isalpha, std::isupper, std::iswprint, std::mbtowc, std::towupper, std::isblank, std::iswalnum, std::iswpunct, std::setlocale, std::wcscoll, std::iscntrl, std::iswalpha, std::iswspace, std::strcoll, std::wcstod, std::isdigit, std::iswblank, std::iswupper, std::strerror, std::wcstombs, std::isgraph, std::iswcntrl, std::iswxdigit, std::strtod, std::wcsxfrm, std::islower, std::iswctype, std::isxdigit.

[bcoconni]

Switching the global locale each time before and after calling atof\strod just seems more heavy handed, but the other issue to consider is that it's setting the global locale for the process, so if there is another thread executing in the process then we could be messing with it's expected locale since it appears to be global as opposed to thread local.

Oh, right. That is indeed a problem.

The other option I came across for converting text to floating point values is the C++ function std::from_chars

https://en.cppreference.com/w/cpp/utility/from_chars

Although it's only included from the C++17 standard onwards. I thought I remember a reference in the JSBSim readme.md mentioning the minimum C++ standard that we support, seem to remember it might only be C++14? Or only C++11? But I can't find it in readme.md.

JSBSim is using C++14

jsbsim/src/CMakeLists.txt

Lines 6 to 7 in 36dda4f

	set(CMAKE_CXX_STANDARD 14)

FlightGear has already switched to C++17 so we could switch as well. The only drawback I can think of is that we might need to drop the support of older versions of Python (3.7 ?) and older platforms for Linux wheels. Regarding Python 3.7, this is not much of a problem since according to the PEP 517, the Python foundation will drop support of Python 3.7 in June 2023.

Basically it's locale independent, basically assumes/applies the C locale standard for parsing floating point numbers.

Good, that might be our way out.

[bcoconni]

Well, unfortunately from_chars (proposal PR0067R5) for floating point values has a very poor availability :

• GNU C++ library 11.1 and higher is needed on Linux. This version is only available on the most recent Linux distributions (Ubuntu 22.04 is OK but Ubuntu 20.04 is not).
• The feature is not supported on clang C++ library (i.e. MacOSX)
• VS 2019 and higher seem fine albeit it is required to set the flag _HAS_STD_BYTE=0 for MSVC to successfully compile C++17. But that's another story…

I have implemented a POC but it fails on all compilers but Windows/VS2019 and Ubuntu 22.04/g++ 11.3.

[seanmcleod]

Hmm, not very promising, it appears we may need to wait a couple more years to be able to use from_chars.

I wonder how well supported strtod_l is by our current build matrix.

[bcoconni]

I just submitted a PR #799 that uses strtod_l, let me know what you think.

[seanmcleod]

What about this variation? Create the required locale once rather than creating and freeing it on every call to atof_locale_c().

Also fabs() doesn't set errno so no need to capture it etc.

struct CNumericLocale
{
  CNumericLocale()
  {
#ifdef _WIN32
    Locale = _create_locale(LC_NUMERIC, "C");
#else
    Locale = newlocale(LC_NUMERIC_MASK, "C", 0);
#endif
  }

  ~CNumericLocale()
  {
#ifdef _WIN32
    _free_locale(Locale);
#else
    freelocale(Locale);
#endif
  }

#ifdef _WIN32
  _locale_t Locale;
#else
  locale_t Locale;
#endif
};

double atof_locale_c(const std::string& input)
{
  static CNumericLocale numeric_c;

  const char* first = input.c_str();

  // Skip leading whitespaces
  while (isspace(*first)) ++first;
  //Ignoring the leading '+' sign
  if (*first == '+') ++first;

#ifdef _WIN32
  double value = _strtod_l(first, nullptr, numeric_c.Locale);
#else
  double value = strtod_l(first, nullptr, numeric_c.Locale);
#endif

  // Error management
  std::stringstream s;

  if (fabs(value) == HUGE_VAL && errno == ERANGE)
    s << "This number is too large: " << input;
  else if (fabs(value) == 0 && errno == EINVAL)
    s << "Expecting numeric attribute value, but got: " << input;
  else
    return value;

  std::cerr << s.str() << std::endl;
  throw JSBSim::BaseException(s.str());
}

[seanmcleod]

Or with the odd typedef and #define.

#ifdef _WIN32
typedef _locale_t locale_t;
#define freelocale _free_locale
#define strtod_l _strtod_l
#endif

struct CNumericLocale
{
  CNumericLocale()
  {
#ifdef _WIN32
    Locale = _create_locale(LC_NUMERIC, "C");
#else
    Locale = newlocale(LC_NUMERIC_MASK, "C", 0);
#endif
  }

  ~CNumericLocale()
  {
    freelocale(Locale);
  }

  locale_t Locale;
};

double atof_locale_c(const std::string& input)
{
  static CNumericLocale numeric_c;

  const char* first = input.c_str();

  // Skip leading whitespaces
  while (isspace(*first)) ++first;
  //Ignoring the leading '+' sign
  if (*first == '+') ++first;

  double value = strtod_l(first, nullptr, numeric_c.Locale);

  // Error management
  std::stringstream s;

  if (fabs(value) == HUGE_VAL && errno == ERANGE)
    s << "This number is too large: " << input;
  else if (fabs(value) == 0 && errno == EINVAL)
    s << "Expecting numeric attribute value, but got: " << input;
  else
    return value;

  std::cerr << s.str() << std::endl;
  throw JSBSim::BaseException(s.str());
}

[bcoconni]

I like your idea of the CNumericLocale object which meets the RAII principle. This would make the function compatible with exception management.

However I am not so enthusiastic about the usage of a static variable as we are having the issue #666 opened about that very topic.

I'm not sure why we would bother avoiding to create/release a locale_t instance at each call to atof_locale-c ? Have you noticed a slowdown in the execution of JSBSim while using this feature ?

[bcoconni]

Also fabs() doesn't set errno so no need to capture it etc.

Well, while investigating how to use errno for error management, I read in the Linux manpage for errno that it was good practice to save errno just after the function that needs to be monitored so I blindly followed their advice. 😉

[seanmcleod]

Also fabs() doesn't set errno so no need to capture it etc.

Well, while investigating how to use errno for error management, I read in the Linux manpage for errno that it was good practice to save errno just after the function that needs to be monitored so I blindly followed their advice. 😉

In your implementation you call other c-runtime functions like freelocale() after the call to strtod_l() which may set errno (although I haven't double-checked the documentation for freelocale()) which is why I assumed you needed to save it. So that's why I double-checked the fabs() documentation when I made my proposed changes.

[bcoconni]

I just committed an update to the PR #799 per your suggestions. Note that I did not make the variable numeric_c static unlike what you've suggested due to the issue #666 and I don't see any noticeable slowdown on my side with the creation/release of locale_tat each call to atof_locale_c().

In your implementation you call other c-runtime functions like freelocale() after the call to strtod_l() which may set errno (although I haven't double-checked the documentation for freelocale()) which is why I assumed you needed to save it. So that's why I double-checked the fabs() documentation when I made my proposed changes.

Right. So I removed the copy of errno per your suggestion.

However errno must be set to 0 prior to calling strtod_l or we might intercept an error EINVAL that occurred somewhere else while strdtod_l returns a valid value of 0.0. This scenario is not theoretical : it occurred on MacOSX while I was migrating the code from std::from_chars to strtod_l. Simply setting errno = 0 made this error vanish.

======================================================================
ERROR: testDebugLvl (__main__.TestDebugLvl)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/jsbsim/jsbsim/tests/CheckDebugLvl.py", line 45, in testDebugLvl
    'ball_orbit.xml'))
  File "_jsbsim.pyx", line 760, in _jsbsim.FGFDMExec.load_script
_jsbsim.BaseError: Expecting numeric attribute value, but got: 0.0

[seanmcleod]

I'm not sure why we would bother avoiding to create/release a locale_t instance at each call to atof_locale-c ? Have you noticed a slowdown in the execution of JSBSim while using this feature?

Nope, I haven't done any performance testing to compare. Rather more along the lines of if we only require a single instance for the duration of the program why create and free one for every call to atof_locale-c if there is no need to.

But the issue with static variables and multiple threads can be an issue as we noticed with issue 666.

Doing a bit more reading, C++11 makes static local variable initialization thread-safe.

Starting in C++11, a static local variable initialization is guaranteed to be thread-safe. This feature is sometimes called magic statics.

Testing this out with C++14 and MSVC.

#include <iostream>
#include <thread>
#include <locale>

using namespace std;

struct A
{
	A()
	{
		cout << "A Ctor " << this_thread::get_id() << endl;
		Locale = _create_locale(LC_NUMERIC, "C");
	}

	~A()
	{
		cout << "A Dtor " << this_thread::get_id() << endl;
		_free_locale(Locale);
	}

	_locale_t Locale;
};

double dummy_atof(const string& data)
{
	static A local;

	cout << "dummy_atof start " << this_thread::get_id() << endl;

	return atof(data.c_str());
}

void threadfunc()
{
	cout << "threadfunc start " << this_thread::get_id() << endl;

	for (int i = 0; i < 3; i++)
	{
		dummy_atof("23.45");
		this_thread::sleep_for(chrono::milliseconds(rand() % 10));
	}
}

int main()
{
	for (int i = 0; i < 10; i++)
	{
		new thread(threadfunc);
	}

	this_thread::sleep_for(chrono::milliseconds(200));

	return 0;
}

With the following example output.

threadfunc start threadfunc start threadfunc start 60948
threadfunc start 53724
threadfunc start 94180
threadfunc start 12316
A Ctor 60948
threadfunc start 45784
dummy_atof start 45784
dummy_atof start 60948dummy_atof start 12316
threadfunc start 93728
dummy_atof start dummy_atof start 53724
dummy_atof start 94180
threadfunc start 8408093728

threadfunc start 52908
dummy_atof start 45784
46868
dummy_atof start 52908

dummy_atof start dummy_atof start 46868
7832884080

dummy_atof start 78328
dummy_atof start dummy_atof start dummy_atof start dummy_atof start 52908457848408094180
dummy_atof start 78328
dummy_atof start 53724

dummy_atof start 46868
dummy_atof start 12316
dummy_atof start 60948
dummy_atof start 93728
dummy_atof start 84080
dummy_atof start 94180
dummy_atof start 60948
dummy_atof start 93728
dummy_atof start 78328
dummy_atof start 52908
dummy_atof start 12316
dummy_atof start 53724
dummy_atof start 46868
A Dtor 84296

C:\source\temp\ThreadsStatics\x64\Debug\ThreadsStatics.exe (process 85848) exited with code 0.

So the ctor is called once and the destructor is called once during program shutdown.

Now in issue 666 things are a bit different in that the user's program created multiple threads, each creating an instance of JSBSim, and each instance had a reference to the static Messages class. So now you had multiple threads calling methods of the Message class without any thread synchronization.

In this case the ctor gets called once and it's the only method that assigns anything to the single member Locale. All other calls from multiple threads to atof_locale_c() only read from the Locale member.

[seanmcleod]

However errno must be set to 0 prior to calling strtod_l or we might intercept an error EINVAL that occurred somewhere else while strdtod_l returns a valid value of 0.0.

Yep, I noticed that was one possibility, although I thought at the time what are the chances of reading a 0 from the FDM XML file and some previous CRT function had failed with EINVAL 😉

[seanmcleod]

Have you noticed a slowdown in the execution of JSBSim while using this feature?

So I couldn't resist a micro-benchmark 😉

void performancetest1()
{
	auto start = chrono::steady_clock::now();

	for (int i = 0; i < 100000; i++)
	{
		auto Locale = _create_locale(LC_NUMERIC, "C");

		double val = _strtod_l("235.8974", nullptr, Locale);

		_free_locale(Locale);
	}

	auto end = chrono::steady_clock::now();

	cout << "Elapsed time in milliseconds: "
		<< chrono::duration_cast<chrono::milliseconds>(end - start).count()
		<< " ms" << endl;
}

void performancetest2()
{
	auto Locale = _create_locale(LC_NUMERIC, "C");

	auto start = chrono::steady_clock::now();

	for (int i = 0; i < 100000; i++)
	{
		double val = _strtod_l("235.8974", nullptr, Locale);
	}

	auto end = chrono::steady_clock::now();

	_free_locale(Locale);

	cout << "Elapsed time in milliseconds: "
		<< chrono::duration_cast<chrono::milliseconds>(end - start).count()
		<< " ms" << endl;
}


int main()
{
	for (int i = 0; i < 10; i++)
	{
		performancetest1();
		performancetest2();
		cout << endl;
	}

	return 0;
}

Results for MSVC release build.

Elapsed time in milliseconds: 68 ms
Elapsed time in milliseconds: 11 ms

Elapsed time in milliseconds: 96 ms
Elapsed time in milliseconds: 10 ms

Elapsed time in milliseconds: 83 ms
Elapsed time in milliseconds: 15 ms

Elapsed time in milliseconds: 73 ms
Elapsed time in milliseconds: 9 ms

Elapsed time in milliseconds: 67 ms
Elapsed time in milliseconds: 19 ms

Elapsed time in milliseconds: 73 ms
Elapsed time in milliseconds: 9 ms

Elapsed time in milliseconds: 71 ms
Elapsed time in milliseconds: 12 ms

Elapsed time in milliseconds: 85 ms
Elapsed time in milliseconds: 14 ms

Elapsed time in milliseconds: 76 ms
Elapsed time in milliseconds: 11 ms

Elapsed time in milliseconds: 70 ms
Elapsed time in milliseconds: 10 ms

C:\source\temp\ThreadsStatics\x64\Release\ThreadsStatics.exe (process 63112) exited with code 0.

So on average an extra 64.2ms per 100,000 atof calls when allocating and deallocating a locale for each call.

How many atof calls for a typical or large FDM? 1,000 to 10,000? So an extra 0.642ms to 6.42ms in terms of startup time?

[bcoconni]

I ran your test on my PC/Win11 and got a ratio of 1:6 as well (but my PC seems twice as slower than yours 😉)

Elapsed time in milliseconds: 149 ms
Elapsed time in milliseconds: 24 ms

Elapsed time in milliseconds: 155 ms
Elapsed time in milliseconds: 31 ms

Elapsed time in milliseconds: 164 ms
Elapsed time in milliseconds: 26 ms

Elapsed time in milliseconds: 186 ms
Elapsed time in milliseconds: 28 ms

Elapsed time in milliseconds: 144 ms
Elapsed time in milliseconds: 18 ms

Elapsed time in milliseconds: 155 ms
Elapsed time in milliseconds: 34 ms

Elapsed time in milliseconds: 123 ms
Elapsed time in milliseconds: 19 ms

Elapsed time in milliseconds: 191 ms
Elapsed time in milliseconds: 28 ms

Elapsed time in milliseconds: 190 ms
Elapsed time in milliseconds: 29 ms

Elapsed time in milliseconds: 159 ms
Elapsed time in milliseconds: 26 ms

[bcoconni]

So are we ready to commit the last PR of 2022 ? 🥳

[seanmcleod]

So are we ready to commit the last PR of 2022 ? 🥳

Go for it!