Skip to content

Commit 0f33917

Browse files
authored
Sort out locales for OSL (#795)
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: AcademySoftwareFoundation/OpenImageIO#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).
1 parent f785cc2 commit 0f33917

File tree

8 files changed

+50
-5
lines changed

8 files changed

+50
-5
lines changed

src/liboslcomp/oslcomp.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,7 @@ OSLCompilerImpl::compile_buffer (string_view sourcecode,
651651
m_output_filename = "<buffer>";
652652

653653
std::ostringstream oso_output;
654+
oso_output.imbue (std::locale::classic()); // force C locale
654655
ASSERT (m_osofile == NULL);
655656
m_osofile = &oso_output;
656657

src/liboslcomp/osllex.l

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ CPLUSCOMMENT \/\/.*\n
9898
#include <string>
9999

100100
#include <OpenImageIO/thread.h>
101+
#include <OpenImageIO/strutil.h>
101102
#include "oslcomp_pvt.h"
102103

103104
using namespace OSL;
@@ -351,13 +352,26 @@ bool
351352
OSLCompilerImpl::osl_parse_buffer (const std::string &preprocessed_buffer)
352353
{
353354
// Thread safety with the lexer/parser
354-
static OIIO::mutex oslcompiler_mutex;
355-
OIIO::lock_guard lock (oslcompiler_mutex);
355+
static std::mutex oslcompiler_mutex;
356+
std::lock_guard<std::mutex> lock (oslcompiler_mutex);
357+
358+
#ifndef OIIO_STRUTIL_HAS_STOF
359+
// Force classic "C" locale for correct '.' decimal parsing.
360+
// N.B. This is not safe in a multi-threaded program where another
361+
// application thread is expecting the native locale to work properly.
362+
// This is not necessary for versions of OIIO that have Strutil::stof,
363+
// and we can remove it entirely when OIIO 1.9 is the minimum.
364+
std::locale oldlocale = std::locale::global (std::locale::classic());
365+
#endif
366+
356367
osl_switch_to_buffer (osl_scan_string (preprocessed_buffer.c_str()));
357368
oslcompiler = this;
358369
oslparse ();
359370
bool parseerr = error_encountered();
360371
osl_delete_buffer (YY_CURRENT_BUFFER);
372+
#ifndef OIIO_STRUTIL_HAS_STOF
373+
std::locale::global (oldlocale); // Restore the original locale.
374+
#endif
361375
return parseerr;
362376
}
363377

src/liboslexec/instance.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,7 @@ std::string
805805
ShaderGroup::serialize () const
806806
{
807807
std::ostringstream out;
808+
out.imbue (std::locale::classic()); // force C locale
808809
out.precision (9);
809810
lock_guard lock (m_mutex);
810811
for (int i = 0, nl = nlayers(); i < nl; ++i) {

src/liboslexec/osolex.l

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ namespace pvt { // OSL::pvt
243243
244244
245245
OSOReader * OSOReader::osoreader = NULL;
246-
static OIIO::mutex osoread_mutex;
246+
static std::mutex osoread_mutex;
247247
248248
249249
@@ -252,7 +252,13 @@ OSOReader::parse_file (const std::string &filename)
252252
{
253253
// The lexer/parser isn't thread-safe, so make sure Only one thread
254254
// can actually be reading a .oso file at a time.
255-
OIIO::lock_guard guard (osoread_mutex);
255+
std::lock_guard<std::mutex> guard (osoread_mutex);
256+
257+
// Force classic "C" locale for correct '.' decimal parsing.
258+
// N.B. This is not safe in a multi-threaded program where another
259+
// application thread is expecting the native locale to work properly.
260+
std::locale oldlocale; // save the previous native locale
261+
std::locale::global (std::locale::classic());
256262
257263
osoin = OIIO::Filesystem::fopen (filename, "r");
258264
if (! osoin) {
@@ -271,6 +277,7 @@ OSOReader::parse_file (const std::string &filename)
271277
}
272278
oso_delete_buffer (YY_CURRENT_BUFFER);
273279
fclose (osoin);
280+
std::locale::global (oldlocale); // Restore the original locale.
274281
275282
return ok;
276283
}
@@ -281,7 +288,16 @@ OSOReader::parse_memory (const std::string &buffer)
281288
{
282289
// The lexer/parser isn't thread-safe, so make sure Only one thread
283290
// can actually be reading a .oso file at a time.
284-
OIIO::lock_guard guard (osoread_mutex);
291+
std::lock_guard<std::mutex> guard (osoread_mutex);
292+
293+
#ifndef OIIO_STRUTIL_HAS_STOF
294+
// Force classic "C" locale for correct '.' decimal parsing.
295+
// N.B. This is not safe in a multi-threaded program where another
296+
// application thread is expecting the native locale to work properly.
297+
// This is not necessary for versions of OIIO that have Strutil::stof,
298+
// and we can remove it entirely when OIIO 1.9 is the minimum.
299+
std::locale oldlocale = std::locale::global (std::locale::classic());
300+
#endif
285301
286302
oso_switch_to_buffer (oso_scan_string (buffer.c_str()));
287303
osoreader = this;
@@ -292,6 +308,9 @@ OSOReader::parse_memory (const std::string &buffer)
292308
m_err.error ("Failed parse of preloaded OSO code");
293309
}
294310
oso_delete_buffer (YY_CURRENT_BUFFER);
311+
#ifndef OIIO_STRUTIL_HAS_STOF
312+
std::locale::global (oldlocale); // Restore the original locale.
313+
#endif
295314
296315
return ok;
297316
}

src/liboslexec/runtimeoptimize.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -807,6 +807,7 @@ OSOProcessorBase::const_value_as_string (const Symbol &A)
807807
TypeDesc type (A.typespec().simpletype());
808808
int n = type.numelements() * type.aggregate;
809809
std::ostringstream s;
810+
s.imbue (std::locale::classic()); // force C locale
810811
if (type.basetype == TypeDesc::FLOAT) {
811812
for (int i = 0; i < n; ++i)
812813
s << (i ? "," : "") << ((const float *)A.data())[i];

src/liboslexec/shadingsys.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1596,6 +1596,7 @@ ShadingSystemImpl::getstats (int level) const
15961596
if (level <= 0)
15971597
return "";
15981598
std::ostringstream out;
1599+
out.imbue (std::locale::classic()); // force C locale
15991600
out << "OSL ShadingSystem statistics (" << (void*)this;
16001601
out << ") ver " << OSL_LIBRARY_VERSION_STRING
16011602
<< ", LLVM " << OSL_LLVM_FULL_VERSION << "\n";

src/oslc/oslcmain.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,10 @@ static OSLC_ErrorHandler default_oslc_error_handler;
112112
int
113113
main (int argc, const char *argv[])
114114
{
115+
// Globally force classic "C" locale, and turn off all formatting
116+
// internationalization, for the entire oslc application.
117+
std::locale::global (std::locale::classic());
118+
115119
OIIO::Filesystem::convert_native_arguments (argc, (const char **)argv);
116120

117121
std::vector<std::string> args;

src/oslinfo/oslinfo.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,10 @@ input_file (int argc, const char *argv[])
237237
int
238238
main (int argc, char *argv[])
239239
{
240+
// Globally force classic "C" locale, and turn off all formatting
241+
// internationalization, for the entire oslinfo application.
242+
std::locale::global (std::locale::classic());
243+
240244
OIIO::Filesystem::convert_native_arguments (argc, (const char **)argv);
241245

242246
OIIO::ArgParse ap (argc, (const char **)argv);

0 commit comments

Comments
 (0)