Adds a flag to the standalone vm to throw an exception on 53-bit integer overflow.

Adds a flag to the standalone vm to throw an exception on 53-bit integer overflow.

R=asiva@google.com, srdjan@google.com

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

[zra, 2013-05-22 22:25:12]

I think I've got the check in all the places that Srdjan mentioned. Please let
me know if I've missed anywhere.

Cheers,
Zach

[siva, 2013-05-23 01:28:22]

There are some intrinsified function in intrinsifier.h which deal with
IntegerImplementation, how are those handled for 53 bit overflows.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc#newc...
runtime/lib/double.cc:19: DECLARE_FLAG(bool, throw_on_53bit_overflow);
I wonder if the flag needs to be:
throw_on_javascript_int_overflow
or something like that.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc#newc...
runtime/lib/double.cc:93: return i.AsValidInteger();
Integer::New(...) above has already done the Smi/Mint demarcation why do we need
to call AsValidInteger again over here and not just do a
return i.raw();

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc#ne...
runtime/lib/integers.cc:42: static void ThrowExceptionOnOverflow(const Integer&
i, const char* msg) {
The name ThrowExceptionOnOverflow seems like a generic overflow exception, maybe
call it CheckForFiftyThreeOverflow?

Also could this be a static method in the Integer class. That way you could use
it in double.cc, typed_data.cc etc.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc#ne...
runtime/lib/integers.cc:372: return i.AsValidInteger();
Ditto question about the need for AsValidInteger.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:400: return i.AsValidInteger();
Ditto comment about the AsValidInteger.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:421: if (value >
static_cast<uint64_t>(Mint::kMaxValue)) {
What about throwing an exception here?

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:430: return i.AsValidInteger();
Ditto comment about AsValidInteger

[zra, 2013-05-23 15:59:06]

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc#newc...
runtime/lib/double.cc:19: DECLARE_FLAG(bool, throw_on_53bit_overflow);
On 2013/05/23 01:28:22, siva wrote:
> I wonder if the flag needs to be:
> throw_on_javascript_int_overflow
> or something like that.

Done.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/double.cc#newc...
runtime/lib/double.cc:93: return i.AsValidInteger();
On 2013/05/23 01:28:22, siva wrote:
> Integer::New(...) above has already done the Smi/Mint demarcation why do we
need
> to call AsValidInteger again over here and not just do a
> return i.raw();

Done.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc#ne...
runtime/lib/integers.cc:42: static void ThrowExceptionOnOverflow(const Integer&
i, const char* msg) {
On 2013/05/23 01:28:22, siva wrote:
> The name ThrowExceptionOnOverflow seems like a generic overflow exception,
maybe
> call it CheckForFiftyThreeOverflow?
> 
> Also could this be a static method in the Integer class. That way you could
use
> it in double.cc, typed_data.cc etc.

Done.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/integers.cc#ne...
runtime/lib/integers.cc:372: return i.AsValidInteger();
On 2013/05/23 01:28:22, siva wrote:
> Ditto question about the need for AsValidInteger.

Done.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:400: return i.AsValidInteger();
On 2013/05/23 01:28:22, siva wrote:
> Ditto comment about the AsValidInteger.

Done.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:421: if (value >
static_cast<uint64_t>(Mint::kMaxValue)) {
On 2013/05/23 01:28:22, siva wrote:
> What about throwing an exception here?

Yes, I've added a throw here, too.

https://codereview.chromium.org/15743017/diff/7001/runtime/lib/typed_data.cc#...
runtime/lib/typed_data.cc:430: return i.AsValidInteger();
On 2013/05/23 01:28:22, siva wrote:
> Ditto comment about AsValidInteger

Done.

[siva, 2013-05-23 17:38:57]

LGTM.

Maybe in
bool Intrinsifier::CanIntrinsify(const Function& function) {
  if (!FLAG_intrinsify) return false;
  if (function.IsClosureFunction()) return false;
  // Can occur because of compile-all flag.
  if (function.is_external()) return false;
  return function.is_intrinsic();
}

we could add

if (FLAG_throw_on_javascript_int_overflow) return;

of course this would slow down other things as well.

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/double.cc#new...
runtime/lib/double.cc:87: if (FLAG_throw_on_javascript_int_overflow &&
!i.FitsIn53Bits()) {
just
if (FLAG_throw_on_javascript_int_overflow) {
 ....
}

The FitsIn53Bits check is already inside CheckForFifty...

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/typed_data.cc
File runtime/lib/typed_data.cc (right):

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/typed_data.cc...
runtime/lib/typed_data.cc:266: if (FLAG_throw_on_javascript_int_overflow &&
!result.FitsIn53Bits()) {       \
Ditto comment about the FitsIn53Bits check here.

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/typed_data.cc...
runtime/lib/typed_data.cc:393: if (FLAG_throw_on_javascript_int_overflow &&
!i.FitsIn53Bits()) {
Ditto comment about the FitsIn53Bits check here.

https://codereview.chromium.org/15743017/diff/11013/runtime/lib/typed_data.cc...
runtime/lib/typed_data.cc:427: if (FLAG_throw_on_javascript_int_overflow &&
!i.FitsIn53Bits()) {
Ditto comment about FitsIn53Bits check here.

[zra, 2013-05-24 23:12:18]

I've updated based on my previous CL. I've also added the check Siva suggested
to CanIntrinsify. Please have another look =)

Cheers,
Zach

[srdjan, 2013-05-28 10:57:14]

DBC

What is the plan, shouldn't we have be turning the flag on as part of the
checked mode? Can we run all tests with that flag on? What is the performance
impact?

Cheers.

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

https://codereview.chromium.org/15743017/diff/34002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10447: return;
remove return.

https://codereview.chromium.org/15743017/diff/34002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10492: static void CheckFiftyThreeBitOverflow(const Integer
&i) {
Optional: The code would be simpler if you have ThrowFifty... only once and use
if (Util::IsInt (...)) return;

https://codereview.chromium.org/15743017/diff/34002/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/15743017/diff/34002/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:239: String toString() => "53-bit Overflow";
I think we should provide more information here (i.e., the value).

[zra, 2013-05-28 17:06:39]

Lots of tests fail with this flag on, so I'm not sure turning it on as part of
checked mode is a good idea. I don't know what the performance impact is.

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

https://codereview.chromium.org/15743017/diff/34002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10447: return;
On 2013/05/28 10:57:14, srdjan wrote:
> remove return.

Done.

https://codereview.chromium.org/15743017/diff/34002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10492: static void CheckFiftyThreeBitOverflow(const Integer
&i) {
On 2013/05/28 10:57:14, srdjan wrote:
> Optional: The code would be simpler if you have ThrowFifty... only once and
use
> if (Util::IsInt (...)) return;

I gave this a shot. Please let me know if it looks like it could be simplified
further.

https://codereview.chromium.org/15743017/diff/34002/sdk/lib/core/errors.dart
File sdk/lib/core/errors.dart (right):

https://codereview.chromium.org/15743017/diff/34002/sdk/lib/core/errors.dart#...
sdk/lib/core/errors.dart:239: String toString() => "53-bit Overflow";
On 2013/05/28 10:57:14, srdjan wrote:
> I think we should provide more information here (i.e., the value).

Done.

[siva, 2013-05-28 17:47:33]

On 2013/05/24 23:12:18, zra wrote:
> I've updated based on my previous CL. I've also added the check Siva suggested
> to CanIntrinsify. Please have another look =)
> 
> Cheers,
> Zach

Turning off intrinsification is going to have a pretty noticeable performance
impact. I am wondering if we should do this only for 64 bit builds and maybe
also separate the list of IntegerImplementation intrinsics from the corelib
list.

[zra, 2013-05-28 21:15:24]

On 2013/05/28 17:47:33, siva wrote:
> Turning off intrinsification is going to have a pretty noticeable performance
> impact. I am wondering if we should do this only for 64 bit builds and maybe
> also separate the list of IntegerImplementation intrinsics from the corelib
> list.

If performance is important when this flag is used, then this is probably a good
idea.

[srdjan, 2013-05-29 12:34:24]

On 2013/05/28 21:15:24, zra wrote:
> On 2013/05/28 17:47:33, siva wrote:
> > Turning off intrinsification is going to have a pretty noticeable
performance
> > impact. I am wondering if we should do this only for 64 bit builds and maybe
> > also separate the list of IntegerImplementation intrinsics from the corelib
> > list.
> 
> If performance is important when this flag is used, then this is probably a
good
> idea.

Yes, performance is important as it will be turned on in checked mode.

[zra, 2013-05-31 18:29:28]

On 2013/05/28 17:47:33, siva wrote:
> Turning off intrinsification is going to have a pretty noticeable performance
> impact. I am wondering if we should do this only for 64 bit builds and maybe
> also separate the list of IntegerImplementation intrinsics from the corelib
> list.

I've created a new list of the Integer intrinsics from corelib. Only these
functions won't be intrinsified if --throw_on_javascript_int_overflow is passed.

Remaining questions: Should this flag be set automatically if --checked is
passed? How do I enable only for 64-bit builds?

Cheers,
Zach

[zra, 2013-06-05 15:10:42]

On 2013/05/31 18:29:28, zra wrote:
> I've created a new list of the Integer intrinsics from corelib. Only these
> functions won't be intrinsified if --throw_on_javascript_int_overflow is
passed.
> 
> Remaining questions: Should this flag be set automatically if --checked is
> passed? How do I enable only for 64-bit builds?
> 
> Cheers,
> Zach

I'd like to go ahead and commit this, and make the changes for 64-bit in another
CL. How does that sound?

Thanks,
Zach

[srdjan, 2013-06-05 15:32:00]

LGTM

https://codereview.chromium.org/15743017/diff/59001/tests/standalone/53bit_ov...
File tests/standalone/53bit_overflow_test.dart (right):

https://codereview.chromium.org/15743017/diff/59001/tests/standalone/53bit_ov...
tests/standalone/53bit_overflow_test.dart:45: Expect.equals(190000000000000
,double_to_int());
Fix comma formatting.

[zra, 2013-06-05 17:18:15]

Committed patchset #10 manually as r23640 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-06-20 12:30:53]

Please CC or add either myself or floitsch as reviewers on library changes (like
the change to lib/core/errors.dart here).