Skip to content

Commit 8d7fa6c

Browse files
committed
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: 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 2dd1476 commit 8d7fa6c

File tree

12 files changed

+62
-10
lines changed

12 files changed

+62
-10
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: 17 additions & 3 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;
@@ -223,7 +224,7 @@ void preprocess (const char *yytext);
223224

224225

225226
{FLT} {
226-
yylval.f = atof (yytext);
227+
yylval.f = OIIO::Strutil::from_string<float>(yytext);
227228
SETLINE;
228229
return FLOAT_LITERAL;
229230
}
@@ -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_STRTOF
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_STRTOF
373+
std::locale::global (oldlocale); // Restore the original locale.
374+
#endif
361375
return parseerr;
362376
}
363377

src/liboslexec/constfold.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1256,7 +1256,7 @@ DECLFOLDER(constfold_stof)
12561256
if (S.is_constant()) {
12571257
ASSERT (S.typespec().is_string());
12581258
ustring s = *(ustring *)S.data();
1259-
int cind = rop.add_constant ((float) strtod(s.c_str(), NULL));
1259+
int cind = rop.add_constant (Strutil::from_string<float>(s));
12601260
rop.turn_into_assign (op, cind, "const fold stof");
12611261
return 1;
12621262
}

src/liboslexec/dictionary.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,11 @@ Dictionary::dict_value (int nodeID, ustring attribname,
385385
if (type.basetype == TypeDesc::FLOAT) {
386386
r.valueoffset = (int) m_floatdata.size();
387387
for (int i = 0; i < n; ++i) {
388+
#if defined(OIIO_STRUTIL_HAS_STRTOF) || OIIO_VERSION > 10900
389+
float v = Strutil::strtof (val, (char **)&val);
390+
#else
388391
float v = (float) strtod (val, (char **)&val);
392+
#endif
389393
while (isspace(*val) || *val == ',')
390394
++val;
391395
m_floatdata.push_back (v);

src/liboslexec/instance.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -787,6 +787,7 @@ std::string
787787
ShaderGroup::serialize () const
788788
{
789789
std::ostringstream out;
790+
out.imbue (std::locale::classic()); // force C locale
790791
out.precision (9);
791792
lock_guard lock (m_mutex);
792793
for (int i = 0, nl = nlayers(); i < nl; ++i) {

src/liboslexec/opstring.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ osl_stoi_is (const char *str)
111111
OSL_SHADEOP float
112112
osl_stof_fs (const char *str)
113113
{
114-
return str ? (float)strtod(str, NULL) : 0.0f;
114+
return str ? Strutil::from_string<float>(str) : 0.0f;
115115
}
116116

117117
OSL_SHADEOP const char *

src/liboslexec/osolex.l

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,7 @@ using namespace OSL::pvt;
200200
}
201201
202202
{FLT} {
203-
yylval.f = atof (yytext);
204-
// std::cerr << "lex float " << yylval.f << "\n";
203+
yylval.f = OIIO::Strutil::from_string<float>(yytext);
205204
return FLOAT_LITERAL;
206205
}
207206
@@ -243,7 +242,7 @@ namespace pvt { // OSL::pvt
243242
244243
245244
OSOReader * OSOReader::osoreader = NULL;
246-
static OIIO::mutex osoread_mutex;
245+
static std::mutex osoread_mutex;
247246
248247
249248
@@ -252,7 +251,13 @@ OSOReader::parse_file (const std::string &filename)
252251
{
253252
// The lexer/parser isn't thread-safe, so make sure Only one thread
254253
// can actually be reading a .oso file at a time.
255-
OIIO::lock_guard guard (osoread_mutex);
254+
std::lock_guard<std::mutex> guard (osoread_mutex);
255+
256+
// Force classic "C" locale for correct '.' decimal parsing.
257+
// N.B. This is not safe in a multi-threaded program where another
258+
// application thread is expecting the native locale to work properly.
259+
std::locale oldlocale; // save the previous native locale
260+
std::locale::global (std::locale::classic());
256261
257262
osoin = OIIO::Filesystem::fopen (filename, "r");
258263
if (! osoin) {
@@ -271,6 +276,7 @@ OSOReader::parse_file (const std::string &filename)
271276
}
272277
oso_delete_buffer (YY_CURRENT_BUFFER);
273278
fclose (osoin);
279+
std::locale::global (oldlocale); // Restore the original locale.
274280
275281
return ok;
276282
}
@@ -281,7 +287,16 @@ OSOReader::parse_memory (const std::string &buffer)
281287
{
282288
// The lexer/parser isn't thread-safe, so make sure Only one thread
283289
// can actually be reading a .oso file at a time.
284-
OIIO::lock_guard guard (osoread_mutex);
290+
std::lock_guard<std::mutex> guard (osoread_mutex);
291+
292+
#ifndef OIIO_STRUTIL_HAS_STRTOF
293+
// Force classic "C" locale for correct '.' decimal parsing.
294+
// N.B. This is not safe in a multi-threaded program where another
295+
// application thread is expecting the native locale to work properly.
296+
// This is not necessary for versions of OIIO that have Strutil::stof,
297+
// and we can remove it entirely when OIIO 1.9 is the minimum.
298+
std::locale oldlocale = std::locale::global (std::locale::classic());
299+
#endif
285300
286301
oso_switch_to_buffer (oso_scan_string (buffer.c_str()));
287302
osoreader = this;
@@ -292,6 +307,9 @@ OSOReader::parse_memory (const std::string &buffer)
292307
m_err.error ("Failed parse of preloaded OSO code");
293308
}
294309
oso_delete_buffer (YY_CURRENT_BUFFER);
310+
#ifndef OIIO_STRUTIL_HAS_STRTOF
311+
std::locale::global (oldlocale); // Restore the original locale.
312+
#endif
295313
296314
return ok;
297315
}

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
@@ -1595,6 +1595,7 @@ ShadingSystemImpl::getstats (int level) const
15951595
if (level <= 0)
15961596
return "";
15971597
std::ostringstream out;
1598+
out.imbue (std::locale::classic()); // force C locale
15981599
out << "OSL ShadingSystem statistics (" << (void*)this;
15991600
out << ") ver " << OSL_LIBRARY_VERSION_STRING
16001601
<< ", 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);

src/testshade/testshade.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,11 @@ action_param (int argc, const char *argv[])
347347
// whole string.
348348
if ((type == TypeDesc::UNKNOWN || type == TypeDesc::TypeFloat)) {
349349
char *endptr = NULL;
350+
#if defined(OIIO_STRUTIL_HAS_STRTOF) || OIIO_VERSION > 10900
351+
float fval = OIIO::Strutil::strtof(stringval.c_str(),&endptr);
352+
#else
350353
float fval = (float) strtod(stringval.c_str(),&endptr);
354+
#endif
351355
if (endptr && *endptr == 0) {
352356
params.push_back (ParamValue());
353357
params.back().init (paramname, TypeDesc::TypeFloat, 1, &fval);

0 commit comments

Comments
 (0)