[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor

[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor

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

Review-Url: https://codereview.chromium.org/2799863002
Cr-Commit-Position: refs/heads/master@{#44542}
Committed: https://chromium.googlesource.com/v8/v8/+/14be6ae5e1d097a0bdcd94872d16e8fb9be08f2c

[aseemgarg, 2017-04-06 01:08:46]

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

[commit-bot: I haz the power, 2017-04-06 01:08:53]

Dry run: 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-06 01:15:22]

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

[commit-bot: I haz the power, 2017-04-06 01:15:23]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)

[aseemgarg, 2017-04-06 01:23:15]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-04-06 01:23:15]

The CQ bit was unchecked by aseemgarg@chromium.org

[aseemgarg, 2017-04-06 01:23:24]

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

[commit-bot: I haz the power, 2017-04-06 01:23:33]

Dry run: 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-06 01:46:45]

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

[commit-bot: I haz the power, 2017-04-06 01:46:45]

Dry run: This issue passed the CQ dry run.

[binji, 2017-04-06 19:04:50]

lgtm w/ some minor fixes

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

https://codereview.chromium.org/2799863002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:33: void
BinaryBuiltinCommon(Node* array, Node* index, Node* value, Node* context,
I think AtomicsRMW is a better name for this.

n/m looks like "binop" is used many places, perhaps "AtomicsBinop" is better

https://codereview.chromium.org/2799863002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:418: AssemblerFunction function =
[&](MachineType type, Node* base,         \
Since this lambda is just forwarding the values through to a function in
compiler::CodeAssembler, you could just have the parameter to
BinaryBuiltinCommon be a member function instead of a std::function.

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:467: } while (0)
need dmb(ISH) after too

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:787: __ j(negative, &binop);           
                         \
should be j(not_equal, &binop) I think

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:722: __ j(negative, &binop);             
                       \
should be j(not_equal, &binop)

https://codereview.chromium.org/2799863002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2799863002/diff/20001/src/objects.h#newcode6342
src/objects.h:6342: V(Add, add, AtomicsAdd)                             \
Why is this not V(Atomics, add, AtomicsAdd)?

[aseemgarg, 2017-04-06 21:03:56]

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

https://codereview.chromium.org/2799863002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:33: void
BinaryBuiltinCommon(Node* array, Node* index, Node* value, Node* context,
On 2017/04/06 19:04:50, binji wrote:
> I think AtomicsRMW is a better name for this.
> 
> n/m looks like "binop" is used many places, perhaps "AtomicsBinop" is better

Done.

https://codereview.chromium.org/2799863002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:418: AssemblerFunction function =
[&](MachineType type, Node* base,         \
On 2017/04/06 19:04:50, binji wrote:
> Since this lambda is just forwarding the values through to a function in
> compiler::CodeAssembler, you could just have the parameter to
> BinaryBuiltinCommon be a member function instead of a std::function.

Done.

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:467: } while (0)
On 2017/04/06 19:04:50, binji wrote:
> need dmb(ISH) after too

Done.

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:787: __ j(negative, &binop);           
                         \
On 2017/04/06 19:04:50, binji wrote:
> should be j(not_equal, &binop) I think

Done.

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/x64/code-g...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/2799863002/diff/20001/src/compiler/x64/code-g...
src/compiler/x64/code-generator-x64.cc:722: __ j(negative, &binop);             
                       \
On 2017/04/06 19:04:50, binji wrote:
> should be j(not_equal, &binop)

Done.

https://codereview.chromium.org/2799863002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2799863002/diff/20001/src/objects.h#newcode6342
src/objects.h:6342: V(Add, add, AtomicsAdd)                             \
On 2017/04/06 19:04:50, binji wrote:
> Why is this not V(Atomics, add, AtomicsAdd)?

Done.

[aseemgarg, 2017-04-06 23:08:27]

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

[commit-bot: I haz the power, 2017-04-06 23:08:34]

Dry run: 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-06 23:36:38]

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

[commit-bot: I haz the power, 2017-04-06 23:36:39]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-04-07 09:55:48]

lgtm

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

https://codereview.chromium.org/2799863002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:442: Node* value_integer =
ToInteger(context, value);
This makes me a bit uneasy, are we sure this cannot go wrong?

Historically, doing validation before a potential call to JS has been source of
serious security bugs (several 100K$ in Pwnium & Pwn2own just for typed array
bugs) because the checked preconditions could be invalidated and attackers would
get access to arbitrary memory. With SharedArrayBuffers, this might be
impossible because there is no neutering and all the buffers are external, but
it still feels dangerous.

At the very least, there should be a comment explaining why the validated
conditions are still valid after a call to arbitrary JS. 

It would be worth considering rechecking the conditions in debug mode.
This should be also accompanied by tests that do call to javascript from here
and manipulate the typed array so that clusterfuzz has a seed to check this
(i.e., for index and value, feed objects that do funky stuff in their valueOf
method).

[binji, 2017-04-08 20:23:12]

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

https://codereview.chromium.org/2799863002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:442: Node* value_integer =
ToInteger(context, value);
On 2017/04/07 09:55:48, Jarin wrote:
> This makes me a bit uneasy, are we sure this cannot go wrong?

I'm fairly certain this is safe, as a typedarray cannot change its length, and
the backing SAB can never be neutered or shrink. In the future when we add SAB
resizing for wasm the SAB may grow, but the backing store pointer will never
change.

> At the very least, there should be a comment explaining why the validated
> conditions are still valid after a call to arbitrary JS. 

Agreed.

> It would be worth considering rechecking the conditions in debug mode.
> This should be also accompanied by tests that do call to javascript from here
> and manipulate the typed array so that clusterfuzz has a seed to check this
> (i.e., for index and value, feed objects that do funky stuff in their valueOf
> method).

Good idea. I noticed that TruncateTaggedToWord32 might also call out to JS,
which would create a validation loop if we actually wanted to check the typed
array length. But the length is always the internal value, regardless of whether
the user defines a new length getter. In fact, the runtime equivalents already
do CHECK_LT(index, NumberToSize(sta->length())) where NumberToSize has a check
that the value is a Smi or HeapNumber, so we should be able to do the same here.

[aseemgarg, 2017-04-10 20:21:14]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-04-10 20:21:15]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org,
jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2799863002/#ps80001
(title: "[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor")

[commit-bot: I haz the power, 2017-04-10 20:21:17]

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

[aseemgarg, 2017-04-10 20:23:38]

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

https://codereview.chromium.org/2799863002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer-gen.cc:442: Node* value_integer =
ToInteger(context, value);
Moved the JS calls above the validations and added tests that try to neuter the
SAB.

On 2017/04/08 20:23:12, binji wrote:
> On 2017/04/07 09:55:48, Jarin wrote:
> > This makes me a bit uneasy, are we sure this cannot go wrong?
> 
> I'm fairly certain this is safe, as a typedarray cannot change its length, and
> the backing SAB can never be neutered or shrink. In the future when we add SAB
> resizing for wasm the SAB may grow, but the backing store pointer will never
> change.
> 
> > At the very least, there should be a comment explaining why the validated
> > conditions are still valid after a call to arbitrary JS. 
> 
> Agreed.
> 
> > It would be worth considering rechecking the conditions in debug mode.
> > This should be also accompanied by tests that do call to javascript from
here
> > and manipulate the typed array so that clusterfuzz has a seed to check this
> > (i.e., for index and value, feed objects that do funky stuff in their
valueOf
> > method).
> 
> Good idea. I noticed that TruncateTaggedToWord32 might also call out to JS,
> which would create a validation loop if we actually wanted to check the typed
> array length. But the length is always the internal value, regardless of
whether
> the user defines a new length getter. In fact, the runtime equivalents already
> do CHECK_LT(index, NumberToSize(sta->length())) where NumberToSize has a check
> that the value is a Smi or HeapNumber, so we should be able to do the same
here.

[commit-bot: I haz the power, 2017-04-10 20:46:22]

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

[commit-bot: I haz the power, 2017-04-10 20:46:23]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/24258)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/24160)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...)

[aseemgarg, 2017-04-10 23:40:32]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2017-04-10 23:40:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
binji@chromium.org
Link to the patchset: https://codereview.chromium.org/2799863002/#ps100001
(title: "[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor")

[commit-bot: I haz the power, 2017-04-10 23:40:41]

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-11 00:09:38]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491867632999330,
"parent_rev": "dbfc03005761b0f78ede078a00f0cc0a898fe8bd", "commit_rev":
"14be6ae5e1d097a0bdcd94872d16e8fb9be08f2c"}

[commit-bot: I haz the power, 2017-04-11 00:09:45]

Description was changed from

==========
[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor

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

to

==========
[Atomics] use TFJ builtins for atomic add, sub, and, or, and xor

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

Review-Url: https://codereview.chromium.org/2799863002
Cr-Commit-Position: refs/heads/master@{#44542}
Committed:
https://chromium.googlesource.com/v8/v8/+/14be6ae5e1d097a0bdcd94872d16e8fb9be...
==========

[commit-bot: I haz the power, 2017-04-11 00:09:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/14be6ae5e1d097a0bdcd94872d16e8fb9be...

[Jarin, 2017-04-11 08:23:16]

https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
File src/builtins/builtins-sharedarraybuffer-gen.cc (right):

https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer-gen.cc:217: Node* value_integer =
ToInteger(context, value);
I believe this is incorrect because the spec says the ToInteger has to be called
after the validation and this is observable (if you supply an invalid index, the
tointeger should not be called). See
https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.ReadModifyWrite

Could you write tests that check the order complies with the spec?

[binji, 2017-04-11 18:02:45]

On 2017/04/11 at 08:23:16, jarin wrote:
>
https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
> File src/builtins/builtins-sharedarraybuffer-gen.cc (right):
> 
>
https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
> src/builtins/builtins-sharedarraybuffer-gen.cc:217: Node* value_integer =
ToInteger(context, value);
> I believe this is incorrect because the spec says the ToInteger has to be
called after the validation and this is observable (if you supply an invalid
index, the tointeger should not be called). See
https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.ReadModifyWrite
> 
> Could you write tests that check the order complies with the spec?

You're right, I wrote a test that breaks this. I'll work with Aseem to follow up
with a fix for this.

[binji, 2017-04-11 18:38:35]

On 2017/04/11 at 18:02:45, binji wrote:
> On 2017/04/11 at 08:23:16, jarin wrote:
> >
https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
> > File src/builtins/builtins-sharedarraybuffer-gen.cc (right):
> > 
> >
https://codereview.chromium.org/2799863002/diff/100001/src/builtins/builtins-...
> > src/builtins/builtins-sharedarraybuffer-gen.cc:217: Node* value_integer =
ToInteger(context, value);
> > I believe this is incorrect because the spec says the ToInteger has to be
called after the validation and this is observable (if you supply an invalid
index, the tointeger should not be called). See
https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.ReadModifyWrite
> > 
> > Could you write tests that check the order complies with the spec?
> 
> You're right, I wrote a test that breaks this. I'll work with Aseem to follow
up with a fix for this.

Starting on https://codereview.chromium.org/2814753003.