Revert "Revert "[cctest] Clarify that tests for sync instructions are simulator specific""

This reverts commit 1feadfe8.

Reason for revert: Reland as bot stayed red after revert.

Original change's description:
> Revert "[cctest] Clarify that tests for sync instructions are simulator specific"
> 
> This reverts commit 4013518f.
> 
> Reason for revert:
> https://build.chromium.org/p/client.v8.ports/builders/V8%20Linux%20-%20arm64%20-%20sim%20-%20gc%20stress
> 
> Original change's description:
> > [cctest] Clarify that tests for sync instructions are simulator specific
> > 
> > Some tests were recently added to test-simulator-arm.cc, however this file is
> > meant for tests that are specific to the simulator and therefore are not written
> > to work on hardware. While this sounds surprising, the reason is that our simulation
> > of synchronisation instructions is more conservative than on hardware.
> > 
> > To make this more clear, this patch renames the "test-simulator-arm{,64}.cc"
> > files to "test-sync-primitives-arm{,64}.cc", and moves the vneg and vabs tests
> > into "test-assembler-arm.cc" which is were tests that are garanteed to work in
> > either native or simulated environments live.
> > 
> > Finally, take the opportunity to share a little bit of code.
> > 
> > Bug: v8:6963
> > Change-Id: Ifb85d3671c823b9bba73d09f419536b089a4e87c
> > Reviewed-on: https://chromium-review.googlesource.com/749387
> > Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
> > Commit-Queue: Pierre Langlois <pierre.langlois@arm.com>
> > Cr-Commit-Position: refs/heads/master@{#49073}
> 
> TBR=clemensh@chromium.org,pierre.langlois@arm.com,bmeurer@chromium.org
> 
> Change-Id: I1bfb4e9c7c18b716f417a84b18a14cb2e1fa3a7a
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: v8:6963
> Reviewed-on: https://chromium-review.googlesource.com/750624
> Reviewed-by: Michael Achenbach <machenbach@chromium.org>
> Commit-Queue: Michael Achenbach <machenbach@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49074}

TBR=machenbach@chromium.org,clemensh@chromium.org,pierre.langlois@arm.com,bmeurer@chromium.org

Change-Id: I5af7bd3678758130534730a2f6f0b651b64c6956
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:6963
Reviewed-on: https://chromium-review.googlesource.com/750903Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49075}

test/cctest/BUILD.gn

@@ -249,6 +249,8 @@ v8_source_set("cctest_sources") {
 
   if (v8_current_cpu == "arm") {
     sources += [  ### gcmole(arch:arm) ###
+      "assembler-helper-arm.cc",
+      "assembler-helper-arm.h",
       "test-assembler-arm.cc",
       "test-code-stubs-arm.cc",
       "test-code-stubs.cc",
@@ -256,7 +258,7 @@ v8_source_set("cctest_sources") {
       "test-disasm-arm.cc",
       "test-macro-assembler-arm.cc",
       "test-run-wasm-relocation-arm.cc",
-      "test-simulator-arm.cc",
+      "test-sync-primitives-arm.cc",
     ]
   } else if (v8_current_cpu == "arm64") {
     sources += [  ### gcmole(arch:arm64) ###
@@ -269,7 +271,7 @@ v8_source_set("cctest_sources") {
       "test-javascript-arm64.cc",
       "test-js-arm64-variables.cc",
       "test-run-wasm-relocation-arm64.cc",
-      "test-simulator-arm64.cc",
+      "test-sync-primitives-arm64.cc",
       "test-utils-arm64.cc",
       "test-utils-arm64.h",
     ]

test/cctest/assembler-helper-arm.cc

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "test/cctest/assembler-helper-arm.h"
+
+#include "src/v8.h"
+#include "test/cctest/cctest.h"
+
+#include "src/isolate-inl.h"
+
+namespace v8 {
+namespace internal {
+
+Address AssembleCode(std::function<void(Assembler&)> assemble) {
+  Isolate* isolate = CcTest::i_isolate();
+  Assembler assm(isolate, nullptr, 0);
+
+  assemble(assm);
+  assm.bx(lr);
+
+  CodeDesc desc;
+  assm.GetCode(isolate, &desc);
+  Handle<Code> code =
+      isolate->factory()->NewCode(desc, Code::STUB, Handle<Code>());
+  if (FLAG_print_code) {
+    code->Print();
+  }
+  return code->entry();
+}
+
+}  // namespace internal
+}  // namespace v8

test/cctest/assembler-helper-arm.h

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_CCTEST_ASSEMBLER_HELPER_ARM_H_
+#define V8_CCTEST_ASSEMBLER_HELPER_ARM_H_
+
+#include <functional>
+
+#include "src/macro-assembler.h"
+
+namespace v8 {
+namespace internal {
+
+// These function prototypes have 5 arguments since they are used with the
+// CALL_GENERATED_CODE macro.
+typedef Object* (*F_iiiii)(int x, int p1, int p2, int p3, int p4);
+typedef Object* (*F_piiii)(void* p0, int p1, int p2, int p3, int p4);
+typedef Object* (*F_ppiii)(void* p0, void* p1, int p2, int p3, int p4);
+typedef Object* (*F_pppii)(void* p0, void* p1, void* p2, int p3, int p4);
+typedef Object* (*F_ippii)(int p0, void* p1, void* p2, int p3, int p4);
+
+Address AssembleCode(std::function<void(Assembler&)> assemble);
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_CCTEST_ASSEMBLER_HELPER_ARM_H_

test/cctest/cctest.gyp

@@ -244,6 +244,8 @@
       'test-run-wasm-relocation-x64.cc',
     ],
     'cctest_sources_arm': [  ### gcmole(arch:arm) ###
+      'assembler-helper-arm.cc',
+      'assembler-helper-arm.h',
       'test-assembler-arm.cc',
       'test-code-stubs.cc',
       'test-code-stubs.h',
@@ -251,7 +253,7 @@
       'test-disasm-arm.cc',
       'test-macro-assembler-arm.cc',
       'test-run-wasm-relocation-arm.cc',
-      'test-simulator-arm.cc',
+      'test-sync-primitives-arm.cc',
     ],
     'cctest_sources_arm64': [  ### gcmole(arch:arm64) ###
       'test-utils-arm64.cc',
@@ -265,7 +267,7 @@
       'test-javascript-arm64.cc',
       'test-js-arm64-variables.cc',
       'test-run-wasm-relocation-arm64.cc',
-      'test-simulator-arm64.cc',
+      'test-sync-primitives-arm64.cc',
     ],
     'cctest_sources_s390': [  ### gcmole(arch:s390) ###
       'test-assembler-s390.cc',

test/cctest/test-simulator-arm.cc → test/cctest/test-sync-primitives-arm.cc

@@ -26,8 +26,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "src/v8.h"
+#include "test/cctest/assembler-helper-arm.h"
 #include "test/cctest/cctest.h"
-#include "test/cctest/compiler/value-helper.h"
 
 #include "src/arm/simulator-arm.h"
 #include "src/assembler-inl.h"
@@ -38,14 +38,25 @@
 namespace v8 {
 namespace internal {
 
+// These tests rely on the behaviour specific to the simulator so we cannot
+// expect the same results on real hardware. The reason for this is that our
+// simulation of synchronisation primitives is more conservative than the
+// reality.
+// For example:
+//   ldrex r1, [r2] ; Load acquire at address r2; r2 is now marked as exclusive.
+//   ldr r0, [r4]   ; This is a normal load, and at a different address.
+//                  ; However, any memory accesses can potentially clear the
+//                  ; exclusivity (See ARM DDI 0406C.c A3.4.5). This is unlikely
+//                  ; on real hardware but to be conservative, the simulator
+//                  ; always does it.
+//   strex r3, r1, [r2] ; As a result, this will always fail in the simulator
+//                      ; but will likely succeed on hardware.
+#if defined(USE_SIMULATOR)
+
 #ifndef V8_TARGET_LITTLE_ENDIAN
 #error Expected ARM to be little-endian
 #endif
 
-// Define these function prototypes to match JSEntryFunction in execution.cc.
-typedef Object* (*F_iiiii)(int p0, int p1, int p2, int p3, int p4);
-typedef Object* (*F_piiii)(void* p0, int p1, int p2, int p3, int p4);
-
 #define __ assm.
 
 namespace {
@@ -168,22 +179,6 @@ void AssembleMemoryAccess(Assembler* assembler, MemoryAccess access,
   }
 }
 
-Address AssembleCode(std::function<void(Assembler&)> assemble) {
-  Isolate* isolate = CcTest::i_isolate();
-  Assembler assm(isolate, nullptr, 0);
-
-  assemble(assm);
-
-  __ bx(lr);
-
-  CodeDesc desc;
-  assm.GetCode(isolate, &desc);
-  Handle<Code> code =
-      isolate->factory()->NewCode(desc, Code::STUB, Handle<Code>());
-  return code->entry();
-}
-
-#if defined(USE_SIMULATOR)
 void AssembleLoadExcl(Assembler* assembler, MemoryAccess access,
                       Register value_reg, Register addr_reg) {
   DCHECK(access.kind == MemoryAccess::Kind::LoadExcl);
@@ -228,32 +223,9 @@ void TestInvalidateExclusiveAccess(TestData initial_data, MemoryAccess access1,
       break;
   }
 }
-#endif
-
-std::vector<Float32> Float32Inputs() {
-  std::vector<Float32> inputs;
-  FOR_FLOAT32_INPUTS(f) {
-    inputs.push_back(Float32::FromBits(bit_cast<uint32_t>(*f)));
-  }
-  FOR_UINT32_INPUTS(bits) { inputs.push_back(Float32::FromBits(*bits)); }
-  return inputs;
-}
-
-std::vector<Float64> Float64Inputs() {
-  std::vector<Float64> inputs;
-  FOR_FLOAT64_INPUTS(f) {
-    inputs.push_back(Float64::FromBits(bit_cast<uint64_t>(*f)));
-  }
-  FOR_UINT64_INPUTS(bits) { inputs.push_back(Float64::FromBits(*bits)); }
-  return inputs;
-}
 
 }  // namespace
 
-// TODO(rodolph.perfetta@arm.com): Enable this test for native hardware, see
-// http://crbug.com/v8/6963.
-#if defined(USE_SIMULATOR)
-
 TEST(simulator_invalidate_exclusive_access) {
   using Kind = MemoryAccess::Kind;
   using Size = MemoryAccess::Size;
@@ -290,10 +262,10 @@ TEST(simulator_invalidate_exclusive_access) {
                                 0, TestData(7));
 }
 
-#endif  // USE_SIMULATOR
+namespace {
 
-static int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
-                               MemoryAccess access) {
+int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
+                        MemoryAccess access) {
   HandleScope scope(isolate);
   F_piiii f = FUNCTION_CAST<F_piiii>(AssembleCode([&](Assembler& assm) {
     AssembleMemoryAccess(&assm, access, r0, r2, r1);
@@ -303,6 +275,8 @@ static int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
       CALL_GENERATED_CODE(isolate, f, test_data, 0, 0, 0, 0));
 }
 
+}  // namespace
+
 class MemoryAccessThread : public v8::base::Thread {
  public:
   MemoryAccessThread()
@@ -370,10 +344,6 @@ class MemoryAccessThread : public v8::base::Thread {
   v8::Isolate* isolate_;
 };
 
-// TODO(rodolph.perfetta@arm.com): Enable this test for native hardware, see
-// http://crbug.com/v8/6963.
-#if defined(USE_SIMULATOR)
-
 TEST(simulator_invalidate_exclusive_access_threaded) {
   using Kind = MemoryAccess::Kind;
   using Size = MemoryAccess::Size;
@@ -422,87 +392,9 @@ TEST(simulator_invalidate_exclusive_access_threaded) {
   thread.Join();
 }
 
-#endif  // USE_SIMULATOR
-
-TEST(simulator_vabs_32) {
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope scope(isolate);
-
-  F_iiiii f = FUNCTION_CAST<F_iiiii>(AssembleCode([](Assembler& assm) {
-    __ vmov(s0, r0);
-    __ vabs(s0, s0);
-    __ vmov(r0, s0);
-  }));
-
-  for (Float32 f32 : Float32Inputs()) {
-    Float32 res = Float32::FromBits(reinterpret_cast<uint32_t>(
-        CALL_GENERATED_CODE(isolate, f, f32.get_bits(), 0, 0, 0, 0)));
-    Float32 exp = Float32::FromBits(f32.get_bits() & ~(1 << 31));
-    CHECK_EQ(exp.get_bits(), res.get_bits());
-  }
-}
-
-TEST(simulator_vabs_64) {
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope scope(isolate);
-
-  F_iiiii f = FUNCTION_CAST<F_iiiii>(AssembleCode([](Assembler& assm) {
-    __ vmov(d0, r0, r1);
-    __ vabs(d0, d0);
-    __ vmov(r1, r0, d0);
-  }));
-
-  for (Float64 f64 : Float64Inputs()) {
-    uint32_t p0 = static_cast<uint32_t>(f64.get_bits());
-    uint32_t p1 = static_cast<uint32_t>(f64.get_bits() >> 32);
-    uint32_t res = reinterpret_cast<uint32_t>(
-        CALL_GENERATED_CODE(isolate, f, p0, p1, 0, 0, 0));
-    Float64 exp = Float64::FromBits(f64.get_bits() & ~(1ull << 63));
-    // We just get back the top word, so only compare that one.
-    CHECK_EQ(exp.get_bits() >> 32, res);
-  }
-}
-
-TEST(simulator_vneg_32) {
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope scope(isolate);
-
-  F_iiiii f = FUNCTION_CAST<F_iiiii>(AssembleCode([](Assembler& assm) {
-    __ vmov(s0, r0);
-    __ vneg(s0, s0);
-    __ vmov(r0, s0);
-  }));
-
-  for (Float32 f32 : Float32Inputs()) {
-    Float32 res = Float32::FromBits(reinterpret_cast<uint32_t>(
-        CALL_GENERATED_CODE(isolate, f, f32.get_bits(), 0, 0, 0, 0)));
-    Float32 exp = Float32::FromBits(f32.get_bits() ^ (1 << 31));
-    CHECK_EQ(exp.get_bits(), res.get_bits());
-  }
-}
-
-TEST(simulator_vneg_64) {
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope scope(isolate);
-
-  F_iiiii f = FUNCTION_CAST<F_iiiii>(AssembleCode([](Assembler& assm) {
-    __ vmov(d0, r0, r1);
-    __ vneg(d0, d0);
-    __ vmov(r1, r0, d0);
-  }));
-
-  for (Float64 f64 : Float64Inputs()) {
-    uint32_t p0 = static_cast<uint32_t>(f64.get_bits());
-    uint32_t p1 = static_cast<uint32_t>(f64.get_bits() >> 32);
-    uint32_t res = reinterpret_cast<uint32_t>(
-        CALL_GENERATED_CODE(isolate, f, p0, p1, 0, 0, 0));
-    Float64 exp = Float64::FromBits(f64.get_bits() ^ (1ull << 63));
-    // We just get back the top word, so only compare that one.
-    CHECK_EQ(exp.get_bits() >> 32, res);
-  }
-}
-
 #undef __
 
+#endif  // defined(USE_SIMULATOR)
+
 }  // namespace internal
 }  // namespace v8

test/cctest/test-simulator-arm64.cc → test/cctest/test-sync-primitives-arm64.cc

@@ -36,6 +36,19 @@
 namespace v8 {
 namespace internal {
 
+// These tests rely on the behaviour specific to the simulator so we cannot
+// expect the same results on real hardware. The reason for this is that our
+// simulation of synchronisation primitives is more conservative than the
+// reality.
+// For example:
+//   ldxr x1, [x2] ; Load acquire at address x2; x2 is now marked as exclusive.
+//   ldr x0, [x4]  ; This is a normal load, and at a different address.
+//                 ; However, any memory accesses can potentially clear the
+//                 ; exclusivity (See ARM DDI 0487B.a B2.9.5). This is unlikely
+//                 ; on real hardware but to be conservative, the simulator
+//                 ; always does it.
+//   stxr w3, x1, [x2] ; As a result, this will always fail in the simulator but
+//                     ; will likely succeed on hardware.
 #if defined(USE_SIMULATOR)
 
 #ifndef V8_TARGET_LITTLE_ENDIAN
@@ -80,9 +93,11 @@ struct TestData {
   int dummy;
 };
 
-static void AssembleMemoryAccess(MacroAssembler* assembler, MemoryAccess access,
-                                 Register dest_reg, Register value_reg,
-                                 Register addr_reg) {
+namespace {
+
+void AssembleMemoryAccess(MacroAssembler* assembler, MemoryAccess access,
+                          Register dest_reg, Register value_reg,
+                          Register addr_reg) {
   MacroAssembler& masm = *assembler;
   __ Add(addr_reg, x0, Operand(access.offset));
 
@@ -162,22 +177,22 @@ static void AssembleMemoryAccess(MacroAssembler* assembler, MemoryAccess access,
   }
 }
 
-static void AssembleLoadExcl(MacroAssembler* assembler, MemoryAccess access,
-                             Register value_reg, Register addr_reg) {
+void AssembleLoadExcl(MacroAssembler* assembler, MemoryAccess access,
+                      Register value_reg, Register addr_reg) {
   DCHECK(access.kind == MemoryAccess::Kind::LoadExcl);
   AssembleMemoryAccess(assembler, access, no_reg, value_reg, addr_reg);
 }
 
-static void AssembleStoreExcl(MacroAssembler* assembler, MemoryAccess access,
-                              Register dest_reg, Register value_reg,
-                              Register addr_reg) {
+void AssembleStoreExcl(MacroAssembler* assembler, MemoryAccess access,
+                       Register dest_reg, Register value_reg,
+                       Register addr_reg) {
   DCHECK(access.kind == MemoryAccess::Kind::StoreExcl);
   AssembleMemoryAccess(assembler, access, dest_reg, value_reg, addr_reg);
 }
 
-static void TestInvalidateExclusiveAccess(
-    TestData initial_data, MemoryAccess access1, MemoryAccess access2,
-    MemoryAccess access3, int expected_res, TestData expected_data) {
+void TestInvalidateExclusiveAccess(TestData initial_data, MemoryAccess access1,
+                                   MemoryAccess access2, MemoryAccess access3,
+                                   int expected_res, TestData expected_data) {
   Isolate* isolate = CcTest::i_isolate();
   HandleScope scope(isolate);
   MacroAssembler masm(isolate, nullptr, 0,
@@ -192,6 +207,7 @@ static void TestInvalidateExclusiveAccess(
   masm.GetCode(isolate, &desc);
   Handle<Code> code =
       isolate->factory()->NewCode(desc, Code::STUB, Handle<Code>());
+
   TestData t = initial_data;
   Simulator::CallArgument args[] = {
       Simulator::CallArgument(reinterpret_cast<uintptr_t>(&t)),
@@ -215,6 +231,8 @@ static void TestInvalidateExclusiveAccess(
   }
 }
 
+}  // namespace
+
 TEST(simulator_invalidate_exclusive_access) {
   using Kind = MemoryAccess::Kind;
   using Size = MemoryAccess::Size;
@@ -251,8 +269,10 @@ TEST(simulator_invalidate_exclusive_access) {
                                 0, TestData(7));
 }
 
-static int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
-                               MemoryAccess access) {
+namespace {
+
+int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
+                        MemoryAccess access) {
   HandleScope scope(isolate);
   MacroAssembler masm(isolate, nullptr, 0,
                       v8::internal::CodeObjectRequired::kYes);
@@ -270,6 +290,8 @@ static int ExecuteMemoryAccess(Isolate* isolate, TestData* test_data,
   return Simulator::current(isolate)->wreg(0);
 }
 
+}  // namespace
+
 class MemoryAccessThread : public v8::base::Thread {
  public:
   MemoryAccessThread()