Fixes javascript integer overflow check.

Fixes javascript integer overflow check.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25675

[zra, 2013-07-30 23:04:52]

What tests were failing on this?

[siva, 2013-07-31 02:55:51]

On 2013/07/30 23:04:52, zra wrote:
> What tests were failing on this?

The failing tests are:
[ $compiler == none && $runtime == drt ]
53bit_overflow_literal_test: fail # Issue 12044
53bit_overflow_test: fail # Issue 12044

These are the only tests which turn that flag on. I think if you run some of the
timer
tests with the flag turned on you may hit similar failures too.

[zra, 2013-07-31 15:17:02]

On 2013/07/31 02:55:51, siva wrote:
> On 2013/07/30 23:04:52, zra wrote:
> > What tests were failing on this?
> 
> The failing tests are:
> [ $compiler == none && $runtime == drt ]
> 53bit_overflow_literal_test: fail # Issue 12044
> 53bit_overflow_test: fail # Issue 12044
> 
> These are the only tests which turn that flag on. I think if you run some of
the
> timer
> tests with the flag turned on you may hit similar failures too.

Thanks. The timer tests without this fix and with the flag triggered the error
on the large constant in the timer library. With this fix the error is gone.
I've also made one small change. If we have a sign bit and 53 bits, then the
range is symmetric and we can't represent the smallest (most negative) 54-bit
twos compliment integer. This test is in Is54BitNoMinInt() in object.cc.

[siva, 2013-07-31 16:08:28]

lgtm

https://codereview.chromium.org/21301003/diff/17001/runtime/lib/errors_patch....
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/21301003/diff/17001/runtime/lib/errors_patch....
runtime/lib/errors_patch.dart:185: String toString() => "54-bit Overflow:
$_value";
Should the string say "Javascript Integer Overflow: $_value"

https://codereview.chromium.org/21301003/diff/17001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/21301003/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:11284: // set, and the fraction is 0, the result is -0.0,
not MIN_53BIT_INT.
The comment here is a bit confusing as we are dealing with integers and
you refer to floating point. I presume you are referring to the
implementation by dart2js, maybe you should make that explicit.

https://codereview.chromium.org/21301003/diff/17001/tests/standalone/standalo...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/21301003/diff/17001/tests/standalone/standalo...
tests/standalone/standalone.status:55: 
Does it make sense to rename these tests javascript_int_overflow_test instead of
the current name which has become misleading now.

[zra, 2013-07-31 17:46:30]

Thanks!

https://codereview.chromium.org/21301003/diff/17001/runtime/lib/errors_patch....
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/21301003/diff/17001/runtime/lib/errors_patch....
runtime/lib/errors_patch.dart:185: String toString() => "54-bit Overflow:
$_value";
On 2013/07/31 16:08:28, siva wrote:
> Should the string say "Javascript Integer Overflow: $_value"

Done.

https://codereview.chromium.org/21301003/diff/17001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/21301003/diff/17001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:11284: // set, and the fraction is 0, the result is -0.0,
not MIN_53BIT_INT.
On 2013/07/31 16:08:28, siva wrote:
> The comment here is a bit confusing as we are dealing with integers and
> you refer to floating point. I presume you are referring to the
> implementation by dart2js, maybe you should make that explicit.

Done.

https://codereview.chromium.org/21301003/diff/17001/tests/standalone/standalo...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/21301003/diff/17001/tests/standalone/standalo...
tests/standalone/standalone.status:55: 
On 2013/07/31 16:08:28, siva wrote:
> Does it make sense to rename these tests javascript_int_overflow_test instead
of
> the current name which has become misleading now.

Done.

[zra, 2013-07-31 17:51:34]

Committed patchset #6 manually as r25675 (presubmit successful).

[srdjan, 2013-07-31 22:16:44]

lgtm

[Florian Schneider, 2013-08-01 15:45:02]

DBC:

https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:11287: // -2^54 - 1 ... 2^54 - 1, i.e. the same as a 54-bit
signed integer
Something is slightly off here: a int54_t has a range -2^53..2^53-1.

JavaScript numbers are precise  in the range -2^53..2^53.

Try "Math.pow(2, 53)+1 == Math.pow(2, 53)". It yields true.

[zra, 2013-08-01 15:58:19]

On 2013/08/01 15:45:02, Florian Schneider wrote:
> DBC:
> 
> https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc#newc...
> runtime/vm/object.cc:11287: // -2^54 - 1 ... 2^54 - 1, i.e. the same as a
54-bit
> signed integer
> Something is slightly off here: a int54_t has a range -2^53..2^53-1.
> 
> JavaScript numbers are precise  in the range -2^53..2^53.
> 
> Try "Math.pow(2, 53)+1 == Math.pow(2, 53)". It yields true.

The comment is wrong from a find-replace error, but the code is correct if you
have 53 bits plus a sign bit. Then the javascript range is -2^53-1 ... 2^53-1.
Above 2^53-1 until 2^54(?) you have precise representation of the even integers.
I don't know what Math.pow is supposed to do, but if you add one to float 2^53,
it isn't too surprising to get back float 2^53.

[Florian Schneider, 2013-08-02 10:24:16]

Not exactly: To be precise, 2^53 is represented precisely by the FP bit
representation 0x4340000000000000.
2^53 - 1, 2^53 - 2, etc. are also precisely represented.

Correspondingly -2^53 is precisely represented as 0xc340000000000000. So is
-2^53 + 1, -2^53 + 2, etc.

Your tests and code do not include those two numbers. Please update them
accordingly so that the valid interval
is (-2^53, 2^53) with the boundaries included.

https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/21301003/diff/32002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:11291: return (Utils::IsInt(54, value)) && (value !=
(-0x1FFFFFFFFFFFFF - 1));
I think this should be

return Utils::IsInt(54, value) || (value == 0x20000000000000);

https://codereview.chromium.org/21301003/diff/32002/tests/standalone/javascri...
File tests/standalone/javascript_int_overflow_literal_test.dart (right):

https://codereview.chromium.org/21301003/diff/32002/tests/standalone/javascri...
tests/standalone/javascript_int_overflow_literal_test.dart:12: var
too_big_literal = 0x20000000000000;  /// 01: compile-time error
0x20000000000000 is fine.

0x20000000000001 too large.

Same with negative integers:

-0x20000000000000 is fine, -0x20000000000001 overflows.