Fix "illegal access" when calling parseInt with a radix that is not a smi.

Fix "illegal access" when calling parseInt with a radix that is not a smi.


BUG=v8:1246
TEST=regress-1246.js

Committed: http://code.google.com/p/v8/source/detail?r=8444

[keuchel, 2011-06-20 08:33:28]

PTAL

[Steven, 2011-06-27 11:39:04]

This is already open for some time. Can someone PTAL?

[Lasse Reichstein, 2011-06-28 08:32:21]

LGTM with comments.

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode109
src/v8natives.js:109: radix = radix || 0;
This seems silly to begin with. Just set 
 radix = 0;
It means the same as radix 10, so the rest of the function will work the same,
and it seems we optimize the code for 0 over 10.

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode113
src/v8natives.js:113: return $NaN;
Could you please put braces around the then-branch of this if while you are
here.

http://codereview.chromium.org/7206019/diff/1/test/mjsunit/regress/regress-12...
File test/mjsunit/regress/regress-1246.js (right):

http://codereview.chromium.org/7206019/diff/1/test/mjsunit/regress/regress-12...
test/mjsunit/regress/regress-1246.js:49: assertEquals(NaN, parseInt("z", "a"));
Good tests.
How about also trying a string with a number (e.g. "10") as radix. 
And NaN and Infinity as radix (they should convert to 0 using ToInt32, so it
shoud work as base-10, but especially NaN can be tricky in comparisons).

[Steven, 2011-06-28 10:10:35]

PTAL again.

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode109
src/v8natives.js:109: radix = radix || 0;
It seems so but it's not: 'radix = 0' will trigger radix detection what you
don't want in the case 'radix == 10'.
On 2011/06/28 08:32:21, Lasse Reichstein wrote:
> This seems silly to begin with. Just set 
>  radix = 0;
> It means the same as radix 10, so the rest of the function will work the same,
> and it seems we optimize the code for 0 over 10.
>

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode113
src/v8natives.js:113: return $NaN;
On 2011/06/28 08:32:21, Lasse Reichstein wrote:
> Could you please put braces around the then-branch of this if while you are
> here.

Done.

http://codereview.chromium.org/7206019/diff/1/test/mjsunit/regress/regress-12...
File test/mjsunit/regress/regress-1246.js (right):

http://codereview.chromium.org/7206019/diff/1/test/mjsunit/regress/regress-12...
test/mjsunit/regress/regress-1246.js:49: assertEquals(NaN, parseInt("z", "a"));
I've included those cases and rewrote the tests by classifying the radix
argument values by which "radix mode" they will result in.
On 2011/06/28 08:32:21, Lasse Reichstein wrote:
> Good tests.
> How about also trying a string with a number (e.g. "10") as radix. 
> And NaN and Infinity as radix (they should convert to 0 using ToInt32, so it
> shoud work as base-10, but especially NaN can be tricky in comparisons).

[Lasse Reichstein, 2011-06-28 10:28:17]

LGTM

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode109
src/v8natives.js:109: radix = radix || 0;
Ack, my bad.
In that case, use
 radix = TO_INT32(radix);
Or, if it's faster (it probably is), just:
 radix = radix | 0;
Both will convert to Int32, which converts both undefined and -0 to 0.
Using || 0 is likely to be slower, since it contains an implicity branch.

[Lasse Reichstein, 2011-06-28 10:31:48]

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js
File src/v8natives.js (right):

http://codereview.chromium.org/7206019/diff/1/src/v8natives.js#newcode109
src/v8natives.js:109: radix = radix || 0;
and because  radix = radix || 0  would not convert a HeapNumber containing 10 to
a smi.

If possible, add a test where the number is a heap-number value equal to 10 (but
it's hard to guarantee the underlying representation, try something like
Math.ln(Math.exp(10)) and cross your fingers).