[wasm-gc][bug] Fix various bugs in wasm gc code

Changes:
- Unpack packed typed in arrays/structs where needed.
- i8 should have log-size 0.
- Use typed-funcref feature flag instead of gc where appropriate.
- Set argument indexes correctly for gc opcodes in
  function-body-decoder.
- Remove no-longer valid TODOs.

Bug: v8:7748
Change-Id: I1a73794d0f93da6c7177e496d47df4106031f0eb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2230520
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68196}

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

@@ -2119,7 +2119,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           break;
         }
         case kExprBrOnNull: {
-          CHECK_PROTOTYPE_OPCODE(gc);
+          CHECK_PROTOTYPE_OPCODE(typed_funcref);
           BranchDepthImmediate<validate> imm(this, this->pc_);
           if (!this->Validate(this->pc_, imm, control_.size())) break;
           len = 1 + imm.length;
@@ -2180,7 +2180,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           uint32_t locals_count = static_cast<uint32_t>(local_type_vec_.size() -
                                                         current_local_count);
           ArgVector let_local_values =
-              PopArgs(VectorOf(local_type_vec_.data(), locals_count));
+              PopArgs(static_cast<uint32_t>(imm.in_arity()),
+                      VectorOf(local_type_vec_.data(), locals_count));
           ArgVector args = PopArgs(imm.sig);
           Control* let_block = PushControl(kControlLet, locals_count);
           SetBlockType(let_block, imm, args.begin());
@@ -2471,7 +2472,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           break;
         }
         case kExprRefAsNonNull: {
-          CHECK_PROTOTYPE_OPCODE(gc);
+          CHECK_PROTOTYPE_OPCODE(typed_funcref);
           Value value = Pop();
           switch (value.type.kind()) {
             case ValueType::kRef: {
@@ -2903,15 +2904,16 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     int count = static_cast<int>(type->field_count());
     ArgVector args(count);
     for (int i = count - 1; i >= 0; i--) {
-      args[i] = Pop(i, type->field(i));
+      args[i] = Pop(i, type->field(i).Unpack());
     }
     return args;
   }
 
-  V8_INLINE ArgVector PopArgs(Vector<ValueType> arg_types) {
+  V8_INLINE ArgVector PopArgs(uint32_t base_index,
+                              Vector<ValueType> arg_types) {
     ArgVector args(arg_types.size());
     for (int i = static_cast<int>(arg_types.size()) - 1; i >= 0; i--) {
-      args[i] = Pop(i, arg_types[i]);
+      args[i] = Pop(base_index + i, arg_types[i]);
     }
     return args;
   }
@@ -3267,14 +3269,13 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         FieldIndexImmediate<validate> field(this, this->pc_ + len);
         if (!this->Validate(this->pc_ + len, field)) break;
         len += field.length;
-        if (!field.struct_index.struct_type->mutability(field.index)) {
+        const StructType* struct_type = field.struct_index.struct_type;
+        if (!struct_type->mutability(field.index)) {
           this->error(this->pc_, "setting immutable struct field");
           break;
         }
-        Value field_value = Pop(
-            0,
-            ValueType(
-                field.struct_index.struct_type->field(field.index).Unpack()));
+        Value field_value =
+            Pop(1, ValueType(struct_type->field(field.index).Unpack()));
         Value struct_obj =
             Pop(0, ValueType(ValueType::kOptRef, field.struct_index.index));
         CALL_INTERFACE_IF_REACHABLE(StructSet, struct_obj, field, field_value);
@@ -3284,8 +3285,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         ArrayIndexImmediate<validate> imm(this, this->pc_ + len);
         len += imm.length;
         if (!this->Validate(this->pc_, imm)) break;
-        Value length = Pop(0, kWasmI32);
-        Value initial_value = Pop(0, imm.array_type->element_type());
+        Value length = Pop(1, kWasmI32);
+        Value initial_value = Pop(0, imm.array_type->element_type().Unpack());
         Value* value = Push(ValueType(ValueType::kRef, imm.index));
         CALL_INTERFACE_IF_REACHABLE(ArrayNew, imm, length, initial_value,
                                     value);
@@ -3301,7 +3302,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
                       "Use array.get_s or array.get_u instead.");
           break;
         }
-        Value index = Pop(0, kWasmI32);
+        Value index = Pop(1, kWasmI32);
         Value array_obj = Pop(0, ValueType(ValueType::kOptRef, imm.index));
         Value* value = Push(imm.array_type->element_type());
         // TODO(7748): Optimize this when array_obj is non-nullable ref.
@@ -3316,8 +3317,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           this->error(this->pc_, "setting element of immutable array");
           break;
         }
-        Value value = Pop(0, imm.array_type->element_type().Unpack());
-        Value index = Pop(0, kWasmI32);
+        Value value = Pop(2, imm.array_type->element_type().Unpack());
+        Value index = Pop(1, kWasmI32);
         Value array_obj = Pop(0, ValueType(ValueType::kOptRef, imm.index));
         // TODO(7748): Optimize this when array_obj is non-nullable ref.
         CALL_INTERFACE_IF_REACHABLE(ArraySet, array_obj, imm, index, value);

src/wasm/value-type.h

@@ -25,16 +25,14 @@ class Simd128;
 // - All reference types, except funcref, are subtypes of eqref.
 // - optref(t1) <: optref(t2) iff t1 <: t2.
 // - ref(t1) <: optref(t2) iff t1 <: t2.
-// = ref(t1) <: ref(t2) iff t1 <: t2.
+// - ref(t1) <: ref(t2) iff t1 <: t2.
 //
 // Format: kind, log2Size, code, machineType, shortName, typeName
 //
 // Some of these types are from proposals that are not standardized yet:
 // - "ref" types per https://github.com/WebAssembly/function-references
 // - "optref"/"eqref" per https://github.com/WebAssembly/gc
-//
-// TODO(7748): Extend this with struct and function subtyping.
-//             Keep up to date with funcref vs. anyref subtyping.
+
 #define FOREACH_VALUE_TYPE(V)                                                \
   V(Stmt, -1, Void, None, 'v', "<stmt>")                                     \
   V(I32, 2, I32, Int32, 'i', "i32")                                          \
@@ -42,7 +40,7 @@ class Simd128;
   V(F32, 2, F32, Float32, 'f', "f32")                                        \
   V(F64, 3, F64, Float64, 'd', "f64")                                        \
   V(S128, 4, S128, Simd128, 's', "s128")                                     \
-  V(I8, 1, I8, Int8, 'b', "i8")                                              \
+  V(I8, 0, I8, Int8, 'b', "i8")                                              \
   V(I16, 1, I16, Int16, 'h', "i16")                                          \
   V(AnyRef, kSystemPointerSizeLog2, AnyRef, TaggedPointer, 'r', "anyref")    \
   V(FuncRef, kSystemPointerSizeLog2, FuncRef, TaggedPointer, 'a', "funcref") \

src/wasm/wasm-subtyping.cc

@@ -126,7 +126,6 @@ bool IsArraySubtype(uint32_t subtype_index, uint32_t supertype_index,
 }  // namespace
 
 // TODO(7748): Extend this with function subtyping.
-//             Keep up to date with funcref vs. anyref subtyping.
 V8_EXPORT_PRIVATE bool IsSubtypeOfRef(ValueType subtype, ValueType supertype,
                                       const WasmModule* module) {
   DCHECK(subtype != supertype && subtype.IsReferenceType() &&
@@ -158,7 +157,6 @@ V8_EXPORT_PRIVATE bool IsSubtypeOfRef(ValueType subtype, ValueType supertype,
 }
 
 // TODO(7748): Extend this with function subtyping.
-//             Keep up to date with funcref vs. anyref subtyping.
 ValueType CommonSubtype(ValueType a, ValueType b, const WasmModule* module) {
   if (a == b) return a;
   if (IsSubtypeOf(a, b, module)) return a;

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

@@ -58,11 +58,14 @@ static const WasmOpcode kInt32BinopOpcodes[] = {
 
 constexpr size_t kMaxByteSizedLeb128 = 127;
 
+using F = std::pair<ValueType, bool>;
+
 // A helper for tests that require a module environment for functions,
 // globals, or memories.
 class TestModuleBuilder {
  public:
-  explicit TestModuleBuilder(ModuleOrigin origin = kWasmOrigin) {
+  explicit TestModuleBuilder(ModuleOrigin origin = kWasmOrigin)
+      : allocator(), mod(std::make_unique<Zone>(&allocator, "TEST_ZONE")) {
     mod.origin = origin;
   }
   byte AddGlobal(ValueType type, bool mutability = true) {
@@ -110,6 +113,23 @@ class TestModuleBuilder {
     return static_cast<byte>(mod.tables.size() - 1);
   }
 
+  byte AddStruct(std::initializer_list<F> fields) {
+    StructType::Builder type_builder(mod.signature_zone.get(),
+                                     static_cast<uint32_t>(fields.size()));
+    for (F field : fields) {
+      type_builder.AddField(field.first, field.second);
+    }
+    mod.add_struct_type(type_builder.Build());
+    return static_cast<byte>(mod.type_kinds.size() - 1);
+  }
+
+  byte AddArray(ValueType type, bool mutability) {
+    ArrayType* array =
+        new (mod.signature_zone.get()) ArrayType(type, mutability);
+    mod.add_array_type(array);
+    return static_cast<byte>(mod.type_kinds.size() - 1);
+  }
+
   void InitializeMemory() {
     mod.has_memory = true;
     mod.initial_pages = 1;
@@ -149,6 +169,7 @@ class TestModuleBuilder {
   WasmModule* module() { return &mod; }
 
  private:
+  AccountingAllocator allocator;
   WasmModule mod;
 };
 
@@ -3341,6 +3362,31 @@ TEST_F(FunctionBodyDecoderTest, TableInitMultiTable) {
   }
 }
 
+TEST_F(FunctionBodyDecoderTest, UnpackPackedTypes) {
+  WASM_FEATURE_SCOPE(anyref);
+  WASM_FEATURE_SCOPE(gc);
+  {
+    TestModuleBuilder builder;
+    byte type_index = builder.AddStruct({F(kWasmI8, true), F(kWasmI16, false)});
+    module = builder.module();
+    ExpectValidates(sigs.v_v(),
+                    {WASM_STRUCT_SET(type_index, 0,
+                                     WASM_STRUCT_NEW(type_index, WASM_I32V(1),
+                                                     WASM_I32V(42)),
+                                     WASM_I32V(-1))});
+  }
+  {
+    TestModuleBuilder builder;
+    byte type_index = builder.AddArray(kWasmI8, true);
+    module = builder.module();
+    ExpectValidates(
+        sigs.v_v(),
+        {WASM_ARRAY_SET(type_index,
+                        WASM_ARRAY_NEW(type_index, WASM_I32V(10), WASM_I32V(5)),
+                        WASM_I32V(3), WASM_I32V(12345678))});
+  }
+}
+
 class BranchTableIteratorTest : public TestWithZone {
  public:
   BranchTableIteratorTest() : TestWithZone() {}