Closed Bug 551118 Opened 10 years ago Closed 10 years ago

js_StringToNumber and js_StringToInt32 mess up negative hex conversion

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

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.
Didn't we fix this in 2008?

/be
fwiw, I have a fix in the works here.
blocking2.0: ? → final
Attached patch Proposed fix (obsolete) — Splinter Review
Attachment #431410 - Flags: review?(gwagner)
Attachment #431410 - Flags: review?(brendan)
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
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
Attachment #431410 - Flags: review?(brendan) → review+
(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
> 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?
(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.
Attachment #431410 - Attachment is obsolete: true
Attachment #431410 - Flags: review?(gwagner)
Pushed http://hg.mozilla.org/tracemonkey/rev/fc1b3ed54cc2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Indeed.  Looks like the problem is that single-char whitespace strings become NaN when treated as doubles, instead of becoming 0.  Patch coming up.
Pushed http://hg.mozilla.org/tracemonkey/rev/2e1171268397 to fix that, with r=jorendorff on irc.
You need to log in before you can comment on or make changes to this bug.