Fix a few potential integer negation overflows

Fix a few potential integer negation overflows

AFAICT none of these can actually be triggered currently; but it's still good to harden the code a little.

Committed: https://crrev.com/e02807ee8a6a254c694da007420ef78b40ac9f62
Cr-Commit-Position: refs/heads/master@{#27848}

[Jakob Kummerow, 2015-04-15 08:57:32]

jkummerow@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Jakob Kummerow, 2015-04-15 08:57:32]

PTAL.

[Sven Panne, 2015-04-15 12:33:33]

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
src/arm64/lithium-codegen-arm64.cc:4319: DCHECK((right > -kMaxInt) && (right <
kMaxInt));
Hmmmm, why do we exclude -kMaxInt here? Shouldn't this be OK?

[Jakob Kummerow, 2015-04-15 12:40:24]

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
src/arm64/lithium-codegen-arm64.cc:4319: DCHECK((right > -kMaxInt) && (right <
kMaxInt));
On 2015/04/15 12:33:33, Sven Panne wrote:
> Hmmmm, why do we exclude -kMaxInt here? Shouldn't this be OK?

Nope, there are expressions like "-right" (line 4412) and "right - 1" (line
4401) below.

[Sven Panne, 2015-04-15 13:02:41]

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1058533007/diff/1/src/arm64/lithium-codegen-a...
src/arm64/lithium-codegen-arm64.cc:4319: DCHECK((right > -kMaxInt) && (right <
kMaxInt));
On 2015/04/15 12:40:24, Jakob wrote:
> On 2015/04/15 12:33:33, Sven Panne wrote:
> > Hmmmm, why do we exclude -kMaxInt here? Shouldn't this be OK?
> 
> Nope, there are expressions like "-right" (line 4412) and "right - 1" (line
> 4401) below.

Uh, oh... Looking at LChunkBuilder::DoMul for ARM64 (only on that platform, why
do we do things differently here???), we already enter undefined-behavior-land
when trying to compute the absolute value of kMinInt. :-P I think we need to fix
this, making sure that kMinInt, -kMaxInt etc. are handled correctly. Otherwise
I'm not sure that we don't hit this assertion, or if we're lucky, or if we
really never fail the DCHECK, or...

[Jakob Kummerow, 2015-04-15 13:14:49]

On 2015/04/15 13:02:41, Sven Panne wrote:
> Uh, oh... Looking at LChunkBuilder::DoMul for ARM64 (only on that platform,
why
> do we do things differently here???), 

Don't know, don't care, Lithium is deprecated.

> we already enter undefined-behavior-land
> when trying to compute the absolute value of kMinInt. :-P I think we need to
fix
> this, making sure that kMinInt, -kMaxInt etc. are handled correctly. Otherwise
> I'm not sure that we don't hit this assertion, or if we're lucky, or if we
> really never fail the DCHECK, or...

Well, we'll only use the result of the Abs() call if the original value was
*not* kMinInt. Also, it just so happens that with our current compilers,
computing -kMinInt returns kMinInt, without any kitten-killing side effects.
I've stared at this code quite a bit and I'm convinced that it's currently safe.
(We can pull the Abs() computating behind the "if (!end_range_constant)" check
if it let you sleep better.)

I agree that ideally we'd fix all of the uses of Abs() all over the code base
(there are many more), but that's a pretty big yak shave for unclear benefit.
I'd rather land this CL as-is as an incremental step towards more
safety/correctness.

[Sven Panne, 2015-04-15 13:24:04]

lgtm

[Jakob Kummerow, 2015-04-15 13:25:31]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2015-04-15 13:25:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1058533007/1

[commit-bot: I haz the power, 2015-04-15 13:55:34]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-15 13:55:44]

Patchset 1 (id:??) landed as
https://crrev.com/e02807ee8a6a254c694da007420ef78b40ac9f62
Cr-Commit-Position: refs/heads/master@{#27848}