[wasm] Fix wasm decoder for multi-byte opcodes

SIMD opcodes consist of the prefix byte, then an LEB128 encoded int. We
were decoding this incorrectly as a fixed uint8. This fixes the decoder
to properly handle multi bytes.

In some cases, the multi byte logic is applied to all prefixed opcodes.
This is not a problem, since for values < 0x80, the LEB encoding is a
single byte, and decodes to the same int. If the prefix opcode has
instructions with index >= 0x80, it would be required to be LEB128
encoded anyway.

There are a bunch of trivial changes to test-run-wasm-simd, to change
the macro from BUILD to BUILD_V, the former only works for single byte
opcodes, the latter is a new template-based macro that correct handles
multi-byte opcodes. The only unchanged test is the shuffle fuzzer test,
which builds its own sequence of bytes without using the BUILD macro.

Bug: v8:10258
Change-Id: Ie7377e899a7eab97ecf28176fd908babc08d0f19
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2118476
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67186}

src/wasm/baseline/liftoff-compiler.cc

@@ -731,10 +731,8 @@ class LiftoffCompiler {
     TraceCacheState(decoder);
 #ifdef DEBUG
     SLOW_DCHECK(__ ValidateCacheState());
-    if (WasmOpcodes::IsPrefixOpcode(opcode) &&
-        decoder->pc() + 1 < decoder->end()) {
-      byte op_index = *(decoder->pc() + 1);
-      opcode = static_cast<WasmOpcode>(opcode << 8 | op_index);
+    if (WasmOpcodes::IsPrefixOpcode(opcode)) {
+      opcode = decoder->read_prefixed_opcode<Decoder::kValidate>(decoder->pc());
     }
     DEBUG_CODE_COMMENT(WasmOpcodes::OpcodeName(opcode));
 #endif

src/wasm/decoder.h

@@ -15,6 +15,7 @@
 #include "src/flags/flags.h"
 #include "src/utils/utils.h"
 #include "src/utils/vector.h"
+#include "src/wasm/wasm-opcodes.h"
 #include "src/wasm/wasm-result.h"
 #include "src/zone/zone-containers.h"
 
@@ -126,6 +127,31 @@ class Decoder {
                                                                name);
   }
 
+  // Reads a prefixed-opcode, possibly with variable-length index.
+  // The length param is set to the number of bytes this index is encoded with.
+  // For most cases (non variable-length), it will be 1.
+  template <ValidateFlag validate>
+  WasmOpcode read_prefixed_opcode(const byte* pc, uint32_t* length = nullptr,
+                                  const char* name = "prefixed opcode") {
+    uint32_t unused_length;
+    if (length == nullptr) {
+      length = &unused_length;
+    }
+    DCHECK(WasmOpcodes::IsPrefixOpcode(static_cast<WasmOpcode>(*pc)));
+    uint32_t index;
+    if (*pc == WasmOpcode::kSimdPrefix) {
+      // SIMD opcodes can be multiple bytes (when LEB128 encoded).
+      index = read_u32v<validate>(pc + 1, length, "prefixed opcode index");
+      // Only support SIMD opcodes that go up to 0xFF (when decoded). Anything
+      // bigger will need 1 more byte, and the '<< 8' below will be wrong.
+      DCHECK_LE(index, 0xff);
+    } else {
+      index = *(pc + 1);
+      *length = 1;
+    }
+    return static_cast<WasmOpcode>((*pc) << 8 | index);
+  }
+
   // Reads a 8-bit unsigned integer (byte) and advances {pc_}.
   uint8_t consume_u8(const char* name = "uint8_t") {
     return consume_little_endian<uint8_t>(name);

src/wasm/function-body-decoder-impl.h

@@ -1379,31 +1379,30 @@ class WasmDecoder : public Decoder {
         }
       }
       case kSimdPrefix: {
-        byte simd_index = decoder->read_u8<validate>(pc + 1, "simd_index");
-        WasmOpcode opcode =
-            static_cast<WasmOpcode>(kSimdPrefix << 8 | simd_index);
+        uint32_t length = 0;
+        opcode = decoder->read_prefixed_opcode<validate>(pc, &length);
         switch (opcode) {
 #define DECLARE_OPCODE_CASE(name, opcode, sig) case kExpr##name:
           FOREACH_SIMD_0_OPERAND_OPCODE(DECLARE_OPCODE_CASE)
 #undef DECLARE_OPCODE_CASE
-          return 2;
+          return 1 + length;
 #define DECLARE_OPCODE_CASE(name, opcode, sig) case kExpr##name:
           FOREACH_SIMD_1_OPERAND_OPCODE(DECLARE_OPCODE_CASE)
 #undef DECLARE_OPCODE_CASE
-          return 3;
+          return 2 + length;
 #define DECLARE_OPCODE_CASE(name, opcode, sig) case kExpr##name:
           FOREACH_SIMD_MEM_OPCODE(DECLARE_OPCODE_CASE)
 #undef DECLARE_OPCODE_CASE
           {
             MemoryAccessImmediate<validate> imm(decoder, pc + 1, UINT32_MAX);
-            return 2 + imm.length;
+            return 1 + length + imm.length;
           }
           // Shuffles require a byte per lane, or 16 immediate bytes.
           case kExprS8x16Shuffle:
-            return 2 + kSimd128Size;
+            return 1 + length + kSimd128Size;
           default:
             decoder->error(pc, "invalid SIMD opcode");
-            return 2;
+            return 1 + length;
         }
       }
       case kAtomicPrefix: {
@@ -1508,7 +1507,7 @@ class WasmDecoder : public Decoder {
       case kNumericPrefix:
       case kAtomicPrefix:
       case kSimdPrefix: {
-        opcode = static_cast<WasmOpcode>(opcode << 8 | *(pc + 1));
+        opcode = this->read_prefixed_opcode<validate>(pc);
         switch (opcode) {
           FOREACH_SIMD_1_OPERAND_1_PARAM_OPCODE(DECLARE_OPCODE_CASE)
             return {1, 1};
@@ -1622,12 +1621,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     if (!WasmOpcodes::IsPrefixOpcode(opcode)) {
       return WasmOpcodes::OpcodeName(static_cast<WasmOpcode>(opcode));
     }
-    // We need one more byte.
-    ++pc;
-    if (pc >= this->end_) return "<end>";
-    byte sub_opcode = *pc;
-    opcode = static_cast<WasmOpcode>(opcode << 8 | sub_opcode);
-    return WasmOpcodes::OpcodeName(static_cast<WasmOpcode>(opcode));
+    opcode = this->template read_prefixed_opcode<Decoder::kValidate>(pc);
+    return WasmOpcodes::OpcodeName(opcode);
   }
 
   inline Zone* zone() const { return zone_; }
@@ -2355,13 +2350,14 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         }
         case kSimdPrefix: {
           CHECK_PROTOTYPE_OPCODE(simd);
-          len++;
-          byte simd_index =
-              this->template read_u8<validate>(this->pc_ + 1, "simd index");
-          opcode = static_cast<WasmOpcode>(opcode << 8 | simd_index);
+          uint32_t length = 0;
+          opcode =
+              this->template read_prefixed_opcode<validate>(this->pc_, &length);
+          len += length;
+
           TRACE_PART(TRACE_INST_FORMAT, startrel(this->pc_),
                      WasmOpcodes::OpcodeName(opcode));
-          len += DecodeSimdOpcode(opcode);
+          len += DecodeSimdOpcode(opcode, length);
           break;
         }
         case kAtomicPrefix: {
@@ -2426,7 +2422,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           auto& val = stack_[i];
           WasmOpcode opcode = static_cast<WasmOpcode>(*val.pc);
           if (WasmOpcodes::IsPrefixOpcode(opcode)) {
-            opcode = static_cast<WasmOpcode>(opcode << 8 | *(val.pc + 1));
+            opcode = this->template read_prefixed_opcode<Decoder::kNoValidate>(
+                val.pc);
           }
           TRACE_PART(" %c@%d:%s", val.type.short_name(),
                      static_cast<int>(val.pc - this->start_),
@@ -2546,9 +2543,11 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     return imm.length;
   }
 
-  int DecodeLoadTransformMem(LoadType type, LoadTransformationKind transform) {
+  int DecodeLoadTransformMem(LoadType type, LoadTransformationKind transform,
+                             uint32_t opcode_length) {
     if (!CheckHasMemory()) return 0;
-    MemoryAccessImmediate<validate> imm(this, this->pc_ + 1, type.size_log_2());
+    MemoryAccessImmediate<validate> imm(this, this->pc_ + opcode_length,
+                                        type.size_log_2());
     auto index = Pop(0, kWasmI32);
     auto* result = Push(kWasmS128);
     CALL_INTERFACE_IF_REACHABLE(LoadTransform, type, transform, imm, index,
@@ -2686,8 +2685,15 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     return 16;
   }
 
-  uint32_t DecodeSimdOpcode(WasmOpcode opcode) {
+  uint32_t DecodeSimdOpcode(WasmOpcode opcode, uint32_t opcode_length) {
+    // opcode_length is the number of bytes that this SIMD-specific opcode takes
+    // up in the LEB128 encoded form.
     uint32_t len = 0;
+    // TODO(v8:10258): Most of the decodings below (like SimdExtractLane) should
+    // take opcode_length as a parameter, since that will determine where the
+    // immediate is located. However, for most of these instructions, their
+    // encoded opcodes take up 2 bytes, so they will not be affected by the
+    // variable-length encoding, and will still work correctly.
     switch (opcode) {
       case kExprF64x2ExtractLane: {
         len = SimdExtractLane(opcode, kWasmF64);
@@ -2739,43 +2745,51 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         break;
       case kExprS8x16LoadSplat:
         len = DecodeLoadTransformMem(LoadType::kI32Load8S,
-                                     LoadTransformationKind::kSplat);
+                                     LoadTransformationKind::kSplat,
+                                     opcode_length);
         break;
       case kExprS16x8LoadSplat:
         len = DecodeLoadTransformMem(LoadType::kI32Load16S,
-                                     LoadTransformationKind::kSplat);
+                                     LoadTransformationKind::kSplat,
+                                     opcode_length);
         break;
       case kExprS32x4LoadSplat:
-        len = DecodeLoadTransformMem(LoadType::kI32Load,
-                                     LoadTransformationKind::kSplat);
+        len = DecodeLoadTransformMem(
+            LoadType::kI32Load, LoadTransformationKind::kSplat, opcode_length);
         break;
       case kExprS64x2LoadSplat:
-        len = DecodeLoadTransformMem(LoadType::kI64Load,
-                                     LoadTransformationKind::kSplat);
+        len = DecodeLoadTransformMem(
+            LoadType::kI64Load, LoadTransformationKind::kSplat, opcode_length);
         break;
       case kExprI16x8Load8x8S:
         len = DecodeLoadTransformMem(LoadType::kI32Load8S,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       case kExprI16x8Load8x8U:
         len = DecodeLoadTransformMem(LoadType::kI32Load8U,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       case kExprI32x4Load16x4S:
         len = DecodeLoadTransformMem(LoadType::kI32Load16S,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       case kExprI32x4Load16x4U:
         len = DecodeLoadTransformMem(LoadType::kI32Load16U,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       case kExprI64x2Load32x2S:
         len = DecodeLoadTransformMem(LoadType::kI64Load32S,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       case kExprI64x2Load32x2U:
         len = DecodeLoadTransformMem(LoadType::kI64Load32U,
-                                     LoadTransformationKind::kExtend);
+                                     LoadTransformationKind::kExtend,
+                                     opcode_length);
         break;
       default: {
         if (!FLAG_wasm_simd_post_mvp &&

src/wasm/function-body-decoder.h

@@ -173,9 +173,7 @@ class V8_EXPORT_PRIVATE BytecodeIterator : public NON_EXPORTED_BASE(Decoder) {
   bool has_next() { return pc_ < end_; }
 
   WasmOpcode prefixed_opcode() {
-    byte prefix = read_u8<Decoder::kNoValidate>(pc_, "expected prefix");
-    byte index = read_u8<Decoder::kNoValidate>(pc_ + 1, "expected index");
-    return static_cast<WasmOpcode>(prefix << 8 | index);
+    return read_prefixed_opcode<Decoder::kNoValidate>(pc_);
   }
 };
 

src/wasm/wasm-interpreter.cc

@@ -1748,7 +1748,7 @@ class ThreadImpl {
       DoTrap(kTrapUnalignedAccess, pc);
       return false;
     }
-    *len = 2 + imm.length;
+    *len += imm.length;
     return true;
   }
 
@@ -1777,7 +1777,7 @@ class ThreadImpl {
       DoTrap(kTrapUnalignedAccess, pc);
       return false;
     }
-    *len = 2 + imm.length;
+    *len += imm.length;
     return true;
   }
 
@@ -2167,7 +2167,7 @@ class ThreadImpl {
 #undef ATOMIC_STORE_CASE
       case kExprAtomicFence:
         std::atomic_thread_fence(std::memory_order_seq_cst);
-        *len += 2;
+        *len += 1;
         break;
       case kExprI32AtomicWait: {
         int32_t val;
@@ -3038,13 +3038,16 @@ class ThreadImpl {
       byte orig = code->start[pc];
       WasmOpcode opcode = static_cast<WasmOpcode>(orig);
       if (WasmOpcodes::IsPrefixOpcode(opcode)) {
-        opcode = static_cast<WasmOpcode>(opcode << 8 | code->start[pc + 1]);
+        uint32_t length;
+        opcode = decoder.read_prefixed_opcode<Decoder::kNoValidate>(
+            &code->start[pc], &length);
+        len += length;
       }
       if (V8_UNLIKELY(orig == kInternalBreakpoint)) {
         orig = code->orig_start[pc];
         if (WasmOpcodes::IsPrefixOpcode(static_cast<WasmOpcode>(orig))) {
-          opcode =
-              static_cast<WasmOpcode>(orig << 8 | code->orig_start[pc + 1]);
+          opcode = decoder.read_prefixed_opcode<Decoder::kNoValidate>(
+              &code->start[pc]);
         }
         if (SkipBreakpoint(code, pc)) {
           // skip breakpoint by switching on original code.
@@ -3647,7 +3650,6 @@ class ThreadImpl {
           break;
         }
         case kNumericPrefix: {
-          ++len;
           if (!ExecuteNumericOp(opcode, &decoder, code, pc, &len)) return;
           break;
         }
@@ -3656,7 +3658,6 @@ class ThreadImpl {
           break;
         }
         case kSimdPrefix: {
-          ++len;
           if (!ExecuteSimdOp(opcode, &decoder, code, pc, &len)) return;
           break;
         }

test/cctest/wasm/wasm-run-utils.cc

@@ -47,6 +47,11 @@ void AppendSingle(std::vector<byte>* code, WasmOpcode op) {
   }
 }
 
+template <>
+void AppendSingle(std::vector<byte>* code, ValueTypeCode op) {
+  code->push_back(op);
+}
+
 TestingModuleBuilder::TestingModuleBuilder(
     Zone* zone, ManuallyImportedJSFunction* maybe_import, ExecutionTier tier,
     RuntimeExceptionSupport exception_support, LowerSimd lower_simd)

test/cctest/wasm/wasm-run-utils.h

@@ -86,6 +86,10 @@ void AppendSingle(std::vector<byte>* code, T t) {
 template <>
 void AppendSingle<WasmOpcode>(std::vector<byte>* code, WasmOpcode op);
 
+// Specialized for ValueTypeCode.
+template <>
+void AppendSingle<ValueTypeCode>(std::vector<byte>* code, ValueTypeCode op);
+
 template <typename... T>
 void Append(std::vector<byte>* code, T... ts) {
   static_assert(sizeof...(ts) == 0, "Base case for appending bytes to code.");

test/mjsunit/regress/wasm/regress-1055692.js

@@ -17,7 +17,7 @@ builder.addFunction(undefined, 0 /* sig */)
 kExprI32Const, 0x75,  // i32.const
 kExprI32Const, 0x74,  // i32.const
 kExprI32Const, 0x18,  // i32.const
-kSimdPrefix, kExprS8x16LoadSplat,  // s8x16.load_splat
+kSimdPrefix, ...kExprS8x16LoadSplat,  // s8x16.load_splat
 kExprUnreachable,  // unreachable
 kExprUnreachable,  // unreachable
 kExprI32Const, 0x6f,  // i32.const

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

@@ -479,7 +479,7 @@ let kExprI16x8ShrS = 0x66;
 let kExprS1x4AllTrue = 0x75;
 let kExprI32x4Add = 0x79;
 let kExprF32x4Min = 0x9e;
-let kExprS8x16LoadSplat = 0xc2;
+let kExprS8x16LoadSplat = [0xc2, 0x1];
 
 // Compilation hint constants.
 let kCompilationHintStrategyDefault = 0x00;

test/unittests/wasm/function-body-decoder-unittest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "src/wasm/leb-helper.h"
 #include "test/unittests/test-utils.h"
 
 #include "src/init/v8.h"
@@ -3628,6 +3629,20 @@ class WasmOpcodeLengthTest : public TestWithZone {
     EXPECT_EQ(expected, OpcodeLength(code, code + sizeof(code)))
         << PrintOpcodes{code, code + sizeof...(bytes)};
   }
+
+  // Helper to check for prefixed opcodes, which can have multiple bytes.
+  void ExpectLengthPrefixed(unsigned operands, WasmOpcode opcode) {
+    uint8_t prefix = (opcode >> 8) & 0xff;
+    DCHECK(WasmOpcodes::IsPrefixOpcode(static_cast<WasmOpcode>(prefix)));
+    uint8_t index = opcode & 0xff;
+    uint8_t encoded[2] = {0, 0};
+    uint8_t* p = encoded;
+    unsigned len = static_cast<unsigned>(LEBHelper::sizeof_u32v(index));
+    DCHECK_GE(2, len);
+    LEBHelper::write_u32v(&p, index);
+    // length of index, + number of operands + prefix bye
+    ExpectLength(len + operands + 1, prefix, encoded[0], encoded[1]);
+  }
 };
 
 TEST_F(WasmOpcodeLengthTest, Statements) {
@@ -3754,17 +3769,15 @@ TEST_F(WasmOpcodeLengthTest, SimpleExpressions) {
 }
 
 TEST_F(WasmOpcodeLengthTest, SimdExpressions) {
-#define TEST_SIMD(name, opcode, sig) \
-  ExpectLength(2, kSimdPrefix, static_cast<byte>(kExpr##name & 0xFF));
+#define TEST_SIMD(name, opcode, sig) ExpectLengthPrefixed(0, kExpr##name);
   FOREACH_SIMD_0_OPERAND_OPCODE(TEST_SIMD)
 #undef TEST_SIMD
-#define TEST_SIMD(name, opcode, sig) \
-  ExpectLength(3, kSimdPrefix, static_cast<byte>(kExpr##name & 0xFF));
+#define TEST_SIMD(name, opcode, sig) ExpectLengthPrefixed(1, kExpr##name);
   FOREACH_SIMD_1_OPERAND_OPCODE(TEST_SIMD)
 #undef TEST_SIMD
-  ExpectLength(18, kSimdPrefix, static_cast<byte>(kExprS8x16Shuffle & 0xFF));
-  // test for bad simd opcode
-  ExpectLength(2, kSimdPrefix, 0xFF);
+  ExpectLengthPrefixed(16, kExprS8x16Shuffle);
+  // test for bad simd opcode, 0xFF is encoded in two bytes.
+  ExpectLength(3, kSimdPrefix, 0xFF, 0x1);
 }
 
 using TypesOfLocals = ZoneVector<ValueType>;