[stringrefs] Switch GC prefix decoding to LEB

So far, we decoded instructions with the 0xFB prefix as two-byte, i.e.
a single "u8" byte following the prefix.
This patch changes that to 0xFB + LEB, which is how all prefixed
instructions are supposed to do it. Currently this makes a difference
only for the stringref proposal (instructions 0x80 through 0xb3).

It has the unfortunate consequence that all stringref instructions need
three bytes for now. We expect them to go back to a two-byte encoding
scheme (while remaining LEB compliant) when their final encoding is
decided.

Bug: v8:12868
Change-Id: I603f60adae88e9b985cb65288d9eeb7f98da8138
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3825887
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82400}

src/wasm/decoder.h

@@ -135,16 +135,6 @@ class Decoder {
     return read_leb<int64_t, validate, kNoTrace, 33>(pc, length, name);
   }
 
-  template <ValidateFlag validate>
-  WasmOpcode read_two_byte_opcode(const byte* pc, uint32_t* length,
-                                  const char* name = "prefixed opcode") {
-    DCHECK(*pc == kGCPrefix);
-    uint32_t index = read_u8<validate>(pc + 1, name);
-    index |= kGCPrefix << 8;
-    *length = 2;
-    return static_cast<WasmOpcode>(index);
-  }
-
   // Convenient overload for callers who don't care about length.
   template <ValidateFlag validate>
   WasmOpcode read_prefixed_opcode(const byte* pc) {
@@ -158,10 +148,6 @@ class Decoder {
   template <ValidateFlag validate>
   WasmOpcode read_prefixed_opcode(const byte* pc, uint32_t* length,
                                   const char* name = "prefixed opcode") {
-    if (*pc == kGCPrefix) {
-      return read_two_byte_opcode<validate>(pc, length, name);
-    }
-
     uint32_t index;
 
     // Prefixed opcodes all use LEB128 encoding.

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

@@ -1983,7 +1983,7 @@ class WasmDecoder : public Decoder {
       case kGCPrefix: {
         uint32_t length = 0;
         opcode =
-            decoder->read_two_byte_opcode<validate>(pc, &length, "gc_index");
+            decoder->read_prefixed_opcode<validate>(pc, &length, "gc_index");
         switch (opcode) {
           case kExprStructNew:
           case kExprStructNewDefault: {
@@ -2256,7 +2256,7 @@ class WasmDecoder : public Decoder {
       }
       case kGCPrefix: {
         uint32_t unused_length;
-        opcode = this->read_two_byte_opcode<validate>(pc, &unused_length);
+        opcode = this->read_prefixed_opcode<validate>(pc, &unused_length);
         switch (opcode) {
           case kExprStructGet:
           case kExprStructGetS:
@@ -3668,7 +3668,7 @@ class WasmFullDecoder : public WasmDecoder<validate, decoding_mode> {
 
   DECODE(GC) {
     uint32_t opcode_length = 0;
-    WasmOpcode full_opcode = this->template read_two_byte_opcode<validate>(
+    WasmOpcode full_opcode = this->template read_prefixed_opcode<validate>(
         this->pc_, &opcode_length, "gc index");
     trace_msg->AppendOpcode(full_opcode);
     if (full_opcode >= kExprStringNewWtf8) {

src/wasm/wasm-disassembler.cc

@@ -255,10 +255,7 @@ void FunctionBodyDisassembler::DecodeGlobalInitializer(StringBuilder& out) {
 WasmOpcode FunctionBodyDisassembler::GetOpcode() {
   WasmOpcode opcode = static_cast<WasmOpcode>(*pc_);
   if (!WasmOpcodes::IsPrefixOpcode(opcode)) return opcode;
-  uint32_t opcode_length = 1;
-  if (opcode == kGCPrefix) {
-    return read_two_byte_opcode<validate>(pc_, &opcode_length);
-  }
+  uint32_t opcode_length;
   return read_prefixed_opcode<validate>(pc_, &opcode_length);
 }
 

src/wasm/wasm-module-builder.cc

@@ -88,16 +88,20 @@ void WasmFunctionBuilder::EmitCode(const byte* code, uint32_t code_size) {
   body_.write(code, code_size);
 }
 
-void WasmFunctionBuilder::Emit(WasmOpcode opcode) { body_.write_u8(opcode); }
+void WasmFunctionBuilder::Emit(WasmOpcode opcode) {
+  DCHECK_LE(opcode, 0xFF);
+  body_.write_u8(opcode);
+}
 
 void WasmFunctionBuilder::EmitWithPrefix(WasmOpcode opcode) {
-  DCHECK_NE(0, opcode & 0xff00);
-  body_.write_u8(opcode >> 8);
-  if ((opcode >> 8) == WasmOpcode::kSimdPrefix) {
-    // SIMD opcodes are LEB encoded
-    body_.write_u32v(opcode & 0xff);
+  DCHECK_GT(opcode, 0xFF);
+  if (opcode > 0xFFFF) {
+    DCHECK_EQ(kSimdPrefix, opcode >> 12);
+    body_.write_u8(kSimdPrefix);
+    body_.write_u32v(opcode & 0xFFF);
   } else {
-    body_.write_u8(opcode);
+    body_.write_u8(opcode >> 8);      // Prefix.
+    body_.write_u32v(opcode & 0xff);  // LEB encoded tail.
   }
 }
 
@@ -525,6 +529,8 @@ void WriteInitializerExpressionWithEnd(ZoneBuffer* buffer,
     case WasmInitExpr::kStructNewDefault:
       static_assert((kExprStructNew >> 8) == kGCPrefix);
       static_assert((kExprStructNewDefault >> 8) == kGCPrefix);
+      static_assert((kExprStructNew & 0x80) == 0);
+      static_assert((kExprStructNewDefault & 0x80) == 0);
       for (const WasmInitExpr& operand : *init.operands()) {
         WriteInitializerExpressionWithEnd(buffer, operand, kWasmBottom);
       }
@@ -545,6 +551,7 @@ void WriteInitializerExpressionWithEnd(ZoneBuffer* buffer,
       break;
     case WasmInitExpr::kArrayNewFixedStatic: {
       static_assert((kExprArrayNewFixedStatic >> 8) == kGCPrefix);
+      static_assert((kExprArrayNewFixedStatic & 0x80) == 0);
       for (const WasmInitExpr& operand : *init.operands()) {
         WriteInitializerExpressionWithEnd(buffer, operand, kWasmBottom);
       }
@@ -558,6 +565,7 @@ void WriteInitializerExpressionWithEnd(ZoneBuffer* buffer,
       WriteInitializerExpressionWithEnd(buffer, (*init.operands())[0],
                                         kWasmI32);
       static_assert((kExprI31New >> 8) == kGCPrefix);
+      static_assert((kExprI31New & 0x80) == 0);
 
       buffer->write_u8(kGCPrefix);
       buffer->write_u8(static_cast<uint8_t>(kExprI31New));
@@ -565,7 +573,7 @@ void WriteInitializerExpressionWithEnd(ZoneBuffer* buffer,
       break;
     case WasmInitExpr::kStringConst:
       buffer->write_u8(kGCPrefix);
-      buffer->write_u8(static_cast<uint8_t>(kExprStringConst));
+      buffer->write_u32v(kExprStringConst & 0xFF);
       buffer->write_u32v(init.immediate().index);
       break;
   }

test/fuzzer/wasm-fuzzer-common.cc

@@ -455,7 +455,7 @@ class InitExprInterface {
 
   void StringConst(FullDecoder* decoder,
                    const StringConstImmediate<validate>& imm, Value* result) {
-    os_ << "kGCPrefix, kExprStringConst, " << index(imm.index);
+    os_ << "...GCInstr(kExprStringConst), " << index(imm.index);
   }
 
   void DoReturn(FullDecoder* decoder, uint32_t /*drop_values*/) { os_ << "]"; }

test/mjsunit/wasm/stringrefs-exec-gc.js

@@ -122,7 +122,7 @@ function makeWtf8TestDataSegment() {
       .addBody([
         kExprCallFunction, make_i8_array,
         kExprLocalGet, 0, kExprLocalGet, 1,
-        kGCPrefix, kExprStringNewWtf8Array, policy
+        ...GCInstr(kExprStringNewWtf8Array), policy
       ]);
   }
 
@@ -133,7 +133,7 @@ function makeWtf8TestDataSegment() {
       ...wasmI32Const("ascii".length),
       kGCPrefix, kExprArrayNewDataStatic, i8_array, ascii_data_index,
       kExprLocalGet, 0, kExprLocalGet, 1,
-      kGCPrefix, kExprStringNewWtf8Array, kWtf8PolicyAccept
+      ...GCInstr(kExprStringNewWtf8Array), kWtf8PolicyAccept
     ]);
 
   let instance = builder.instantiate();
@@ -228,7 +228,7 @@ function makeWtf16TestDataSegment() {
     .addBody([
       kExprCallFunction, make_i16_array,
       kExprLocalGet, 0, kExprLocalGet, 1,
-      kGCPrefix, kExprStringNewWtf16Array
+      ...GCInstr(kExprStringNewWtf16Array)
     ]);
 
   builder.addFunction("bounds_check", kSig_w_ii)
@@ -238,7 +238,7 @@ function makeWtf16TestDataSegment() {
       ...wasmI32Const("ascii".length),
       kGCPrefix, kExprArrayNewDataStatic, i16_array, ascii_data_index,
       kExprLocalGet, 0, kExprLocalGet, 1,
-      kGCPrefix, kExprStringNewWtf16Array
+      ...GCInstr(kExprStringNewWtf16Array)
     ]);
 
   let instance = builder.instantiate();
@@ -286,14 +286,14 @@ function makeWtf16TestDataSegment() {
         kExprLocalGet, 0,
         kExprLocalGet, 3,
         kExprLocalGet, 2,
-        kGCPrefix, kExprStringEncodeWtf8Array, policy,
+        ...GCInstr(kExprStringEncodeWtf8Array), policy,
         kExprLocalSet, 4,
 
         // Read buffer.
         kExprLocalGet, 3,
         kExprLocalGet, 2,
         kExprLocalGet, 2, kExprLocalGet, 4, kExprI32Add,
-        kGCPrefix, kExprStringNewWtf8Array, kWtf8PolicyAccept,
+        ...GCInstr(kExprStringNewWtf8Array), kWtf8PolicyAccept,
       ]);
   }
 
@@ -303,17 +303,17 @@ function makeWtf16TestDataSegment() {
         kExprRefNull, kStringRefCode,
         kExprI32Const, 0, kGCPrefix, kExprArrayNewDefault, i8_array,
         kExprI32Const, 0,
-        kGCPrefix, kExprStringEncodeWtf8Array, 0,
+        ...GCInstr(kExprStringEncodeWtf8Array), 0,
       ]);
   builder.addFunction("encode_null_array", kSig_i_v)
     .exportFunc()
     .addBody([
         kExprI32Const, 0, kGCPrefix, kExprArrayNewDefault, i8_array,
         kExprI32Const, 0, kExprI32Const, 0,
-        kGCPrefix, kExprStringNewWtf8Array, kWtf8PolicyAccept,
+        ...GCInstr(kExprStringNewWtf8Array), kWtf8PolicyAccept,
         kExprRefNull, i8_array,
         kExprI32Const, 0,
-        kGCPrefix, kExprStringEncodeWtf8Array, kWtf8PolicyAccept,
+        ...GCInstr(kExprStringEncodeWtf8Array), kWtf8PolicyAccept,
       ]);
 
   let instance = builder.instantiate();
@@ -389,14 +389,14 @@ function makeWtf16TestDataSegment() {
       kExprLocalGet, 0,
       kExprLocalGet, 3,
       kExprLocalGet, 2,
-      kGCPrefix, kExprStringEncodeWtf16Array,
+      ...GCInstr(kExprStringEncodeWtf16Array),
       kExprLocalSet, 4,
 
       // Read buffer.
       kExprLocalGet, 3,
       kExprLocalGet, 2,
       kExprLocalGet, 2, kExprLocalGet, 4, kExprI32Add,
-      kGCPrefix, kExprStringNewWtf16Array,
+      ...GCInstr(kExprStringNewWtf16Array),
     ]);
 
   builder.addFunction("encode_null_string", kSig_i_v)
@@ -405,17 +405,17 @@ function makeWtf16TestDataSegment() {
         kExprRefNull, kStringRefCode,
         kExprI32Const, 0, kGCPrefix, kExprArrayNewDefault, i16_array,
         kExprI32Const, 0,
-        kGCPrefix, kExprStringEncodeWtf16Array
+        ...GCInstr(kExprStringEncodeWtf16Array)
       ]);
   builder.addFunction("encode_null_array", kSig_i_v)
     .exportFunc()
     .addBody([
         kExprI32Const, 0, kGCPrefix, kExprArrayNewDefault, i16_array,
         kExprI32Const, 0, kExprI32Const, 0,
-        kGCPrefix, kExprStringNewWtf16Array,
+        ...GCInstr(kExprStringNewWtf16Array),
         kExprRefNull, i16_array,
         kExprI32Const, 0,
-        kGCPrefix, kExprStringEncodeWtf16Array
+        ...GCInstr(kExprStringEncodeWtf16Array)
       ]);
 
   let instance = builder.instantiate();

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

@@ -469,6 +469,16 @@ for (let prefix in kPrefixOpcodes) {
   defineWasmOpcode(`k${prefix}Prefix`, kPrefixOpcodes[prefix]);
 }
 
+// Use these for multi-byte instructions (opcode > 0x7F needing two LEB bytes):
+function SimdInstr(opcode) {
+  if (opcode <= 0x7F) return [kSimdPrefix, opcode];
+  return [kSimdPrefix, 0x80 | (opcode & 0x7F), opcode >> 7];
+}
+function GCInstr(opcode) {
+  if (opcode <= 0x7F) return [kGCPrefix, opcode];
+  return [kGCPrefix, 0x80 | (opcode & 0x7F), opcode >> 7];
+}
+
 // GC opcodes
 let kExprStructGet = 0x03;
 let kExprStructGetS = 0x04;

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

@@ -4821,17 +4821,15 @@ TEST_F(WasmOpcodeLengthTest, GCOpcodes) {
   ExpectLength(4, 0xfb, kExprBrOnCastStatic & 0xFF);
   ExpectLength(4, 0xfb, kExprBrOnCastStaticFail & 0xFF);
 
-  // GC opcodes aren't parsed as LEBs.
-
   // struct.new, with leb immediate operand.
   ExpectLength(3, 0xfb, 0x07, 0x42);
   ExpectLength(4, 0xfb, 0x07, 0x80, 0x00);
 
   // string.new_wtf8 with $mem=0, $policy=0.
-  ExpectLength(4, 0xfb, 0x80, 0x00, 0x00);
+  ExpectLength(5, 0xfb, 0x80, 0x01, 0x00, 0x00);
 
   // string.as_wtf8.
-  ExpectLength(2, 0xfb, 0x90);
+  ExpectLength(3, 0xfb, 0x90, 0x01);
 }
 
 TEST_F(WasmOpcodeLengthTest, PrefixedOpcodesLEB) {