Re-land "Clusterfuzz identified overflow check needed in dehoisting."

Re-land "Clusterfuzz identified overflow check needed in dehoisting."

BUG=380092
LOG=N
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21920

[mvstanton, 2014-06-18 14:02:11]

Hi Danno,
PTAL, here is the dehoist overflow CL now with the use of the safe numerics
library for overflow checking. It is fitting that it was kicked out of the tree
last week because of a buggy "hand crafted" overflow checker. :)

Note - I kept the base_offset in HLoad/StoreKeyed as uint32_t because there is
safe addition now and it confuses me to store a "signed" integer in the bitfield
of HLoadKeyed.

Hi Jakob, please see the wrappers I added in utils.h for the use of safe
numerics, and a small test. The library itself is exhaustively tested in
chromium.

[danno, 2014-06-18 14:55:04]

https://codereview.chromium.org/335063005/diff/40001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/335063005/diff/40001/src/utils.h#newcode256
src/utils.h:256: template<typename T> bool MultiplyOverflows(T x, T y) {
Do you really need these wrappers? It seems that they make you do the math
twice. Is there really any downside to having the CheckedNumber<T> directly at
the site where it's used... and just using the checked_result.value()?

[Jakob Kummerow, 2014-06-18 14:57:32]

I'm afraid I don't agree with these wrappers, because they require callers to
perform the actual computation again, making the overflow check optional. On
success, the result should be retrieved from the checked_result object (so that
maybe long-term we can entirely blacklist unchecked operations). That requires
either the wrapper to return both a bool and a value, or the CheckedNumerics
object to be used directly.

Also, I think for the multiplication part, the actual bug is that "return 31 -
ElementsKindToShiftSize(elements_kind_);" should s/31/30/ (or the caller must "1
<< (array_operation->MaxBaseOffsetBits() - 1)", because it's used as shift width
and 1 << 31 overflows. With that fixed, we still need to guard the addition,
where safe_math comes in handy.

[mvstanton, 2014-06-18 15:19:22]

Thx yall. I have eliminated the ill-chosen wrappers. 

Jakob, are you saying I don't need to check the multiplication result for
overflow? I have in the test a case where it would overflow, sending value
negative in DehoistArrayIndex before trying to add it to the existing Array base
offset.

[Jakob Kummerow, 2014-06-20 08:24:50]

> Jakob, are you saying I don't need to check the multiplication result for
> overflow? I have in the test a case where it would overflow, sending value
> negative in DehoistArrayIndex before trying to add it to the existing Array
base
> offset.

I'm saying: if you fixed MaxBaseOffsetBits() to return what it's supposed to
return, then you wouldn't have to check the multiplication because it would then
be guaranteed not to overflow. But putting all the validity checking logic into
the new Try...() method is fine too.

LGTM with nits.

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3495: addition_result = addition_result +
increase_by_value;
nit: consider using += for conciseness.

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3497: !BaseOffsetField::is_valid(base_offset +
increase_by_value)) {
I'd split this check in two to make it clearer what's going on:

addition_result += increase_by_value;
if (!addition_result.IsValid()) return false;
base_offset = addition_result.ValueOrDie();
if (!BaseOffsetField::is_valid(base_offset)) return false;
bit_field_ = ...update(..., base_offset);

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4090: addition_result = addition_result +
increase_by_value;
nit: +=

[mvstanton, 2014-06-23 08:31:14]

thx for comments! Submitting...

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3495: addition_result = addition_result +
increase_by_value;
On 2014/06/20 08:24:50, Jakob wrote:
> nit: consider using += for conciseness.

Done.

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3497: !BaseOffsetField::is_valid(base_offset +
increase_by_value)) {
On 2014/06/20 08:24:50, Jakob wrote:
> I'd split this check in two to make it clearer what's going on:
> 
> addition_result += increase_by_value;
> if (!addition_result.IsValid()) return false;
> base_offset = addition_result.ValueOrDie();
> if (!BaseOffsetField::is_valid(base_offset)) return false;
> bit_field_ = ...update(..., base_offset);

Good idea, thx, I like it better too.

https://codereview.chromium.org/335063005/diff/80001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4090: addition_result = addition_result +
increase_by_value;
On 2014/06/20 08:24:50, Jakob wrote:
> nit: +=

Done.

[mvstanton, 2014-06-23 09:09:14]

Committed patchset #4 manually as r21920 (presubmit successful).