[corelib] Add explicit overflow checks to int.parse

[corelib] Add explicit overflow checks to int.parse

In the --limit-ints-to-64-bits mode, when Dart integers are limited
to 64 bits, int.parse() should detect and reject large integers.
This CL adds explicit overflow checks into int.parse() implementation.

R=lrn@google.com, zra@google.com

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

[alexmarkov, 2017-07-27 21:01:07]

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

[zra, 2017-07-27 21:22:38]

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:239: static const _int64OverflowLimits = const [
Is there any way to populate this table lazily?

https://codereview.chromium.org/2987003002/diff/1/tests/corelib_2/int_parse_w...
File tests/corelib_2/int_parse_with_limited_ints_test.dart (right):

https://codereview.chromium.org/2987003002/diff/1/tests/corelib_2/int_parse_w...
tests/corelib_2/int_parse_with_limited_ints_test.dart:1: // Copyright (c) 2015,
the Dart project authors.  Please see the AUTHORS file
2017

[alexmarkov, 2017-07-27 23:37:33]

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:239: static const _int64OverflowLimits = const [
On 2017/07/27 21:22:38, zra wrote:
> Is there any way to populate this table lazily?

Done.

https://codereview.chromium.org/2987003002/diff/1/tests/corelib_2/int_parse_w...
File tests/corelib_2/int_parse_with_limited_ints_test.dart (right):

https://codereview.chromium.org/2987003002/diff/1/tests/corelib_2/int_parse_w...
tests/corelib_2/int_parse_with_limited_ints_test.dart:1: // Copyright (c) 2015,
the Dart project authors.  Please see the AUTHORS file
On 2017/07/27 21:22:38, zra wrote:
> 2017

Done.

[Lasse Reichstein Nielsen, 2017-07-27 23:40:18]

lgtm

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:136: int negativeOverflowLimit =
_int64OverflowLimits[tableIndex + 1];
Only read these if they are needed (which goes well with the separate loop
suggested below).

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:153: }
This increases the inner loop size significantly if the condition is not
optimized away by the compiler.

Can you check how much this affects performance when the flag isn't set?

Maybe have an entire different loop for the limit-to-64-bits case.
Heck, maybe have an entire different method and algorithm since we know that
there are at most two/three smis in 64 bits.
(I'm also considering splitting the parsing into different loops for base <= 10
and base > 10, instead of having the radix check in every call to _parseBlock.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:226: // (--limit-ints-to-64-bits mode is
enabled).
Use doc-comments ("///"). It might make some editors show the documentation when
hovering, and it can't hurt.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:230: static const _minInt64 = -_maxInt64 - 1;
In the "limit integers to 64-bit mode", you should find a way to accept the
expression `-0x8000000000000000`, even if it requires special-casing the unary
minus+integer literal case.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:239: static const _int64OverflowLimits = const [
Don't make it const, then it's evaluated the first time it's accessed.
That might slow down all accesses, though, depending on the implementation (I
have no idea how it's actually implemented, but that's the effect of static
variables in C++ - an extra "is initialized" check on each access).

[alexmarkov, 2017-07-28 17:29:02]

Thank you for the review. Please take a look at the new version without large
const table.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:136: int negativeOverflowLimit =
_int64OverflowLimits[tableIndex + 1];
On 2017/07/27 23:40:18, Lasse Reichstein Nielsen wrote:
> Only read these if they are needed (which goes well with the separate loop
> suggested below).

Done (see the newer CL).

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:153: }
On 2017/07/27 23:40:18, Lasse Reichstein Nielsen wrote:
> This increases the inner loop size significantly if the condition is not
> optimized away by the compiler.
> 
> Can you check how much this affects performance when the flag isn't set?
> 
> Maybe have an entire different loop for the limit-to-64-bits case.
> Heck, maybe have an entire different method and algorithm since we know that
> there are at most two/three smis in 64 bits.
> (I'm also considering splitting the parsing into different loops for base <=
10
> and base > 10, instead of having the radix check in every call to _parseBlock.

Optimizer should be able to remove "if (_limitIntsTo64Bits)" entirely, because
_limitIntsTo64Bits is a compile-time constant. I did performance measurements on
micro-benchmark on my linux desktop and didn't notice any performance difference
in default mode (if --limit-ints-to-64-bits is not enabled). I'll double check
by looking into the optimized code for this method to ensure that the check for
_limitIntsTo64Bits is completely eliminated.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:226: // (--limit-ints-to-64-bits mode is
enabled).
On 2017/07/27 23:40:18, Lasse Reichstein Nielsen wrote:
> Use doc-comments ("///"). It might make some editors show the documentation
when
> hovering, and it can't hurt.

Done.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:230: static const _minInt64 = -_maxInt64 - 1;
On 2017/07/27 23:40:17, Lasse Reichstein Nielsen wrote:
> In the "limit integers to 64-bit mode", you should find a way to accept the
> expression `-0x8000000000000000`, even if it requires special-casing the unary
> minus+integer literal case.

It's a known problem and will be addressed separately. Unfortunately, for now
literals like -0x8000000000000000 or -9223372036854775808 do not work in
--limit-ints-to-64-bits mode. I'll correct this constant after such literals are
supported.

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:239: static const _int64OverflowLimits = const [
On 2017/07/27 23:40:17, Lasse Reichstein Nielsen wrote:
> Don't make it const, then it's evaluated the first time it's accessed.
> That might slow down all accesses, though, depending on the implementation (I
> have no idea how it's actually implemented, but that's the effect of static
> variables in C++ - an extra "is initialized" check on each access).

Done.

[alexmarkov, 2017-07-28 18:25:56]

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/2987003002/diff/1/runtime/lib/integers_patch....
runtime/lib/integers_patch.dart:153: }
On 2017/07/28 17:29:02, alexmarkov wrote:
> On 2017/07/27 23:40:18, Lasse Reichstein Nielsen wrote:
> > This increases the inner loop size significantly if the condition is not
> > optimized away by the compiler.
> > 
> > Can you check how much this affects performance when the flag isn't set?
> > 
> > Maybe have an entire different loop for the limit-to-64-bits case.
> > Heck, maybe have an entire different method and algorithm since we know that
> > there are at most two/three smis in 64 bits.
> > (I'm also considering splitting the parsing into different loops for base <=
> 10
> > and base > 10, instead of having the radix check in every call to
_parseBlock.
> 
> Optimizer should be able to remove "if (_limitIntsTo64Bits)" entirely, because
> _limitIntsTo64Bits is a compile-time constant. I did performance measurements
on
> micro-benchmark on my linux desktop and didn't notice any performance
difference
> in default mode (if --limit-ints-to-64-bits is not enabled). I'll double check
> by looking into the optimized code for this method to ensure that the check
for
> _limitIntsTo64Bits is completely eliminated.

As I expected, both JIT and AOT optimizers are able to remove the code "if
(_limitIntsTo64Bits) { ... }" entirely.

[zra, 2017-07-28 19:25:12]

lgtm

lgtm

[zra, 2017-07-28 19:25:12]

lgtm

[alexmarkov, 2017-07-31 15:31:51]

Description was changed from

==========
[corelib] Add explicit overflow checks to int.parse

In the --limit-ints-to-64-bits mode, when Dart integers are limited
to 64 bits, int.parse() should detect and reject large integers.
This CL adds explicit overflow checks into int.parse() implementation.

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

to

==========
[corelib] Add explicit overflow checks to int.parse

In the --limit-ints-to-64-bits mode, when Dart integers are limited
to 64 bits, int.parse() should detect and reject large integers.
This CL adds explicit overflow checks into int.parse() implementation.

R=lrn@google.com, zra@google.com

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

[alexmarkov, 2017-07-31 15:31:52]

Committed patchset #4 (id:60001) manually as
b64014c624ea51a0cf194c3d29fd7d16cd0a8cd6 (presubmit successful).