[SAB] Validate index before value conversion

[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,jkummerow@chromium.org

Review-Url: https://codereview.chromium.org/2814753003
Cr-Commit-Position: refs/heads/master@{#44626}
Committed: https://chromium.googlesource.com/v8/v8/+/7b300ba2e966c0d104d3755707a6d59a74944771

[binji, 2017-04-11 20:31:47]

Description was changed from

==========
[SAB] Validate index before value conversion

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

BUG=v8:4614
R=jarin@chromium.org,aseemgarg@chromium.org
==========

to

==========
[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,aseemgarg@chromium.org,jkummerow@chromium.org
==========

[binji, 2017-04-11 20:39:23]

binji@chromium.org changed reviewers:
+ jkummerow@chromium.org
- aseemgarg@chromium.org

[binji, 2017-04-11 20:39:27]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:39:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[binji, 2017-04-11 20:39:39]

Description was changed from

==========
[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,aseemgarg@chromium.org,jkummerow@chromium.org
==========

to

==========
[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,jkummerow@chromium.org
==========

[commit-bot: I haz the power, 2017-04-11 21:12:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 21:12:01]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-04-11 23:11:50]

aseemgarg@chromium.org changed reviewers:
+ aseemgarg@chromium.org

[aseemgarg, 2017-04-11 23:11:50]

lgtm with nits

https://codereview.chromium.org/2814753003/diff/20001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2814753003/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1878: temp[0] =
g.UseByteRegister(node);
Although this is correct, it might be better to force byte-register (edx) only
if it is Int8 or Uint8 type.

[binji, 2017-04-11 23:49:12]

https://codereview.chromium.org/2814753003/diff/20001/src/compiler/ia32/instr...
File src/compiler/ia32/instruction-selector-ia32.cc (right):

https://codereview.chromium.org/2814753003/diff/20001/src/compiler/ia32/instr...
src/compiler/ia32/instruction-selector-ia32.cc:1878: temp[0] =
g.UseByteRegister(node);
On 2017/04/11 at 23:11:50, aseemgarg wrote:
> Although this is correct, it might be better to force byte-register (edx) only
if it is Int8 or Uint8 type.

Done.

[binji, 2017-04-11 23:49:18]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 23:49:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aseemgarg, 2017-04-11 23:49:55]

lgtm

[commit-bot: I haz the power, 2017-04-12 00:13:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:13:16]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-04-12 04:39:01]

lgtm. Thanks!

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer-gen.cc (right):

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:341: ValidateAtomicIndex(array,
index_word32, context);
Nit: It would be even better if we could use here a version of validation that
CSA_ASSERTs (instead of throwing).

[Jakob Kummerow, 2017-04-12 11:47:33]

LGTM with comments (typed those before discovering that Jaro agrees :-) )

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer-gen.cc (right):

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:201: #if DEBUG
This sounds like a use case for CSA_ASSERT. If the sanity check fails, you want
to crash hard, not just throw a harmless exception.

  CSA_ASSERT(
      this,
      Uint32LessThan(
          index_word32,
          TruncateTaggedToWord32(
              context, LoadObjectField(array, JSTypedArray::kLengthOffset))));

(or SmiToWord32 instead of TruncateTaggedToWord32 if the stored length is
guaranteed to be a Smi).

https://codereview.chromium.org/2814753003/diff/40001/test/mjsunit/harmony/at...
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/2814753003/diff/40001/test/mjsunit/harmony/at...
test/mjsunit/harmony/atomics.js:571: var dummy = 0;
maybe s/dummy/valueof_has_been_called/? Descriptive names FTW!

[binji, 2017-04-12 18:43:44]

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer-gen.cc (right):

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:201: #if DEBUG
On 2017/04/12 at 11:47:33, Jakob Kummerow wrote:
> This sounds like a use case for CSA_ASSERT. If the sanity check fails, you
want to crash hard, not just throw a harmless exception.
> 
>   CSA_ASSERT(
>       this,
>       Uint32LessThan(
>           index_word32,
>           TruncateTaggedToWord32(
>               context, LoadObjectField(array, JSTypedArray::kLengthOffset))));
> 
> (or SmiToWord32 instead of TruncateTaggedToWord32 if the stored length is
guaranteed to be a Smi).

Done, thanks! Didn't know about CSA_ASSERT.

https://codereview.chromium.org/2814753003/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:341: ValidateAtomicIndex(array,
index_word32, context);
On 2017/04/12 at 04:39:01, Jarin wrote:
> Nit: It would be even better if we could use here a version of validation that
CSA_ASSERTs (instead of throwing).

Done.

https://codereview.chromium.org/2814753003/diff/40001/test/mjsunit/harmony/at...
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/2814753003/diff/40001/test/mjsunit/harmony/at...
test/mjsunit/harmony/atomics.js:571: var dummy = 0;
On 2017/04/12 at 11:47:33, Jakob Kummerow wrote:
> maybe s/dummy/valueof_has_been_called/? Descriptive names FTW!

Good point, done!

[binji, 2017-04-12 18:43:54]

The CQ bit was checked by binji@chromium.org

[binji, 2017-04-12 18:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
aseemgarg@chromium.org, jarin@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/2814753003/#ps60001
(title: "feedback")

[commit-bot: I haz the power, 2017-04-12 18:44:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:08:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492022634529040,
"parent_rev": "9c1dcfd9114e7f3ee9ebfe3785532c8443c29f01", "commit_rev":
"7b300ba2e966c0d104d3755707a6d59a74944771"}

[commit-bot: I haz the power, 2017-04-12 19:08:48]

Description was changed from

==========
[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,jkummerow@chromium.org
==========

to

==========
[SAB] Validate index before value conversion using ToIndex

It's required by the spec -- and observable -- that the index be validated
before the conversion of the value(s) via ToInteger.

The previous implementation also had an old test for validating the atomic
index, which has now been switched to ToIndex.

This also exposed an issue in the ia32 code generator: cmpxchg_b requires a
byte register, but the ia32 instruction selector was ensuring that the
new_value was a byte register, not the TempRegister. This change forces the
temp register to use edx, which always can be used as a byte register (dl).
This is the same behavior as currently used in UseByteRegister.

BUG=v8:4614
R=jarin@chromium.org,jkummerow@chromium.org

Review-Url: https://codereview.chromium.org/2814753003
Cr-Commit-Position: refs/heads/master@{#44626}
Committed:
https://chromium.googlesource.com/v8/v8/+/7b300ba2e966c0d104d3755707a6d59a749...
==========

[commit-bot: I haz the power, 2017-04-12 19:08:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/7b300ba2e966c0d104d3755707a6d59a749...