IGES import fails in german environment

Digging with gdb through the code I found the reason for the described behaviour. At least in IGESData_ParamReader.cxx atof() is used, but LC_NUMERIC is not changed to "C".

Steps to reproduce false conversion of reals with atof():
1) take Attachment 2 (CPP) and compile to i.e. a.out and place hammer.igs in the same directory
2) gdb --args a.out hammer.igs
3) set breakpoint to src/IGESData/IGESData_IGESReaderTool.cxx:131 with condition num == 3
4) set breakpoint to src/IGESData/IGESData_ParamReader.cxx:906 with condition i == 13 and disable it
5) set breakpoint to src/IGESData/IGESData_ParamReader.cxx:1235 and disable it
6) run
7) enable once breakpoint from step 4 and continue
8) enable once breakpoint from step 5 and continue
9) print text
10) next
11) print val

text should be something like "-2.220446E-015\000\063\000\b\016 \374\266h\344\377\277\020b\377\267\260\345\031\b\002 \374\266\376\377\377\377\034\325(\267h\344\377\277"

val should be -2

Due to the locale settings (in german setting decimal point is ',') atof() gives not the expected result.

Import works when applying Attachment 1 (DIFF).

I changed LC_NUMERIC to C and restore it afterwards. It is placed as deep in the code as possible (with respect to the goal to do it only once for a iges import) to minimize possible side effects.

I would change the status to fixed, but I see not how I could do that...

We have modified Mantis settings so that now you should be able to switch the issue to resolved; could you please try?

IGES writer has to be fixed too.

The same here (LANG=ca_ES.UTF-8). I have to start the opencascade programs with LANG=C.

This happens since years ago; in 6.3 already it worked that way.

Exporting STEP is affected too.

Any will to fix this by upstream developers? It's very annoying; it makes the programs crash in non-english or non-c locales.

I don't think the temporary locale change trick is a nice fix though. :)

Couldn't we change 'atof()' to 'atof_l()'? That should allow fixing the locale at runtime for that atof operation that has to be run in a specific locale.

Good idea! Do you know if functions with _l suffix are supported on different platforms? Gnu LibC seems to have it, as well as MS VC++ since at least 7.x (VS 2005), so we must have a try.

To follow common pattern, we can define global function Atof() in Standard package, and then use it instead of atof() in all relevant places (currently I find ~ 1400 occurrences). The same should be done for other functions (e.g. strtod).

I've no idea. But I imagine that a new Atof() is perfect, it calling atof_l when possible, and falling back to atof() in platforms where atof_l isn't available.

Same for strtod, yes.

And just one more point against the 'temporary locale change': it can be terribly bad for a multithread program. The opencascade thread will affect the rest.

The corrections are pushed to branch CR22898, please review.
I have tested it with locale "deutsh" on Windows built with VC 2009, 32-bit mode; other platforms are to be tested.

Note that I have rebased CR22898 on current master

Branch CR22898 reviewed without remarks, ready for testing.

No patch is not ready for testing - Microsoft-specific names used.

Standard_CString.cxx
 264 namespace {
 265 class CLocalePtr {
 266 public:
 267 CLocalePtr () { myLocale = _create_locale (LC_ALL, "C"); }
 268 ~CLocalePtr () { _free_locale (myLocale); }
 269 operator _locale_t () const { return myLocale; }
 270 private:
 271 _locale_t myLocale;
 272 };
 273 static CLocalePtr theCLocale;
 274 };
On POSIX systems:
_locale_t -> locale_t
_create_locale (LC_ALL, "C") -> newlocale(LC_ALL_MASK, "C", NULL)
_free_locale(myLocale) -> freelocale(myLocale)

Notice that for older systems (though it is difficult to track strtod_l history) we can also use more complicated C++ syntax:
> double aResult;
> std::istringstream aStream (theString);
> std::locale aLocale ("C");
> aStream.imbue (aLocale);
> aStream >> aResult;

There is strtod_l on Linux glibc but no vprintf_l, vsprintf_l and vfprintf_l (though MacOS X provides them for a long time). Please see CR22898_1 branch for update.

Taking into account this fact patch should be redesigned somehow.

IMHO the submitter was right, it is better to switch to C locale when reading/writing files. As can be seen in the CR22898_1 branch, replacing atof/strtod/printf is a huge and error prone work.
This can be done by thread (uselocale+newlocale on Linux and Mac, _configurethreadlocale on Windows).

If the locale can be changed per thread, fine.

I have pushed to CR22898_1 a correction which should fix the problem of missing v*printf_l functions in GLibc. The proposed solution is to switch to C locale (thread-local) and back each time. Though this might appear to be expensive, the implementation of uselocale() in glibc contains just 3 comparisons and 7 assignments (including 4 thread-local), and I believe this should be much less than call to v*printf() itself, thus not too much overhead.

Also a test case is added.

Note that I have not changed yet all calls to *printf() functions to locale-independent ones; this is to be completed.

I have some doubts if check for presence and flavor of of strtod_l and other functions available in given compiler / environment are done correctly, this could likely be improved.

Please review anyway.

To Denis: though global replacement of functions like atof() and *printf() to OCCT-specific equivalents is a lot of changes, this is quite straightforward (actually, automatic) and guarantees that the code is independent on global locale. I would appreciate knowing your opinion on why this is error-prone.

My comment was misleading; I said that this is error prone because your branch will not compile on Linux (hint: HAVE_XLOCALE_H is set, thus the #else clause l.324 is never used), and it is hard to ensure that all occurences of operator<< are safe. You also have to check third-party code (as you did in OpenGl_GraphicDriver_Export.cxx, but you should not have used setlocale() because of threading issues).

BTW your test case shows here that everything works fine except with IGES (a patch is provided in this bugreport for reading, a trivial patch is also needed for writing) and STEP (not investigated yet). So modifying those low-level functions to fix this issue looks overkill to me.

Dear Commenter 2,

patch was updated in CR22898_1 branch.
Please test compilation of the patch on all supported platforms and perform regression tests.

Dear kgv,
could you please rebase CR22898_1 branch with current master.

Rebased patch located in CR22898_2 branch

Dear Commenter 2,
Branch CR22898_2 (and products from GIT master) was compiled on Linux and Windows platforms.
Where is following compilation error on Linux platform:

http://jenkins-test-01.nnov.opencascade.com/user/mnt/my-views/view/CR22898_2/job/mnt-CR22898_2-master_build_occt_linux/2/console
../../../src/Standard/Standard_CLocaleSentry.cxx: In destructor 'virtual Standard_CLocaleSentry::~Standard_CLocaleSentry()':
../../../src/Standard/Standard_CLocaleSentry.cxx:113: error: '_locale_t' was not declared in this scope

Compilation error should be fixed now

Exporting STEP is affected too on Linux (german) WORKS on Windows (german)

Dear Commenter 2,

Branch CR22898_2 was rebased on the current master.
Conflicts files was merged by Commenter 1.

Branch CR222898_2 (and products from GIT master) was compiled on Linux and Windows platforms and tested.

Number of compiler warnings:

occt component :
Linux: 3 (3 on master)
Windows: 56 (57 on master)

products component :
Linux: 9 (9 on master)
Windows: 50 (50 on master)

Regressions:
http://occt-tests/CR22898-2-master-occt/Mandriva2010/bugs/xde/bug22898.html
bugs xde(005) bug22898 - on Linux only

Improvements:
No improvements

Testing cases:
bugs xde(005) bug22898

The problem in test on Linux is obviously due to absence of Deutsh locale on the test machine. We shall either install Deutsh support or use one of available locales (fr or ru, I guess they both use comma as decimal radix).

To Denis (sorry for late reply): thank you for highlighting the issue with Gl2Ps export, should be corrected now. As for the test case, it is clearly very incomplete as it checks only import / export -- the operation which is 100% affected. Note that export / import can pass well even if the file exported is wrong, this only checks consistency of export / import. The test case needs to be improved for sure. Actually a lot of other situations are possible with wrong locale; the fix has been actually tested by running the full set of OCCT tests with Deutsh locale.

Test case bugs/xde/bug22898 was modified in CR22898_2 branch to use French locale.

Please check only THIS test case - there no need to re-perform building from sources and whole testing company.

Dear Commenter 2,

Test case bugs/xde/bug22898 was retested, it is OK.