I've been seeing this problem with a Linux gcc 3.0.1 build with -O2, and waterson saw it on a Linux gcc 2.95.2 build with -O2. It prevents jrgm's performance tests from working on such builds. In some cases calling toString() on a number causes the first two digits to appears as a ':' instead. However, when toString is called a second time on the same number, the result is correct. (When I stick in a JS_GetStringBytes call to look at the number at the point when it's wrong the first time, it stays wrong the second time as well.) Expected results from testcase alerts (example; date dependent): 1002049323261 1002049323261 1002049323261 124 Actual results: :02049323261 1002049323261 1002049323261 124

Debugging the problem, it seemed like there were two garbage characters at the beginning of the buffer at the end of js_NumberToString before the real string began, but otherwise that string was correct. Yet the result in num_toString seemed correct when looking through the result of JS_GetStringChars, unless JS_GetStringBytes were called first. Or something like that... And it is worth noting that num_toString was only called once for the number for the three toString calls.

Reassigning to Kenton. Here is the text of David's testcase - var foo = (new Date()).getTime(); ++foo; var bar = foo.toString(); var baz = foo.toString(); var bax = foo.toString(); alert(bar); alert(baz); alert(bax); var ba = 124; alert(ba.toString());

This smells of memory ambiguity -- more of our (jsdouble*) can alias (uint32*) assumptions, which standard C and C99 do not help us implement (:-(). /be

Does -fno-strict-aliasing affect the problem? That would help confirm or deny brendan's suspicion.

This is probably a dupe of/related to bug 94375. See that bug for various compiler flags tested (although that was only with -O3, and didn't show on -O2. I've seen that problem on my gcc-2.96 -O2 builds, though)

Using -fno-strict-aliasing does fix the problem with gcc-2.96-81 (RH). My test was, with the standalone js build, the following: var foo = (new Date()).getTime(); foo.toString(); foo.toString();

David's testcase added to JS testsuite: mozilla/js/tests/js1_5/Regress/regress-102725.js Can be run in the interactive JS shell as follows: [//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js js> load('../../tests/js1_5/shell.js') js> load('../../tests/js1_5/Regress/regress-102725.js') BUGNUMBER: 102725 STATUS: Testing converting numbers to strings This denotes success. A failure would look something like: BUGNUMBER: 102725 STATUS: Testing converting numbers to strings FAILED!: [reported from test()] Section 1 of test - FAILED!: [reported from test()] Expected value '1002677799937XYZ', Actual value '1002677799937'

Compiling only jsdtoa.c with -fno-strict-aliasing is sufficient to fix the problem.

drepper, any ideas on this? could this be similar to the aliasing problem we uncovered in jsnum.h?

Has anybody looked which path the js_NumberToString takes? I'd expect js_NumberToString > JS_snprintf > cvt_l > fill_n In neither of these functions I've seen an unclean cast which could cause aliasing problems. If somebody could tell me which functions are actually called in case of the failing print I'll look at it more deeply. Sorry, don't have the time in the moment to do this myself.

How about this code at line 244 of jsdtoa.c ? #if defined (IEEE_8087) && !defined(__arm) && !defined(__arm32__) && !defined(__arm26__) #define word0(x) ((ULong *)&x)[1] #define word1(x) ((ULong *)&x)[0] #else #define word0(x) ((ULong *)&x)[0] #define word1(x) ((ULong *)&x)[1] #endif This looks like exactly what we had to fix in jsnum.h... why isn't this code using those macros?

> How about this code at line 244 of jsdtoa.c ? I wasn't looking at that code. I assumes the integer arithmetic is using 64-bit and therefore 1002049323261 is an integer. OK, jsdtoa.c indeed has two problems: line 244ff word0(), word1() line 256ff Storeinc() The cleanest solution is for word0()/word1() to be changed to two sets of macros: get_word1()/get_word1() and set_word0()/set_word1(). Storeinc() is used only for storing so this is OK. Once you've done this you can easily adapt the macros from jsnum.h for this purpose. Note, though, how the endianess in the access is handled differently. In jsnum.h the IS_LITTLE_ENDIAN macro is used, jsdtoa.c uses explicit tests for Arm.

Ok, I think I have a patch that fixes the word0/word1 problems, but I'm not sure what to do about Storeinc(). Do we need similar helpers to access the high and low 16 bits of a 32 bit integer?

> but I'm not sure what to do about Storeinc(). Do we need similar helpers to > access the high and low 16 bits of a 32 bit integer? I have no idea why the Storeinc macro is that complicated anyway. Somebody tried to optimize by hand what the compiler should better do itself. I would simply remove the current definitions and use instead the one given in the comment above: #define Storeinc(a,b,c) (*a++ = b << 16 | c & 0xffff)

> Created an attachment (id=53433) *Looks* (haven't tested it) fine to me. Two things: - I think it is generally bad to not use macro parameters of the macro is in fact used like a function. Even if this means simply passing the parameters through. - I know I gave the form of Storeinc in the comment as the preferred form but it should be changed slightly just to be save: #define Storeinc(a,b,c) (*(a)++ = (b) << 16 | (c) & 0xffff) Note the parenthesis around the parameters. It might not be necessary right now but these are the ugliest bugs to track down so it's better to be cautious.

bryner: jsdtoa.c is derived from the David M. Gay / Guy Steele portable double-to-string code of olde. It wasn't integrated into the JS sources as well as I wanted, and I never had time to fix it myself. Thanks for the patch. A couple of nits: - Don't we have an inconsistency still between jsdtoa.c and jsnum.h on arm? Fix that and we won't need the proliferation of BIGENDIAN_ and LITTLEENDIAN_ macros. - drepper's right, always parenthesize macro parameters in the definition. - Please respect the 80th column (leave it empty for Emacs) in JS sources. /be

> Don't we have an inconsistency still between jsdtoa.c and jsnum.h on arm? > Fix that and we won't need the proliferation of BIGENDIAN_ and LITTLEENDIAN_ > macros. The Arm situation is really ugly. This is the <bits/endian.h> file I have in glibc: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /* ARM is (usually) little-endian but with a big-endian FPU. */ #ifndef _ENDIAN_H # error "Never use <bits/endian.h> directly; include <endian.h> instead." #endif #ifdef __ARMEB__ #define __BYTE_ORDER __BIG_ENDIAN #else #define __BYTE_ORDER __LITTLE_ENDIAN #endif #define __FLOAT_WORD_ORDER __BIG_ENDIAN ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This means that Arm is normally little endian but the floating-point format is always big endian. You will never be able to handle endianness in integer and floating-point numbers with one macros. Having this said it is clear that the current jsnum.h macros (even before the changes made for aliasing some time back) could never have worked for Arm. If you want to solve the problem for good introduce in some central place macros just like those above.

Comment on attachment 53499 [details] [diff] [review] patch v2 Looks good to me, thanks again. Uber-nit: JS major comments look like this: /* * Blah blah blah * (blah). */ Note the "empty" first line is /*\n. Fix that, in your earlier comment and in Stefan's, and sr=brendan@mozilla.org. /be

Comment on attachment 53499 [details] [diff] [review] patch v2 r=bbaetz

checked in.

dbaron@fas.harvard.edu: if you get a chance, would you be able to verify this fix? I was never able to see the failure - thanks.

Verified with my -O2 js shell and a .js file reduced from the testcase. /be