[compiler] Better encapsulation of the TSAN Relaxed codegen stores

Introduce EmitTSANStoreOOLIfNeeded methods which make it easier on the
eyes in code-generator.cc.

Also pass along the size, which lays the groundwork for the other
instructions e.g. kX64Movq since we don't require the store to be a
Tagged one. This creates new builtins (since we now have a version with
32 bits and another one for 64 bits stores). We can extract the common
code in builtins-internal-gen.cc to de-duplicate the common code.

Bug: v8:7790, v8:11600
Change-Id: I81d80b852ec96b94d170a20f6d61621743b74b32
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2933664Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74971}

src/builtins/builtins-definitions.h

@@ -41,8 +41,10 @@ namespace internal {
   TFC(EphemeronKeyBarrierIgnoreFP, WriteBarrier)                               \
                                                                                \
   /* TSAN support for tagged stores in generated code.*/                       \
-  IF_TSAN(TFC, TSANRelaxedStoreIgnoreFP, TSANRelaxedStore)                     \
-  IF_TSAN(TFC, TSANRelaxedStoreSaveFP, TSANRelaxedStore)                       \
+  IF_TSAN(TFC, TSANRelaxedStore32IgnoreFP, TSANRelaxedStore)                   \
+  IF_TSAN(TFC, TSANRelaxedStore32SaveFP, TSANRelaxedStore)                     \
+  IF_TSAN(TFC, TSANRelaxedStore64IgnoreFP, TSANRelaxedStore)                   \
+  IF_TSAN(TFC, TSANRelaxedStore64SaveFP, TSANRelaxedStore)                     \
                                                                                \
   /* Adaptor for CPP builtin */                                                \
   TFC(AdaptorWithBuiltinExitFrame, CppBuiltinAdaptor)                          \

src/builtins/builtins-internal-gen.cc

@@ -9,6 +9,7 @@
 #include "src/codegen/code-stub-assembler.h"
 #include "src/codegen/interface-descriptors-inl.h"
 #include "src/codegen/macro-assembler.h"
+#include "src/common/globals.h"
 #include "src/execution/frame-constants.h"
 #include "src/heap/memory-chunk.h"
 #include "src/ic/accessor-assembler.h"
@@ -418,32 +419,46 @@ class TSANRelaxedStoreCodeStubAssembler : public CodeStubAssembler {
   explicit TSANRelaxedStoreCodeStubAssembler(
       compiler::CodeAssemblerState* state)
       : CodeStubAssembler(state) {}
-};
 
-TF_BUILTIN(TSANRelaxedStoreIgnoreFP, TSANRelaxedStoreCodeStubAssembler) {
-  TNode<ExternalReference> function =
-      ExternalConstant(ExternalReference::tsan_relaxed_store_function());
-  auto address = UncheckedParameter<IntPtrT>(Descriptor::kAddress);
-  TNode<IntPtrT> value =
-      BitcastTaggedToWord(UncheckedParameter<Object>(Descriptor::kValue));
+  TNode<ExternalReference> GetExternalReference(int size) {
+    if (size == kInt32Size) {
+      return ExternalConstant(
+          ExternalReference::tsan_relaxed_store_function_32_bits());
+    } else {
+      CHECK_EQ(size, kInt64Size);
+      return ExternalConstant(
+          ExternalReference::tsan_relaxed_store_function_64_bits());
+    }
+  }
+
+  void GenerateTSANRelaxedStore(SaveFPRegsMode fp_mode, int size) {
+    TNode<ExternalReference> function = GetExternalReference(size);
+    auto address =
+        UncheckedParameter<IntPtrT>(TSANRelaxedStoreDescriptor::kAddress);
+    TNode<IntPtrT> value = BitcastTaggedToWord(
+        UncheckedParameter<Object>(TSANRelaxedStoreDescriptor::kValue));
     CallCFunctionWithCallerSavedRegisters(
-        function, MachineType::Int32(), SaveFPRegsMode::kIgnore,
+        function, MachineType::Int32(), fp_mode,
         std::make_pair(MachineType::IntPtr(), address),
         std::make_pair(MachineType::IntPtr(), value));
     Return(UndefinedConstant());
+  }
+};
+
+TF_BUILTIN(TSANRelaxedStore32IgnoreFP, TSANRelaxedStoreCodeStubAssembler) {
+  GenerateTSANRelaxedStore(SaveFPRegsMode::kIgnore, kInt32Size);
 }
 
-TF_BUILTIN(TSANRelaxedStoreSaveFP, TSANRelaxedStoreCodeStubAssembler) {
-  TNode<ExternalReference> function =
-      ExternalConstant(ExternalReference::tsan_relaxed_store_function());
-  auto address = UncheckedParameter<IntPtrT>(Descriptor::kAddress);
-  TNode<IntPtrT> value =
-      BitcastTaggedToWord(UncheckedParameter<Object>(Descriptor::kValue));
-  CallCFunctionWithCallerSavedRegisters(
-      function, MachineType::Int32(), SaveFPRegsMode::kSave,
-      std::make_pair(MachineType::IntPtr(), address),
-      std::make_pair(MachineType::IntPtr(), value));
-  Return(UndefinedConstant());
+TF_BUILTIN(TSANRelaxedStore32SaveFP, TSANRelaxedStoreCodeStubAssembler) {
+  GenerateTSANRelaxedStore(SaveFPRegsMode::kSave, kInt32Size);
+}
+
+TF_BUILTIN(TSANRelaxedStore64IgnoreFP, TSANRelaxedStoreCodeStubAssembler) {
+  GenerateTSANRelaxedStore(SaveFPRegsMode::kIgnore, kInt64Size);
+}
+
+TF_BUILTIN(TSANRelaxedStore64SaveFP, TSANRelaxedStoreCodeStubAssembler) {
+  GenerateTSANRelaxedStore(SaveFPRegsMode::kSave, kInt64Size);
 }
 #endif  // V8_IS_TSAN
 

src/builtins/builtins.h

@@ -114,10 +114,17 @@ class Builtins {
   }
 
 #ifdef V8_IS_TSAN
-  static Name GetTSANRelaxedStoreStub(SaveFPRegsMode fp_mode) {
-    return fp_mode == SaveFPRegsMode::kIgnore
-               ? Builtins::kTSANRelaxedStoreIgnoreFP
-               : Builtins::kTSANRelaxedStoreSaveFP;
+  static Name GetTSANRelaxedStoreStub(SaveFPRegsMode fp_mode, int size) {
+    if (size == kInt32Size) {
+      return fp_mode == SaveFPRegsMode::kIgnore
+                 ? Builtins::kTSANRelaxedStore32IgnoreFP
+                 : Builtins::kTSANRelaxedStore32SaveFP;
+    } else {
+      CHECK_EQ(size, kInt64Size);
+      return fp_mode == SaveFPRegsMode::kIgnore
+                 ? Builtins::kTSANRelaxedStore64IgnoreFP
+                 : Builtins::kTSANRelaxedStore64SaveFP;
+    }
   }
 #endif  // V8_IS_TSAN
 

src/codegen/external-reference.cc

@@ -1161,26 +1161,36 @@ FUNCTION_REFERENCE(atomic_pair_compare_exchange_function,
                    atomic_pair_compare_exchange)
 
 #ifdef V8_IS_TSAN
-// Mimics the store in generated code (generated by kArchStoreWithWriteBarrier)
-// by having a relaxed store to the same address, with the same value. This is
-// done in order for TSAN to see these stores from generated code.
-// TODO(solanes): Also do this for non-kArchStoreWithWriteBarrier stores.
-static void tsan_relaxed_store(Address addr, int64_t value) {
+// Mimics the store in generated code by having a relaxed store to the same
+// address, with the same value. This is done in order for TSAN to see these
+// stores from generated code.
+// Note that {value} is an int64_t irrespective of the store size. This is on
+// purpose to keep the function signatures the same accross stores. The
+// static_cast inside the method will ignore the bits which will not be stored.
+static void tsan_relaxed_store_32_bits(Address addr, int64_t value) {
 #if V8_TARGET_ARCH_X64
-  if (COMPRESS_POINTERS_BOOL) {
     base::Relaxed_Store(reinterpret_cast<base::Atomic32*>(addr),
                         static_cast<base::Atomic32>(value));
-  } else {
-    base::Relaxed_Store(reinterpret_cast<base::Atomic64*>(addr),
-                        static_cast<base::Atomic64>(value));
-  }
 #else
   UNREACHABLE();
 #endif  // V8_TARGET_ARCH_X64
 }
+
+static void tsan_relaxed_store_64_bits(Address addr, int64_t value) {
+#if V8_TARGET_ARCH_X64
+  base::Relaxed_Store(reinterpret_cast<base::Atomic64*>(addr),
+                      static_cast<base::Atomic64>(value));
+#else
+  UNREACHABLE();
+#endif  // V8_TARGET_ARCH_X64
+}
+
 #endif  // V8_IS_TSAN
 
-IF_TSAN(FUNCTION_REFERENCE, tsan_relaxed_store_function, tsan_relaxed_store)
+IF_TSAN(FUNCTION_REFERENCE, tsan_relaxed_store_function_32_bits,
+        tsan_relaxed_store_32_bits)
+IF_TSAN(FUNCTION_REFERENCE, tsan_relaxed_store_function_64_bits,
+        tsan_relaxed_store_64_bits)
 
 static int EnterMicrotaskContextWrapper(HandleScopeImplementer* hsi,
                                         Address raw_context) {

src/codegen/external-reference.h

@@ -266,7 +266,10 @@ class StatsCounter;
   V(atomic_pair_exchange_function, "atomic_pair_exchange_function")            \
   V(atomic_pair_compare_exchange_function,                                     \
     "atomic_pair_compare_exchange_function")                                   \
-  IF_TSAN(V, tsan_relaxed_store_function, "tsan_relaxed_store_function")       \
+  IF_TSAN(V, tsan_relaxed_store_function_32_bits,                              \
+          "tsan_relaxed_store_function_32_bits")                               \
+  IF_TSAN(V, tsan_relaxed_store_function_64_bits,                              \
+          "tsan_relaxed_store_function_64_bits")                               \
   V(js_finalization_registry_remove_cell_from_unregister_token_map,            \
     "JSFinalizationRegistry::RemoveCellFromUnregisterTokenMap")                \
   V(re_match_for_call_from_js, "IrregexpInterpreter::MatchForCallFromJs")      \

src/codegen/x64/macro-assembler-x64.cc

@@ -480,7 +480,7 @@ void TurboAssembler::CallRecordWriteStub(
 
 #ifdef V8_IS_TSAN
 void TurboAssembler::CallTSANRelaxedStoreStub(Register address, Register value,
-                                              SaveFPRegsMode fp_mode,
+                                              SaveFPRegsMode fp_mode, int size,
                                               StubCallMode mode) {
   DCHECK(!AreAliased(address, value));
   TSANRelaxedStoreDescriptor descriptor;
@@ -500,10 +500,10 @@ void TurboAssembler::CallTSANRelaxedStoreStub(Register address, Register value,
 
   if (mode == StubCallMode::kCallWasmRuntimeStub) {
     // Use {near_call} for direct Wasm call within a module.
-    auto wasm_target = wasm::WasmCode::GetTSANRelaxedStoreStub(fp_mode);
+    auto wasm_target = wasm::WasmCode::GetTSANRelaxedStoreStub(fp_mode, size);
     near_call(wasm_target, RelocInfo::WASM_STUB_CALL);
   } else {
-    auto builtin_index = Builtins::GetTSANRelaxedStoreStub(fp_mode);
+    auto builtin_index = Builtins::GetTSANRelaxedStoreStub(fp_mode, size);
     Handle<Code> code_target =
         isolate()->builtins()->builtin_handle(builtin_index);
     Call(code_target, RelocInfo::CODE_TARGET);

src/codegen/x64/macro-assembler-x64.h

@@ -517,7 +517,7 @@ class V8_EXPORT_PRIVATE TurboAssembler : public SharedTurboAssembler {
 
 #ifdef V8_IS_TSAN
   void CallTSANRelaxedStoreStub(
-      Register address, Register value, SaveFPRegsMode fp_mode,
+      Register address, Register value, SaveFPRegsMode fp_mode, int size,
       StubCallMode mode = StubCallMode::kCallBuiltinPointer);
 #endif  // V8_IS_TSAN
 

src/compiler/backend/x64/code-generator-x64.cc

@@ -12,6 +12,7 @@
 #include "src/codegen/optimized-compilation-info.h"
 #include "src/codegen/x64/assembler-x64.h"
 #include "src/codegen/x64/register-x64.h"
+#include "src/common/globals.h"
 #include "src/compiler/backend/code-generator-impl.h"
 #include "src/compiler/backend/code-generator.h"
 #include "src/compiler/backend/gap-resolver.h"
@@ -327,7 +328,7 @@ class OutOfLineRecordWrite final : public OutOfLineCode {
 class OutOfLineTSANRelaxedStore final : public OutOfLineCode {
  public:
   OutOfLineTSANRelaxedStore(CodeGenerator* gen, Operand operand, Register value,
-                            Register scratch0, StubCallMode stub_mode)
+                            Register scratch0, StubCallMode stub_mode, int size)
       : OutOfLineCode(gen),
         operand_(operand),
         value_(value),
@@ -335,6 +336,7 @@ class OutOfLineTSANRelaxedStore final : public OutOfLineCode {
 #if V8_ENABLE_WEBASSEMBLY
         stub_mode_(stub_mode),
 #endif  // V8_ENABLE_WEBASSEMBLY
+        size_(size),
         zone_(gen->zone()) {
     DCHECK(!AreAliased(value, scratch0));
   }
@@ -350,13 +352,13 @@ class OutOfLineTSANRelaxedStore final : public OutOfLineCode {
       // A direct call to a wasm runtime stub defined in this module.
       // Just encode the stub index. This will be patched when the code
       // is added to the native module and copied into wasm code space.
-      __ CallTSANRelaxedStoreStub(scratch0_, value_, save_fp_mode,
+      __ CallTSANRelaxedStoreStub(scratch0_, value_, save_fp_mode, size_,
                                   StubCallMode::kCallWasmRuntimeStub);
       return;
     }
 #endif  // V8_ENABLE_WEBASSEMBLY
 
-    __ CallTSANRelaxedStoreStub(scratch0_, value_, save_fp_mode);
+    __ CallTSANRelaxedStoreStub(scratch0_, value_, save_fp_mode, size_);
   }
 
  private:
@@ -366,8 +368,41 @@ class OutOfLineTSANRelaxedStore final : public OutOfLineCode {
 #if V8_ENABLE_WEBASSEMBLY
   StubCallMode const stub_mode_;
 #endif  // V8_ENABLE_WEBASSEMBLY
+  int size_;
   Zone* zone_;
 };
+
+void EmitTSANStoreOOLIfNeeded(Zone* zone, CodeGenerator* codegen,
+                              TurboAssembler* tasm, Operand operand,
+                              Register value_reg, X64OperandConverter& i,
+                              StubCallMode mode, int size) {
+  Register scratch0 = i.TempRegister(0);
+  auto tsan_ool = zone->New<OutOfLineTSANRelaxedStore>(
+      codegen, operand, value_reg, scratch0, mode, size);
+  tasm->jmp(tsan_ool->entry());
+  tasm->bind(tsan_ool->exit());
+}
+
+void EmitTSANStoreOOLIfNeeded(Zone* zone, CodeGenerator* codegen,
+                              TurboAssembler* tasm, Operand operand,
+                              Immediate value, X64OperandConverter& i,
+                              StubCallMode mode, int size) {
+  Register value_reg = i.TempRegister(1);
+  tasm->movq(value_reg, value);
+  EmitTSANStoreOOLIfNeeded(zone, codegen, tasm, operand, value_reg, i, mode,
+                           size);
+}
+
+#else
+void EmitTSANStoreOOLIfNeeded(Zone* zone, CodeGenerator* codegen,
+                              TurboAssembler* tasm, Operand operand,
+                              Register value_reg, X64OperandConverter& i,
+                              StubCallMode mode, int size) {}
+
+void EmitTSANStoreOOLIfNeeded(Zone* zone, CodeGenerator* codegen,
+                              TurboAssembler* tasm, Operand operand,
+                              Immediate value, X64OperandConverter& i,
+                              StubCallMode mode, int size) {}
 #endif  // V8_IS_TSAN
 
 #if V8_ENABLE_WEBASSEMBLY
@@ -1236,13 +1271,8 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
       Register value = i.InputRegister(index);
       Register scratch0 = i.TempRegister(0);
 
-#ifdef V8_IS_TSAN
-      auto tsan_ool = zone()->New<OutOfLineTSANRelaxedStore>(
-          this, operand, value, scratch0, DetermineStubCallMode());
-      __ jmp(tsan_ool->entry());
-      __ bind(tsan_ool->exit());
-#endif  // V8_IS_TSAN
-
+      EmitTSANStoreOOLIfNeeded(zone(), this, tasm(), operand, value, i,
+                               DetermineStubCallMode(), kTaggedSize);
       Register scratch1 = i.TempRegister(1);
       auto ool = zone()->New<OutOfLineRecordWrite>(this, object, operand, value,
                                                    scratch0, scratch1, mode,
@@ -2187,25 +2217,13 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
       Operand operand = i.MemoryOperand(&index);
       if (HasImmediateInput(instr, index)) {
         Immediate value = i.InputImmediate(index);
-#ifdef V8_IS_TSAN
-        Register value_reg = i.TempRegister(1);
-        __ movq(value_reg, value);
-        Register scratch0 = i.TempRegister(0);
-        auto tsan_ool = zone()->New<OutOfLineTSANRelaxedStore>(
-            this, operand, value_reg, scratch0, DetermineStubCallMode());
-        __ jmp(tsan_ool->entry());
-        __ bind(tsan_ool->exit());
-#endif  // V8_IS_TSAN
+        EmitTSANStoreOOLIfNeeded(zone(), this, tasm(), operand, value, i,
+                                 DetermineStubCallMode(), kTaggedSize);
         __ StoreTaggedField(operand, value);
       } else {
         Register value = i.InputRegister(index);
-#ifdef V8_IS_TSAN
-        Register scratch0 = i.TempRegister(0);
-        auto tsan_ool = zone()->New<OutOfLineTSANRelaxedStore>(
-            this, operand, value, scratch0, DetermineStubCallMode());
-        __ jmp(tsan_ool->entry());
-        __ bind(tsan_ool->exit());
-#endif  // V8_IS_TSAN
+        EmitTSANStoreOOLIfNeeded(zone(), this, tasm(), operand, value, i,
+                                 DetermineStubCallMode(), kTaggedSize);
         __ StoreTaggedField(operand, value);
       }
       break;

src/compiler/backend/x64/instruction-selector-x64.cc

@@ -295,10 +295,10 @@ ArchOpcode GetLoadOpcode(LoadRepresentation load_rep) {
     case MachineRepresentation::kWord64:
       opcode = kX64Movq;
       break;
-    case MachineRepresentation::kSimd128:  // Fall through.
+    case MachineRepresentation::kSimd128:
       opcode = kX64Movdqu;
       break;
-    case MachineRepresentation::kNone:
+    case MachineRepresentation::kNone:  // Fall through.
     case MachineRepresentation::kMapWord:
       UNREACHABLE();
   }
@@ -331,9 +331,9 @@ ArchOpcode GetStoreOpcode(StoreRepresentation store_rep) {
       return kX64MovqCompressTagged;
     case MachineRepresentation::kWord64:
       return kX64Movq;
-    case MachineRepresentation::kSimd128:  // Fall through.
+    case MachineRepresentation::kSimd128:
       return kX64Movdqu;
-    case MachineRepresentation::kNone:
+    case MachineRepresentation::kNone:  // Fall through.
     case MachineRepresentation::kMapWord:
       UNREACHABLE();
   }

src/debug/debug-evaluate.cc

@@ -1055,8 +1055,10 @@ static bool TransitivelyCalledBuiltinHasNoSideEffect(Builtins::Name caller,
     case Builtins::kToObject:
     case Builtins::kToString:
 #ifdef V8_IS_TSAN
-    case Builtins::kTSANRelaxedStoreIgnoreFP:
-    case Builtins::kTSANRelaxedStoreSaveFP:
+    case Builtins::kTSANRelaxedStore32IgnoreFP:
+    case Builtins::kTSANRelaxedStore32SaveFP:
+    case Builtins::kTSANRelaxedStore64IgnoreFP:
+    case Builtins::kTSANRelaxedStore64SaveFP:
 #endif  // V8_IS_TSAN
     case Builtins::kWeakMapLookupHashIndex:
       return true;

src/wasm/wasm-code-manager.h

@@ -95,8 +95,10 @@ struct WasmModule;
   V(RecordWriteEmitRememberedSetIgnoreFP) \
   V(RecordWriteOmitRememberedSetIgnoreFP) \
   V(ToNumber)                             \
-  IF_TSAN(V, TSANRelaxedStoreIgnoreFP)    \
-  IF_TSAN(V, TSANRelaxedStoreSaveFP)      \
+  IF_TSAN(V, TSANRelaxedStore32IgnoreFP)  \
+  IF_TSAN(V, TSANRelaxedStore32SaveFP)    \
+  IF_TSAN(V, TSANRelaxedStore64IgnoreFP)  \
+  IF_TSAN(V, TSANRelaxedStore64SaveFP)    \
   V(WasmAllocateArrayWithRtt)             \
   V(WasmArrayCopy)                        \
   V(WasmArrayCopyWithChecks)              \
@@ -177,10 +179,18 @@ class V8_EXPORT_PRIVATE WasmCode final {
   }
 
 #ifdef V8_IS_TSAN
-  static RuntimeStubId GetTSANRelaxedStoreStub(SaveFPRegsMode fp_mode) {
-    return fp_mode == SaveFPRegsMode::kIgnore
-               ? RuntimeStubId::kTSANRelaxedStoreIgnoreFP
-               : RuntimeStubId::kTSANRelaxedStoreSaveFP;
+  static RuntimeStubId GetTSANRelaxedStoreStub(SaveFPRegsMode fp_mode,
+                                               int size) {
+    if (size == kInt32Size) {
+      return fp_mode == SaveFPRegsMode::kIgnore
+                 ? RuntimeStubId::kTSANRelaxedStore32IgnoreFP
+                 : RuntimeStubId::kTSANRelaxedStore32SaveFP;
+    } else {
+      CHECK_EQ(size, kInt64Size);
+      return fp_mode == SaveFPRegsMode::kIgnore
+                 ? RuntimeStubId::kTSANRelaxedStore64IgnoreFP
+                 : RuntimeStubId::kTSANRelaxedStore64SaveFP;
+    }
   }
 #endif  // V8_IS_TSAN