[vm] Change TypedData_getUint64 native method to silently cast values

[vm] Change TypedData_getUint64 native method to silently cast values

The TypedData_getUint64 native method is used to implement Uint64List
and ByteData.getUint64().

When running in --limit-ints-to-64-bits mode (with Dart integers
limited to 64 bits), this native method and its clients should
silently cast values (with wrap-around) to signed 64-bit integer.
This is the intended behavior, chosen to make it possible to work
with arbitrary data.

This CL changes underlying Integer::NewFromUint64() to do the silent
conversion instead of returning Integer::null().

Another client of Integer::NewFromUint64, Dart_NewIntegerFromUint64 is
changed to handle error without relying on Integer::NewFromUint64().

R=zra@google.com

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed: https://github.com/dart-lang/sdk/commit/41fb3c6d16f1d466d4e7968c2c677c415c397d7c

[alexmarkov, 2017-07-28 20:53:11]

alexmarkov@google.com changed reviewers:
+ asiva@google.com, zra@google.com

[zra, 2017-07-28 21:47:46]

Could you add a test for this behavior?

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2095: } else {
else not needed.

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

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18161: } else {
The else is unnecessary.

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18169: } else {
ditto

[alexmarkov, 2017-07-28 23:09:30]

Added test for ByteData.set/getUint64, Uint64List with limited integers; fixed
review comments.

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:2095: } else {
On 2017/07/28 21:47:46, zra wrote:
> else not needed.

Done.

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

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18161: } else {
On 2017/07/28 21:47:46, zra wrote:
> The else is unnecessary.

Done.

https://codereview.chromium.org/2992013002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:18169: } else {
On 2017/07/28 21:47:46, zra wrote:
> ditto

In this particular case I think 'else' adds readability: there are 2 distinct
implementations, and the active implementation is selected by a flag. There is
no preference between implementations, no fall-through or connection between
them. So I would prefer to treat them evenly, without placing one of them into
'if' and another after 'if'.

[zra, 2017-07-31 14:36:10]

https://codereview.chromium.org/2992013002/diff/20001/tests/corelib_2/typed_d...
File tests/corelib_2/typed_data_with_limited_ints_test.dart (right):

https://codereview.chromium.org/2992013002/diff/20001/tests/corelib_2/typed_d...
tests/corelib_2/typed_data_with_limited_ints_test.dart:45: testByteData();
We should probably check that optimized code has the same behavior by looping
over these calls and setting --optimization-counter-threshold as usual.

[alexmarkov, 2017-07-31 16:00:35]

Added looping into the test.

Also merged with the tip of the source tree, so there are several unrelated
changes in .status file. Please disregard them.

https://codereview.chromium.org/2992013002/diff/20001/tests/corelib_2/typed_d...
File tests/corelib_2/typed_data_with_limited_ints_test.dart (right):

https://codereview.chromium.org/2992013002/diff/20001/tests/corelib_2/typed_d...
tests/corelib_2/typed_data_with_limited_ints_test.dart:45: testByteData();
On 2017/07/31 14:36:10, zra wrote:
> We should probably check that optimized code has the same behavior by looping
> over these calls and setting --optimization-counter-threshold as usual.

Done.

[zra, 2017-07-31 20:42:58]

lgtm

[alexmarkov, 2017-07-31 21:06:38]

Description was changed from

==========
[vm] Change TypedData_getUint64 native method to silently cast values

The TypedData_getUint64 native method is used to implement Uint64List
and ByteData.getUint64().

When running in --limit-ints-to-64-bits mode (with Dart integers
limited to 64 bits), this native method and its clients should
silently cast values (with wrap-around) to signed 64-bit integer.
This is the intended behavior, chosen to make it possible to work
with arbitrary data.

This CL changes underlying Integer::NewFromUint64() to do the silent
conversion instead of returning Integer::null().

Another client of Integer::NewFromUint64, Dart_NewIntegerFromUint64 is
changed to handle error without relying on Integer::NewFromUint64().

Issue: https://github.com/dart-lang/sdk/issues/30103
==========

to

==========
[vm] Change TypedData_getUint64 native method to silently cast values

The TypedData_getUint64 native method is used to implement Uint64List
and ByteData.getUint64().

When running in --limit-ints-to-64-bits mode (with Dart integers
limited to 64 bits), this native method and its clients should
silently cast values (with wrap-around) to signed 64-bit integer.
This is the intended behavior, chosen to make it possible to work
with arbitrary data.

This CL changes underlying Integer::NewFromUint64() to do the silent
conversion instead of returning Integer::null().

Another client of Integer::NewFromUint64, Dart_NewIntegerFromUint64 is
changed to handle error without relying on Integer::NewFromUint64().

R=zra@google.com

Issue: https://github.com/dart-lang/sdk/issues/30103
Committed:
https://github.com/dart-lang/sdk/commit/41fb3c6d16f1d466d4e7968c2c677c415c397d7c
==========

[alexmarkov, 2017-07-31 21:06:39]

Committed patchset #3 (id:40001) manually as
41fb3c6d16f1d466d4e7968c2c677c415c397d7c (presubmit successful).

[siva, 2017-07-31 21:46:08]

https://codereview.chromium.org/2992013002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (left):

https://codereview.chromium.org/2992013002/diff/40001/runtime/vm/object.cc#ol...
runtime/vm/object.cc:18166: return Integer::New(value, space);
why is it not ok to just change this to 
  return Integer::New(static_cast<int64_t>(value), space);

and not have this new function IsValidUint64 ... and drop the changes in
Dart_NewIntegerFromUint64

[alexmarkov, 2017-07-31 23:58:12]

https://codereview.chromium.org/2992013002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (left):

https://codereview.chromium.org/2992013002/diff/40001/runtime/vm/object.cc#ol...
runtime/vm/object.cc:18166: return Integer::New(value, space);
On 2017/07/31 21:46:08, siva wrote:
> why is it not ok to just change this to 
>   return Integer::New(static_cast<int64_t>(value), space);
> 
> and not have this new function IsValidUint64 ... and drop the changes in
> Dart_NewIntegerFromUint64

Integer::NewFromUint64 is called from 2 places: Dart_NewIntegerFromUint64 and
TypedData_getUint64. The latter requires silent wrap-around, so range checking
is moved to Dart_NewIntegerFromUint64.

As per our discussion, I'm going to rename Integer::IsValidUint64 in order to
improve readability in the follow-up CL:
https://codereview.chromium.org/2992783002/