[wasm-simd] Fix init of SIMD global

Using uint8_t[] causes decay to pointer issue, which manifests in
copying garbage values in the call to WriteLittleEndianValue. Change it
to use a std::array, which doesn't have the decaying behavior.

Also add a regression test from comment#6 of the linked bug.

Bug: v8:10731
Change-Id: I4a1ca69fe99806642e9931625ca7aeab6663f955
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2316465Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69052}

src/base/memory.h

@@ -76,6 +76,9 @@ static inline V ReadLittleEndianValue(V* p) {
 
 template <typename V>
 static inline void WriteLittleEndianValue(V* p, V value) {
+  static_assert(
+      !std::is_array<V>::value,
+      "Passing an array decays to pointer, causing unexpected results.");
   WriteLittleEndianValue<V>(reinterpret_cast<Address>(p), value);
 }
 

src/wasm/module-instantiate.cc

@@ -1537,8 +1537,8 @@ void InstanceBuilder::InitGlobals(Handle<WasmInstanceObject> instance) {
         break;
       case WasmInitExpr::kS128Const:
         DCHECK(enabled_.has_simd());
-        WriteLittleEndianValue<uint8_t[kSimd128Size]>(
-            GetRawGlobalPtr<uint8_t[kSimd128Size]>(global),
+        WriteLittleEndianValue<std::array<uint8_t, kSimd128Size>>(
+            GetRawGlobalPtr<std::array<uint8_t, kSimd128Size>>(global),
             global.init.immediate().s128_const);
         break;
       case WasmInitExpr::kRefNullConst:

src/wasm/wasm-module-builder.cc

@@ -444,7 +444,7 @@ void WriteGlobalInitializer(ZoneBuffer* buffer, const WasmInitExpr& init,
     case WasmInitExpr::kS128Const:
       buffer->write_u8(kSimdPrefix);
       buffer->write_u8(kExprS128Const & 0xFF);
-      buffer->write(init.immediate().s128_const, kSimd128Size);
+      buffer->write(init.immediate().s128_const.data(), kSimd128Size);
       break;
     case WasmInitExpr::kGlobalGet:
       buffer->write_u8(kExprGlobalGet);

src/wasm/wasm-opcodes.h

@@ -773,7 +773,7 @@ class WasmInitExpr {
     int64_t i64_const;
     float f32_const;
     double f64_const;
-    uint8_t s128_const[kSimd128Size];
+    std::array<uint8_t, kSimd128Size> s128_const;
     uint32_t index;
     HeapType::Representation heap_type;
   };
@@ -792,7 +792,7 @@ class WasmInitExpr {
     immediate_.f64_const = v;
   }
   explicit WasmInitExpr(uint8_t v[kSimd128Size]) : kind_(kS128Const) {
-    memcpy(immediate_.s128_const, v, kSimd128Size);
+    memcpy(immediate_.s128_const.data(), v, kSimd128Size);
   }
 
   MOVE_ONLY_NO_DEFAULT_CONSTRUCTOR(WasmInitExpr);
@@ -855,8 +855,7 @@ class WasmInitExpr {
       case kF64Const:
         return immediate().f64_const == other.immediate().f64_const;
       case kS128Const:
-        return 0 == memcmp(immediate().s128_const, other.immediate().s128_const,
-                           kSimd128Size);
+        return immediate().s128_const == other.immediate().s128_const;
       case kRefNullConst:
       case kRttCanon:
         return immediate().heap_type == other.immediate().heap_type;

test/mjsunit/wasm/simd-globals.js

+// Copyright 2020 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.
+
+// Flags: --experimental-wasm-simd
+
+load("test/mjsunit/wasm/wasm-module-builder.js");
+
+// Test for S128 global with initialization.
+// This checks for a bug in copying the immediate values from the
+// initialization expression into the globals area of the module.
+(function TestS128() {
+  var builder = new WasmModuleBuilder();
+  var g = builder.addGlobal(kWasmS128);
+  g.init = [1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0, 4, 0, 0, 0];
+
+  // Check that all lanes have the right values by creating 4 functions that
+  // extract each lane.
+  function addGetFunction(i) {
+    builder.addFunction(`get${i}`, kSig_i_v)
+      .addBody([
+        kExprGlobalGet, g.index,
+        kSimdPrefix, kExprI32x4ExtractLane, i])
+      .exportAs(`get${i}`);
+  }
+
+  for (let i = 0; i < 4; i++) {
+    addGetFunction(i);
+  }
+
+  var instance = builder.instantiate();
+
+  for (let i = 0; i < 4; i++) {
+    // get0 will get lane0, which has value 1
+    assertEquals(i+1, instance.exports[`get${i}`]());
+  }
+})();

test/mjsunit/wasm/wasm-module-builder.js

@@ -470,12 +470,13 @@ let kExprI64AtomicCompareExchange32U = 0x4e;
 // Simd opcodes.
 let kExprS128LoadMem = 0x00;
 let kExprS128StoreMem = 0x0b;
+let kExprS128Const = 0x0c;
 let kExprS8x16Shuffle = 0x0d;
 let kExprI8x16Splat = 0x0f;
 let kExprI16x8Splat = 0x10;
 let kExprI32x4Splat = 0x11;
 let kExprF32x4Splat = 0x13;
-let kExprI32x4ExtractLane = 0x15;
+let kExprI32x4ExtractLane = 0x1b;
 let kExprI8x16LtU = 0x26;
 let kExprI8x16LeU = 0x2a;
 let kExprI32x4Eq = 0x37;
@@ -1124,6 +1125,9 @@ class WasmModuleBuilder {
             case kWasmF64:
               section.emit_bytes(wasmF64Const(global.init));
               break;
+            case kWasmS128:
+              section.emit_bytes(wasmS128Const(global.init));
+              break;
             case kWasmExternRef:
               section.emit_u8(kExprRefNull);
               section.emit_u8(kWasmExternRef);
@@ -1482,3 +1486,8 @@ function wasmF64Const(f) {
     byte_view[3], byte_view[4], byte_view[5], byte_view[6], byte_view[7]
   ];
 }
+
+function wasmS128Const(f) {
+  // Write in little-endian order at offset 0.
+  return [kSimdPrefix, kExprS128Const, ...f];
+}

test/unittests/wasm/module-decoder-unittest.cc

@@ -270,12 +270,13 @@ TEST_F(WasmModuleVerifyTest, OneGlobal) {
 
 TEST_F(WasmModuleVerifyTest, S128Global) {
   WASM_FEATURE_SCOPE(simd);
-  uint8_t v[kSimd128Size] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
+  std::array<uint8_t, kSimd128Size> v = {1, 2,  3,  4,  5,  6,  7, 8,
+                                         9, 10, 11, 12, 13, 14, 15};
   static const byte data[] = {SECTION(Global,          // --
                                       ENTRY_COUNT(1),  // --
                                       kLocalS128,      // memory type
                                       0,               // immutable
-                                      WASM_SIMD_CONSTANT(v), kExprEnd)};
+                                      WASM_SIMD_CONSTANT(v.data()), kExprEnd)};
   ModuleResult result = DecodeModule(data, data + sizeof(data));
   EXPECT_OK(result);
   const WasmGlobal* global = &result.value()->globals.back();
@@ -283,7 +284,7 @@ TEST_F(WasmModuleVerifyTest, S128Global) {
   EXPECT_EQ(0u, global->offset);
   EXPECT_FALSE(global->mutability);
   EXPECT_EQ(WasmInitExpr::kS128Const, global->init.kind());
-  EXPECT_EQ(0, memcmp(global->init.immediate().s128_const, v, kSimd128Size));
+  EXPECT_EQ(global->init.immediate().s128_const, v);
 }
 
 TEST_F(WasmModuleVerifyTest, ExternRefGlobal) {