551118 - js_StringToNumber and js_StringToInt32 mess up negative hex conversion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Shell testcase:

  for (var j = 0; j < 3; ++j) {
      print(-16 == "-0x10");
  }

This should print "false" three times.  Instead it prints "false" twice then "true".

The problem is that js_StringToNumber and js_StringToInt32 don't do the "check for negative hex" check that js_ValueToNumber does.

It would be really nice to combine these three codepaths if we can... certainly js_StringToNumber and js_ValueToNumber can use the same code.

Comment 1

[Brendan Eich [:brendan]]

Didn't we fix this in 2008?

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

fwiw, I have a fix in the works here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 431410 [details] [diff] [review]
Proposed fix

>+template<typename T> struct js_number_traits { };

Put namespace js {...} around this and use NumberTraits (studlycaps typenames) for the template names.

>+template<typename T>
>+static JS_ALWAYS_INLINE T js_strtonumber(JSContext *cx, JSString *str)

Still want line starting with function name.

Function name should be js_StringToNumberType or something like that. The JS_strtod/js_strtointeger naming horrors are just that -- avoid imitating.

>+    /*
>+     * Note that ECMA doesn't treat a string beginning with a '0' as
>+     * an octal number here. This works because all such numbers will
>+     * be interpreted as decimal by js_strtod.  Also, any hex numbers
>+     * that have made it here (which can only be negative ones) will
>+     * be treated as 0 without consuming the 'x' by js_strtod.
>+     */
>+    if (!js_strtod(cx, bp, end, &ep, &d) ||

Please do file that bug on js_strtod failure being ignored here (at least it's one place now -- thanks!).

I was thinking we could make js_strtod infallible and use jsdouble as its return type for better perf, if we avoid malloc'ing a buffer for really long strings, instead using some generous stack buffer (128 chars? big enough by far for any sane numeric strings), and if the length is too big to fit, we pre-parse the string to see whether it is well-formed. The RegExp for doubles is easy enough to implement by hand in C++:

/^\d+\.\d*(?:[eE][-+]?\d+)?|^\d+(?:\.\d*)?[eE][-+]?\d+|^\.\d+(?:[eE][-+]?\d+)?/

What the pre-parse pass would do would be throw away insignificant trailing digits (ones that cannot possibly cause rounding to go differently from when those digits are dropped), but of course do no conversion.

Anyway, this is for that other bug.

r=me with namespaced typenames and style nits picked.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Please do file that bug on js_strtod failure being ignored here (at least it's
> one place now -- thanks!).

Sorry, behind on bugmail -- you already filed bug 551229. Thanks again.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Function name should be js_StringToNumberType or something like that.

If it's in namespace js, do you still want the "js_" prefix?  Or just call it StringToNumberType?

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> If it's in namespace js, do you still want the "js_" prefix?  Or just call it
> StringToNumberType?

Righto on the latter.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/fc1b3ed54cc2

Comment 10

[Brendan Eich [:brendan]]

Followup nit picking:

http://hg.mozilla.org/tracemonkey/rev/24b2678b458d

/be

Comment 11

[Bob Clary [:bc] (inactive)]

From tracemonkey's pushlog it looks like this caused:

ecma/Expressions/11.4.6.js | +(' ') wrong value item 2
ecma/Expressions/11.4.6.js | +(\t) wrong value item 3
ecma/Expressions/11.4.6.js | +(\n) wrong value item 4
ecma/Expressions/11.4.6.js | +(\r) wrong value item 5
ecma/Expressions/11.4.6.js | +(\f) wrong value item 6
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x0009) wrong value item 7
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x0020) wrong value item 8
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x000C) wrong value item 9
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x000B) wrong value item 10
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x000D) wrong value item 11
ecma/Expressions/11.4.6.js | +(String.fromCharCode(0x000A) wrong value item 12
ecma/Expressions/11.4.7-01.js | -(' ') wrong value item 2
ecma/Expressions/11.4.7-01.js | -(\t) wrong value item 3
ecma/Expressions/11.4.7-01.js | -(\n) wrong value item 4
ecma/Expressions/11.4.7-01.js | -(\r) wrong value item 5
ecma/Expressions/11.4.7-01.js | -(\f) wrong value item 6
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x0009) wrong value item 7
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x0020) wrong value item 8
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x000C) wrong value item 9
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x000B) wrong value item 10
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x000D) wrong value item 11
ecma/Expressions/11.4.7-01.js | -(String.fromCharCode(0x000A) wrong value item 12
ecma/Expressions/11.9.1.js | 0 == '
ecma/Expressions/11.9.2.js | 0 != '
ecma/Expressions/11.9.3.js | 0 == '
ecma/GlobalObject/15.1.2.6.js | isNaN( ' ' ) wrong value item 11
ecma/GlobalObject/15.1.2.7.js | isFinite( ' ' ) wrong value item 11
ecma/TypeConversion/9.3.1-1.js | Number(' ') wrong value item 2
ecma/TypeConversion/9.3.1-1.js | Number(\t) wrong value item 3
ecma/TypeConversion/9.3.1-1.js | Number(\n) wrong value item 4
ecma/TypeConversion/9.3.1-1.js | Number(\r) wrong value item 5
ecma/TypeConversion/9.3.1-1.js | Number(\f) wrong value item 6
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x0009) wrong value item 7
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x0020) wrong value item 8
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x000C) wrong value item 9
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x000B) wrong value item 10
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x000D) wrong value item 11
ecma/TypeConversion/9.3.1-1.js | Number(String.fromCharCode(0x000A) wrong value item 12
ecma/TypeConversion/9.3.1-3.js | -" " wrong value item 99

Comment 12

[Boris Zbarsky [:bzbarsky]]

Indeed.  Looks like the problem is that single-char whitespace strings become NaN when treated as doubles, instead of becoming 0.  Patch coming up.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/2e1171268397 to fix that, with r=jorendorff on irc.