102725 - gcc -O2 problems converting numbers to strings

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Phil Schwartau]

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());

Comment 4

[Brendan Eich [:brendan]]

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

/be

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 6

[Bradley Baetz (:bbaetz)]

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)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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();

Comment 8

[Phil Schwartau]

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'

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[Brian Ryner (not reading)]

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

Comment 11

[Ulrich Drepper]

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.

Comment 12

[Brian Ryner (not reading)]

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?

Comment 13

[Ulrich Drepper]

> 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.

Comment 14

[Brian Ryner (not reading)]

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?

Comment 15

[Ulrich Drepper]

> 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)

Comment 16

[Brian Ryner (not reading)]

Attachment: patch

Comment 17

[Ulrich Drepper]

> 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.

Comment 18

[Brendan Eich [:brendan]]

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

Comment 19

[Ulrich Drepper]

> 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 20

[Brian Ryner (not reading)]

Attachment: patch v2

Comment 21

[Brendan Eich [:brendan]]

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=[e-mail].

/be

Comment 22

[Bradley Baetz (:bbaetz)]

Comment on attachment 53499 [details] [diff] [review]
patch v2

r=bbaetz

Comment 23

[Brian Ryner (not reading)]

checked in.

Comment 24

[Phil Schwartau]

[e-mail]: if you get a chance, would you be able to 
verify this fix? I was never able to see the failure - thanks.

Comment 25

[Brendan Eich [:brendan]]

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

/be