Decimal parsing fails on locales using comma as radix

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Set locale to something that uses a decimal comma, e.g. French
2. Use non-integer float literals like 0.4 and 1.8 in a shader

What is the expected output? What do you see instead?
Expected output is for 0.4 to parse as 0.4, but it is parsed as 0.0. 1.8 is 
parsed as 1.0.

What version of the product are you using? On what operating system?
The version in Firefox Minefield nightly 2010-9-11, Windows 7.

Please provide any additional information below.
The reason for the misparsing of floats is that the lexer seems to be using 
atof for string-to-float conversions and atof uses the current locale to 
determine the radix character.

http://code.google.com/p/angleproject/source/browse/trunk/src/compiler/glslang.l
#178

Original issue reported on code.google.com by Ilmari.H...@gmail.com on 11 Sep 2010 at 5:08

[GoogleCodeExporter]

The C standard library functions for converting strings to floats are not 
usable at all here. Indeed, even if you set the C locale before using atof(), 
since this locale is shared among all threads, you still have bugs in 
multi-threaded applications.

This affects not only atof() but also all other C library functions that I know 
of, such as sscanf().

The best solution IMO is to use C++ streams here (e.g. sstringstream). They 
store their locale as non-static member data, so they can each have their own. 
You control that by calling imbue() on them.

Original comment by jacob.be...@gmail.com on 11 Sep 2010 at 1:22

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

There seem to be quite few calls to take care of: no strtoX -> floating point, 
no Xscanf -> floating point, the only atof() calls are:

[bjacob@cahouette ~]$ grep atof -R angleproject-read-only/ | grep -v svn
angleproject-read-only/src/compiler/glslang.l:{D}+{E}           { 
pyylval->lex.line = yylineno; pyylval->lex.f = 
static_cast<float>(atof(yytext)); return(FLOATCONSTANT); }
angleproject-read-only/src/compiler/glslang.l:{D}+"."{D}*({E})? { 
pyylval->lex.line = yylineno; pyylval->lex.f = 
static_cast<float>(atof(yytext)); return(FLOATCONSTANT); }
angleproject-read-only/src/compiler/glslang.l:"."{D}+({E})?     { 
pyylval->lex.line = yylineno; pyylval->lex.f = 
static_cast<float>(atof(yytext)); return(FLOATCONSTANT); }
angleproject-read-only/src/compiler/preprocessor/tokens.c:            
yylvalpp->sc_fval=(float)atof(yylvalpp->symbol_name);

Original comment by jacob.be...@gmail.com on 11 Sep 2010 at 1:28

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

OK, here's a patch. Should fix it all, but I haven't tried compiling so you'll 
probably have to fix some stupid typos.

It defines a extern "C" function atof_no_locale using a C++ stream and imbue().

I didn't know where to put that. So I put it in parseHelper.h / .cpp but since 
that is a C++ header, I couldn't include it from tokens.c so I copied the 
declaration there. You probably want to put it in a C header instead... I just 
didn't know where.

Original comment by jacob.be...@gmail.com on 11 Sep 2010 at 2:23

Attachments:

• angle-fix-atof.diff

[GoogleCodeExporter]

Note that this is probably not very efficient: imbue() has to parse a string!

It would be more efficient to construct a istringstream once and reuse it. But 
that would not play well with having a C file, tokens.c.

Probably the most efficient solution will be to write a C atof_no_locale() from 
scratch.

Original comment by jacob.be...@gmail.com on 11 Sep 2010 at 2:31

[GoogleCodeExporter]

I'd recommend adding a new C file and associated header to the src/compiler 
directory (called util.h/.c for example).   Add the new atof_no_locale function 
to this file, and then it can be used in both the compiler and the preprocessor.

As for the implementation of atof_no_locale, it seems there is already a 
version of atof that takes a specific locale.   This is called _atof_l on 
windows (see http://msdn.microsoft.com/en-us/library/hc25t012(VS.80).aspx) and 
atof_l in osx (see http://www.unix.com/man-page/osx/3/atof_l/) . I'm not sure 
if there is an equivalent for linux.  atof_no_locale should just be a wrapper 
for the appropriate version of this, depending on the platform,  and it can 
just use the C locale.

Hope this helps.
Daniel

Original comment by dan...@transgaming.com on 13 Sep 2010 at 2:20

[GoogleCodeExporter]

Thanks; I think I'll settle for strtod_l since atof is getting deprecated in 
favor of strtod.

I'll patch our copy in Mozilla and I'll send you my patch...

Original comment by jacob.be...@gmail.com on 27 Sep 2010 at 8:41

[GoogleCodeExporter]

Here's the patch!

Original comment by jacob.be...@gmail.com on 3 Oct 2010 at 12:42

Attachments:

• fix-angle-use-of-atof

[GoogleCodeExporter]

The attached patch uses _atof_l and atof_l, not strtod_l as per comment #8.  Is 
this the correct patch?

Thanks,
Daniel

Original comment by dan...@transgaming.com on 5 Oct 2010 at 4:15

[GoogleCodeExporter]

Yes it is, I figured that since I had to call _atof_l on windows anyway, it 
just made sense... Feel free to change it and do test it before pushing it, I'm 
not confident enough in my understanding of the build system.

Original comment by jacob.be...@gmail.com on 5 Oct 2010 at 5:30

[GoogleCodeExporter]

Are you using this patch in mozilla?  Does it work under linux?  I'm a bit 
concerned that atof_l doesn't seem to be part of the BSD standard and that this 
won't compile there.

Original comment by dan...@transgaming.com on 7 Oct 2010 at 12:27

[GoogleCodeExporter]

It's this mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=594489

Not pushed yet, but I was planning to do it tomorrow.

Indeed there was a compile issue on linux. strtod_l seems to work as a 
replacement but i need to set up a system with foreign locales in order to 
reproduce.

I didn't expect BSD problems as the OS X man pages say "BSD", but i'll check. 
We don't have BSD test servers at Mozilla.

Original comment by jacob.be...@gmail.com on 7 Oct 2010 at 12:57

[GoogleCodeExporter]

It turned out to be a bit too tricky to get the non-standard C functions to 
work on all platforms we needed to support. On my linux machine it worked, but 
on our Linux test servers, it failed.

So here's a patch going back to using the standard c++ library instead.

Original comment by jacob.be...@gmail.com on 14 Oct 2010 at 7:17

Attachments:

• fix-angle-use-of-atof.patch

[GoogleCodeExporter]

OK, sorry, previous patch had generated files. Here's a clean patch.

Original comment by jacob.be...@gmail.com on 14 Oct 2010 at 7:36

Attachments:

• fix-angle-use-of-atof

[GoogleCodeExporter]

Original comment by dan...@transgaming.com on 21 Oct 2010 at 1:18

[GoogleCodeExporter]

This is fixed in r470.

Uses _atof_l under visual studio and stream imbue otherwise.  Please verify 
that this works on all target platforms.

Original comment by dan...@transgaming.com on 29 Oct 2010 at 3:15

• Changed state: Fixed

[GoogleCodeExporter]

This should work for us. Already checked for stream imbue, and _atof_l 
shouldn't be a problem with visual studio.

Original comment by jacob.be...@gmail.com on 29 Oct 2010 at 11:33