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

src/builtins/builtins-sharedarraybuffer-gen.cc

Index: src/builtins/builtins-sharedarraybuffer-gen.cc
diff --git a/src/builtins/builtins-sharedarraybuffer-gen.cc b/src/builtins/builtins-sharedarraybuffer-gen.cc
index 318860cac47f884cfaa6f09c7c96a6f6a1543526..056dfc6e1a5c27c0f6c289b3af17ca57b2141c79 100644
--- a/src/builtins/builtins-sharedarraybuffer-gen.cc
+++ b/src/builtins/builtins-sharedarraybuffer-gen.cc
@@ -19,6 +19,9 @@ class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {
       : CodeStubAssembler(state) {}
 
  protected:
+  typedef Node* (CodeAssembler::*AssemblerFunction)(MachineType type,
+                                                    Node* base, Node* offset,
+                                                    Node* value);
   void ValidateSharedTypedArray(Node* tagged, Node* context,
                                 Node** out_instance_type,
                                 Node** out_backing_store);
@@ -26,6 +29,9 @@ class SharedArrayBufferBuiltinsAssembler : public CodeStubAssembler {
                                          Node** number_index);
   void ValidateAtomicIndex(Node* index_word, Node* array_length_word,
                            Node* context);
+  void AtomicBinopBuiltinCommon(Node* array, Node* index, Node* value,
+                                Node* context, AssemblerFunction function,
+                                Runtime::FunctionId runtime_function);
 };
 
 void SharedArrayBufferBuiltinsAssembler::ValidateSharedTypedArray(
@@ -144,13 +150,14 @@ TF_BUILTIN(AtomicsLoad, SharedArrayBufferBuiltinsAssembler) {
   Node* index = Parameter(Descriptor::kIndex);
   Node* context = Parameter(Descriptor::kContext);
 
+  Node* index_integer;
+  Node* index_word32 =
+      ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+
   Node* instance_type;
   Node* backing_store;
   ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
 
-  Node* index_integer;
-  Node* index_word32 =
-      ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
   Node* array_length_word32 = TruncateTaggedToWord32(
       context, LoadObjectField(array, JSTypedArray::kLengthOffset));
   ValidateAtomicIndex(index_word32, array_length_word32, context);
@@ -203,21 +210,26 @@ TF_BUILTIN(AtomicsStore, SharedArrayBufferBuiltinsAssembler) {
   Node* value = Parameter(Descriptor::kValue);
   Node* context = Parameter(Descriptor::kContext);
 
-  Node* instance_type;
-  Node* backing_store;
-  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+  // The value_integer needs to be computed before the validations as the
+  // ToInteger function can be potentially modified in JS to invalidate the
+  // conditions. This is just a no-cost safety measure as SABs can't be neutered
+  // or shrunk.
+  Node* value_integer = ToInteger(context, value);

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

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?

+  Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
 
   Node* index_integer;
   Node* index_word32 =
       ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+
+  Node* instance_type;
+  Node* backing_store;
+  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+
   Node* array_length_word32 = TruncateTaggedToWord32(
       context, LoadObjectField(array, JSTypedArray::kLengthOffset));
   ValidateAtomicIndex(index_word32, array_length_word32, context);
   Node* index_word = ChangeUint32ToWord(index_word32);
 
-  Node* value_integer = ToInteger(context, value);
-  Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
-
   Label u8(this), u16(this), u32(this), other(this);
   int32_t case_values[] = {
       FIXED_INT8_ARRAY_TYPE,   FIXED_UINT8_ARRAY_TYPE, FIXED_INT16_ARRAY_TYPE,
@@ -255,19 +267,24 @@ TF_BUILTIN(AtomicsExchange, SharedArrayBufferBuiltinsAssembler) {
   Node* value = Parameter(Descriptor::kValue);
   Node* context = Parameter(Descriptor::kContext);
 
-  Node* instance_type;
-  Node* backing_store;
-  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+  // The value_integer needs to be computed before the validations as the
+  // ToInteger function can be potentially modified in JS to invalidate the
+  // conditions. This is just a no-cost safety measure as SABs can't be neutered
+  // or shrunk.
+  Node* value_integer = ToInteger(context, value);
 
   Node* index_integer;
   Node* index_word32 =
       ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+
+  Node* instance_type;
+  Node* backing_store;
+  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+
   Node* array_length_word32 = TruncateTaggedToWord32(
       context, LoadObjectField(array, JSTypedArray::kLengthOffset));
   ValidateAtomicIndex(index_word32, array_length_word32, context);
 
-  Node* value_integer = ToInteger(context, value);
-
 #if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64
   Return(CallRuntime(Runtime::kAtomicsExchange, context, array, index_integer,
                      value_integer));
@@ -327,21 +344,25 @@ TF_BUILTIN(AtomicsCompareExchange, SharedArrayBufferBuiltinsAssembler) {
   Node* new_value = Parameter(Descriptor::kNewValue);
   Node* context = Parameter(Descriptor::kContext);
 
-  Node* instance_type;
-  Node* backing_store;
-  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+  // The value_integers needs to be computed before the validations as the
+  // ToInteger function can be potentially modified in JS to invalidate the
+  // conditions. This is just a no-cost safety measure as SABs can't be neutered
+  // or shrunk.
+  Node* old_value_integer = ToInteger(context, old_value);
+  Node* new_value_integer = ToInteger(context, new_value);
 
   Node* index_integer;
   Node* index_word32 =
       ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+
+  Node* instance_type;
+  Node* backing_store;
+  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+
   Node* array_length_word32 = TruncateTaggedToWord32(
       context, LoadObjectField(array, JSTypedArray::kLengthOffset));
   ValidateAtomicIndex(index_word32, array_length_word32, context);
 
-  Node* old_value_integer = ToInteger(context, old_value);
-
-  Node* new_value_integer = ToInteger(context, new_value);
-
 #if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64 || \
     V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X
   Return(CallRuntime(Runtime::kAtomicsCompareExchange, context, array,
@@ -402,5 +423,99 @@ TF_BUILTIN(AtomicsCompareExchange, SharedArrayBufferBuiltinsAssembler) {
         // || V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X
 }
 
+#define BINOP_BUILTIN(op)                                       \
+  TF_BUILTIN(Atomics##op, SharedArrayBufferBuiltinsAssembler) { \
+    Node* array = Parameter(Descriptor::kArray);                \
+    Node* index = Parameter(Descriptor::kIndex);                \
+    Node* value = Parameter(Descriptor::kValue);                \
+    Node* context = Parameter(Descriptor::kContext);            \
+    AtomicBinopBuiltinCommon(array, index, value, context,      \
+                             &CodeAssembler::Atomic##op,        \
+                             Runtime::kAtomics##op);            \
+  }
+BINOP_BUILTIN(Add)
+BINOP_BUILTIN(Sub)
+BINOP_BUILTIN(And)
+BINOP_BUILTIN(Or)
+BINOP_BUILTIN(Xor)
+#undef BINOP_BUILTIN
+
+void SharedArrayBufferBuiltinsAssembler::AtomicBinopBuiltinCommon(
+    Node* array, Node* index, Node* value, Node* context,
+    AssemblerFunction function, Runtime::FunctionId runtime_function) {
+  // The value_integer needs to be computed before the validations as the
+  // ToInteger function can be potentially modified in JS to invalidate the
+  // conditions. This is just a no-cost safety measure as SABs can't be neutered
+  // or shrunk.
+  Node* value_integer = ToInteger(context, value);
+
+  Node* index_integer;
+  Node* index_word32 =
+      ConvertTaggedAtomicIndexToWord32(index, context, &index_integer);
+
+  Node* instance_type;
+  Node* backing_store;
+  ValidateSharedTypedArray(array, context, &instance_type, &backing_store);
+
+  Node* array_length_word32 = TruncateTaggedToWord32(
+      context, LoadObjectField(array, JSTypedArray::kLengthOffset));
+  ValidateAtomicIndex(index_word32, array_length_word32, context);
+
+#if V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64 || \
+    V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X
+  Return(CallRuntime(runtime_function, context, array, index_integer,
+                     value_integer));
+#else
+  Node* index_word = ChangeUint32ToWord(index_word32);
+
+  Node* value_word32 = TruncateTaggedToWord32(context, value_integer);
+
+  Label i8(this), u8(this), i16(this), u16(this), i32(this), u32(this),
+      other(this);
+  int32_t case_values[] = {
+      FIXED_INT8_ARRAY_TYPE,   FIXED_UINT8_ARRAY_TYPE, FIXED_INT16_ARRAY_TYPE,
+      FIXED_UINT16_ARRAY_TYPE, FIXED_INT32_ARRAY_TYPE, FIXED_UINT32_ARRAY_TYPE,
+  };
+  Label* case_labels[] = {
+      &i8, &u8, &i16, &u16, &i32, &u32,
+  };
+  Switch(instance_type, &other, case_values, case_labels,
+         arraysize(case_labels));
+
+  Bind(&i8);
+  Return(SmiFromWord32((this->*function)(MachineType::Int8(), backing_store,
+                                         index_word, value_word32)));
+
+  Bind(&u8);
+  Return(SmiFromWord32((this->*function)(MachineType::Uint8(), backing_store,
+                                         index_word, value_word32)));
+
+  Bind(&i16);
+  Return(
+      SmiFromWord32((this->*function)(MachineType::Int16(), backing_store,
+                                      WordShl(index_word, 1), value_word32)));
+
+  Bind(&u16);
+  Return(
+      SmiFromWord32((this->*function)(MachineType::Uint16(), backing_store,
+                                      WordShl(index_word, 1), value_word32)));
+
+  Bind(&i32);
+  Return(ChangeInt32ToTagged(
+      (this->*function)(MachineType::Int32(), backing_store,
+                        WordShl(index_word, 2), value_word32)));
+
+  Bind(&u32);
+  Return(ChangeUint32ToTagged(
+      (this->*function)(MachineType::Uint32(), backing_store,
+                        WordShl(index_word, 2), value_word32)));
+
+  // This shouldn't happen, we've already validated the type.
+  Bind(&other);
+  Unreachable();
+#endif  // V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64 || V8_TARGET_ARCH_PPC64
+        // || V8_TARGET_ARCH_PPC || V8_TARGET_ARCH_S390 || V8_TARGET_ARCH_S390X
+}
+
 }  // namespace internal
 }  // namespace v8