From 94b7a941365ae4ab69273f382b9b1e818fea7172 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 11 Oct 2017 21:44:38 -0700 Subject: [PATCH] Sort out locales for OSL I got quite the education this week, realizing just how much of C++'s I/O functionality implicitly takes the system's "locale" into consideration. Of particular concern is the conversion of text to floating point numeric values and vice versa, for which much C++ functionality will happily swap the role of '.' and, say, ',' (comma) for the decimal mark, depending on the user's country and whether they are savvy enough to set their locale to the classic "C" (essentially US-style) version. On the other hand, who can blame somebody for not blindly accepting US hegemony these days? But dammit, OSL is a programming language, and so '.' has a specific role in numeric constants and ',' does something quite different (comma operator!), and if you start mixing up the two when parsing programs, it will all end in tears. All other major programming languages just adopt this convention as well. Now, if you were writing a full app, you could set the locale to the classic one globally (to that process) upon startup, and then never worry about it. Which we can do for oslc itself. But much of OSL is actually a library that can be embedded into other libraries or apps, so globally setting the locale can mess up UI internationalization or the intentional localization of other app I/O. So we have to handle this deftly throughout. The fix comes in three parts: 1. I've submitted a separate review to OIIO here: https://github.com/OpenImageIO/oiio/pull/1785 which tries to straighten up that library, and in particular the various Strutil and ustring utilities that we liberally use throughout OSL. And patrol some of the places where we directly call the locale-sensitive C++ functions like atof and strtod, and replace them with the now-safe OIIO::Strutil equivalents. We'll be able to count on all this working for OIIO >= 1.8.6. 2. In a variety of places that don't rely on OIIO (for example, various outputs via std::ostringstream), we need to take care to explicitly set the stream's locale to produce the output we expect. 3. For people building OSL against a too-old version of OIIO to contain the fixes described above, when compiling a buffer of OSL source code, do the gross but necessary safeguard of saving the prior locale, globally changing to "C" locale while we compile the OSL, then restoring the locale. And *HOPE* that the surrounding application isn't simultaneously counting on the original locale-based internationalization still being in effect for something it is doing in another thread that's unaware that we've temporarily altered a global setting. That's inherently unsafe, but remember it only affects OSL builds against pre-1.8 OIIO (once that patch is applied on the OIIO side), and only will be symptomatic for systems using a non-. locale (which is, empirically speaking, rare enough that nobody noticed this issue until this week). --- src/liboslcomp/oslcomp.cpp | 1 + src/liboslcomp/osllex.l | 18 ++++++++++++++++-- src/liboslexec/instance.cpp | 1 + src/liboslexec/osolex.l | 25 ++++++++++++++++++++++--- src/liboslexec/runtimeoptimize.cpp | 1 + src/liboslexec/shadingsys.cpp | 1 + src/oslc/oslcmain.cpp | 4 ++++ src/oslinfo/oslinfo.cpp | 4 ++++ 8 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/liboslcomp/oslcomp.cpp b/src/liboslcomp/oslcomp.cpp index 84be90e1d..ba6b28d9b 100644 --- a/src/liboslcomp/oslcomp.cpp +++ b/src/liboslcomp/oslcomp.cpp @@ -651,6 +651,7 @@ OSLCompilerImpl::compile_buffer (string_view sourcecode, m_output_filename = ""; std::ostringstream oso_output; + oso_output.imbue (std::locale::classic()); // force C locale ASSERT (m_osofile == NULL); m_osofile = &oso_output; diff --git a/src/liboslcomp/osllex.l b/src/liboslcomp/osllex.l index cbd8c9815..2f88558a5 100644 --- a/src/liboslcomp/osllex.l +++ b/src/liboslcomp/osllex.l @@ -98,6 +98,7 @@ CPLUSCOMMENT \/\/.*\n #include #include +#include #include "oslcomp_pvt.h" using namespace OSL; @@ -351,13 +352,26 @@ bool OSLCompilerImpl::osl_parse_buffer (const std::string &preprocessed_buffer) { // Thread safety with the lexer/parser - static OIIO::mutex oslcompiler_mutex; - OIIO::lock_guard lock (oslcompiler_mutex); + static std::mutex oslcompiler_mutex; + std::lock_guard lock (oslcompiler_mutex); + +#ifndef OIIO_STRUTIL_HAS_STOF + // Force classic "C" locale for correct '.' decimal parsing. + // N.B. This is not safe in a multi-threaded program where another + // application thread is expecting the native locale to work properly. + // This is not necessary for versions of OIIO that have Strutil::stof, + // and we can remove it entirely when OIIO 1.9 is the minimum. + std::locale oldlocale = std::locale::global (std::locale::classic()); +#endif + osl_switch_to_buffer (osl_scan_string (preprocessed_buffer.c_str())); oslcompiler = this; oslparse (); bool parseerr = error_encountered(); osl_delete_buffer (YY_CURRENT_BUFFER); +#ifndef OIIO_STRUTIL_HAS_STOF + std::locale::global (oldlocale); // Restore the original locale. +#endif return parseerr; } diff --git a/src/liboslexec/instance.cpp b/src/liboslexec/instance.cpp index b424100d4..dec46186c 100644 --- a/src/liboslexec/instance.cpp +++ b/src/liboslexec/instance.cpp @@ -805,6 +805,7 @@ std::string ShaderGroup::serialize () const { std::ostringstream out; + out.imbue (std::locale::classic()); // force C locale out.precision (9); lock_guard lock (m_mutex); for (int i = 0, nl = nlayers(); i < nl; ++i) { diff --git a/src/liboslexec/osolex.l b/src/liboslexec/osolex.l index ea1504e1c..0504c9ddc 100644 --- a/src/liboslexec/osolex.l +++ b/src/liboslexec/osolex.l @@ -243,7 +243,7 @@ namespace pvt { // OSL::pvt OSOReader * OSOReader::osoreader = NULL; -static OIIO::mutex osoread_mutex; +static std::mutex osoread_mutex; @@ -252,7 +252,13 @@ OSOReader::parse_file (const std::string &filename) { // The lexer/parser isn't thread-safe, so make sure Only one thread // can actually be reading a .oso file at a time. - OIIO::lock_guard guard (osoread_mutex); + std::lock_guard guard (osoread_mutex); + + // Force classic "C" locale for correct '.' decimal parsing. + // N.B. This is not safe in a multi-threaded program where another + // application thread is expecting the native locale to work properly. + std::locale oldlocale; // save the previous native locale + std::locale::global (std::locale::classic()); osoin = OIIO::Filesystem::fopen (filename, "r"); if (! osoin) { @@ -271,6 +277,7 @@ OSOReader::parse_file (const std::string &filename) } oso_delete_buffer (YY_CURRENT_BUFFER); fclose (osoin); + std::locale::global (oldlocale); // Restore the original locale. return ok; } @@ -281,7 +288,16 @@ OSOReader::parse_memory (const std::string &buffer) { // The lexer/parser isn't thread-safe, so make sure Only one thread // can actually be reading a .oso file at a time. - OIIO::lock_guard guard (osoread_mutex); + std::lock_guard guard (osoread_mutex); + +#ifndef OIIO_STRUTIL_HAS_STOF + // Force classic "C" locale for correct '.' decimal parsing. + // N.B. This is not safe in a multi-threaded program where another + // application thread is expecting the native locale to work properly. + // This is not necessary for versions of OIIO that have Strutil::stof, + // and we can remove it entirely when OIIO 1.9 is the minimum. + std::locale oldlocale = std::locale::global (std::locale::classic()); +#endif oso_switch_to_buffer (oso_scan_string (buffer.c_str())); osoreader = this; @@ -292,6 +308,9 @@ OSOReader::parse_memory (const std::string &buffer) m_err.error ("Failed parse of preloaded OSO code"); } oso_delete_buffer (YY_CURRENT_BUFFER); +#ifndef OIIO_STRUTIL_HAS_STOF + std::locale::global (oldlocale); // Restore the original locale. +#endif return ok; } diff --git a/src/liboslexec/runtimeoptimize.cpp b/src/liboslexec/runtimeoptimize.cpp index e52784909..3c1aaae4d 100644 --- a/src/liboslexec/runtimeoptimize.cpp +++ b/src/liboslexec/runtimeoptimize.cpp @@ -807,6 +807,7 @@ OSOProcessorBase::const_value_as_string (const Symbol &A) TypeDesc type (A.typespec().simpletype()); int n = type.numelements() * type.aggregate; std::ostringstream s; + s.imbue (std::locale::classic()); // force C locale if (type.basetype == TypeDesc::FLOAT) { for (int i = 0; i < n; ++i) s << (i ? "," : "") << ((const float *)A.data())[i]; diff --git a/src/liboslexec/shadingsys.cpp b/src/liboslexec/shadingsys.cpp index 36f330556..b4dae1a38 100644 --- a/src/liboslexec/shadingsys.cpp +++ b/src/liboslexec/shadingsys.cpp @@ -1596,6 +1596,7 @@ ShadingSystemImpl::getstats (int level) const if (level <= 0) return ""; std::ostringstream out; + out.imbue (std::locale::classic()); // force C locale out << "OSL ShadingSystem statistics (" << (void*)this; out << ") ver " << OSL_LIBRARY_VERSION_STRING << ", LLVM " << OSL_LLVM_FULL_VERSION << "\n"; diff --git a/src/oslc/oslcmain.cpp b/src/oslc/oslcmain.cpp index 9a77e2525..95dd99122 100644 --- a/src/oslc/oslcmain.cpp +++ b/src/oslc/oslcmain.cpp @@ -112,6 +112,10 @@ static OSLC_ErrorHandler default_oslc_error_handler; int main (int argc, const char *argv[]) { + // Globally force classic "C" locale, and turn off all formatting + // internationalization, for the entire oslc application. + std::locale::global (std::locale::classic()); + OIIO::Filesystem::convert_native_arguments (argc, (const char **)argv); std::vector args; diff --git a/src/oslinfo/oslinfo.cpp b/src/oslinfo/oslinfo.cpp index 845cf42da..d6d069f83 100644 --- a/src/oslinfo/oslinfo.cpp +++ b/src/oslinfo/oslinfo.cpp @@ -237,6 +237,10 @@ input_file (int argc, const char *argv[]) int main (int argc, char *argv[]) { + // Globally force classic "C" locale, and turn off all formatting + // internationalization, for the entire oslinfo application. + std::locale::global (std::locale::classic()); + OIIO::Filesystem::convert_native_arguments (argc, (const char **)argv); OIIO::ArgParse ap (argc, (const char **)argv);