[wasm] Properly implement parsing of s33 values

Motivation:
We used to approximate s33/i33 value parsing by first checking for
specific negative codes, and then parsing an u32 value if that failed.
This is not correct in all cases.

Changes:
- Implement i33 parsing in Decoder.
- Factor out parsing of heap types into read_heap_type.
- Introduce HeapType::kBottom.
- Introduce helper functions in WasmFeatures and value_type_reader.
- Remove macros from the parsing of value types.
- HeapType::code now returns an i32 for compatibility with the i33
  requirement.
- Introduce HeapType::Repr.
- Renamings: HeapType::type() -> representation(),
             ValueType::heap() -> heap_representation()

Bug: v8:7748
Change-Id: I04deabce8837a48af2226411cd706a397f9e5725
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2274118
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68633}

src/compiler/wasm-compiler.cc

@@ -5377,7 +5377,8 @@ Node* WasmGraphBuilder::RttSub(wasm::HeapType type, Node* parent_rtt) {
   }
   return CALL_BUILTIN(
       WasmAllocateRtt,
-      graph()->NewNode(mcgraph()->common()->NumberConstant(type.type())),
+      graph()->NewNode(
+          mcgraph()->common()->NumberConstant(type.representation())),
       parent_rtt,
       LOAD_INSTANCE_FIELD(NativeContext, MachineType::TaggedPointer()));
 }
@@ -5780,7 +5781,7 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
     switch (type.kind()) {
       case wasm::ValueType::kRef:
       case wasm::ValueType::kOptRef: {
-        switch (type.heap()) {
+        switch (type.heap_representation()) {
           case wasm::HeapType::kExtern:
           case wasm::HeapType::kExn:
             return input;

src/wasm/c-api.cc

@@ -73,7 +73,7 @@ ValKind V8ValueTypeToWasm(i::wasm::ValueType v8_valtype) {
       return F64;
     case i::wasm::ValueType::kRef:
     case i::wasm::ValueType::kOptRef:
-      switch (v8_valtype.heap()) {
+      switch (v8_valtype.heap_representation()) {
         case i::wasm::HeapType::kFunc:
           return FUNCREF;
         case i::wasm::HeapType::kExtern:
@@ -1828,7 +1828,7 @@ auto Table::type() const -> own<TableType> {
   uint32_t max;
   if (!table->maximum_length().ToUint32(&max)) max = 0xFFFFFFFFu;
   ValKind kind;
-  switch (table->type().heap()) {
+  switch (table->type().heap_representation()) {
     case i::wasm::HeapType::kFunc:
       kind = FUNCREF;
       break;

src/wasm/decoder.h

@@ -127,6 +127,14 @@ class Decoder {
                                                                name);
   }
 
+  // Reads a variable-length 33-bit signed integer (little endian).
+  template <ValidateFlag validate>
+  int64_t read_i33v(const byte* pc, uint32_t* length,
+                    const char* name = "signed LEB33") {
+    return read_leb<int64_t, validate, kNoAdvancePc, kNoTrace, 33>(pc, length,
+                                                                   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.
@@ -354,21 +362,23 @@ class Decoder {
   }
 
   template <typename IntType, ValidateFlag validate, AdvancePCFlag advance_pc,
-            TraceFlag trace>
+            TraceFlag trace, size_t size_in_bits = 8 * sizeof(IntType)>
   inline IntType read_leb(const byte* pc, uint32_t* length,
                           const char* name = "varint") {
     DCHECK_IMPLIES(advance_pc, pc == pc_);
+    static_assert(size_in_bits <= 8 * sizeof(IntType),
+                  "leb does not fit in type");
     TRACE_IF(trace, "  +%u  %-20s: ", pc_offset(), name);
-    return read_leb_tail<IntType, validate, advance_pc, trace, 0>(pc, length,
-                                                                  name, 0);
+    return read_leb_tail<IntType, validate, advance_pc, trace, size_in_bits, 0>(
+        pc, length, name, 0);
   }
 
   template <typename IntType, ValidateFlag validate, AdvancePCFlag advance_pc,
-            TraceFlag trace, int byte_index>
+            TraceFlag trace, size_t size_in_bits, int byte_index>
   IntType read_leb_tail(const byte* pc, uint32_t* length, const char* name,
                         IntType result) {
     constexpr bool is_signed = std::is_signed<IntType>::value;
-    constexpr int kMaxLength = (sizeof(IntType) * 8 + 6) / 7;
+    constexpr int kMaxLength = (size_in_bits + 6) / 7;
     static_assert(byte_index < kMaxLength, "invalid template instantiation");
     constexpr int shift = byte_index * 7;
     constexpr bool is_last_byte = byte_index == kMaxLength - 1;
@@ -387,7 +397,7 @@ class Decoder {
       // Compilers are not smart enough to figure out statically that the
       // following call is unreachable if is_last_byte is false.
       constexpr int next_byte_index = byte_index + (is_last_byte ? 0 : 1);
-      return read_leb_tail<IntType, validate, advance_pc, trace,
+      return read_leb_tail<IntType, validate, advance_pc, trace, size_in_bits,
                            next_byte_index>(pc + 1, length, name, result);
     }
     if (advance_pc) pc_ = pc + (at_end ? 0 : 1);
@@ -404,11 +414,11 @@ class Decoder {
       // For unsigned values, the extra bits must be all zero.
       // For signed values, the extra bits *plus* the most significant bit must
       // either be 0, or all ones.
-      constexpr int kExtraBits = (sizeof(IntType) * 8) - ((kMaxLength - 1) * 7);
+      constexpr int kExtraBits = size_in_bits - ((kMaxLength - 1) * 7);
       constexpr int kSignExtBits = kExtraBits - (is_signed ? 1 : 0);
       const byte checked_bits = b & (0xFF << kSignExtBits);
       constexpr byte kSignExtendedExtraBits = 0x7f & (0xFF << kSignExtBits);
-      bool valid_extra_bits =
+      const bool valid_extra_bits =
           checked_bits == 0 ||
           (is_signed && checked_bits == kSignExtendedExtraBits);
       if (!validate) {

src/wasm/local-decl-encoder.cc

@@ -38,7 +38,7 @@ size_t LocalDeclEncoder::Emit(byte* buffer) const {
       ++pos;
     }
     if (locals_type.encoding_needs_heap_type()) {
-      LEBHelper::write_u32v(&pos, locals_type.heap_type().code());
+      LEBHelper::write_i32v(&pos, locals_type.heap_type().code());
     }
   }
   DCHECK_EQ(Size(), pos - buffer);
@@ -66,7 +66,7 @@ size_t LocalDeclEncoder::Size() const {
             1 +                                // Opcode
             (p.second.has_depth() ? 1 : 0) +   // Inheritance depth
             (p.second.encoding_needs_heap_type()
-                 ? LEBHelper::sizeof_u32v(p.second.heap_type().code())
+                 ? LEBHelper::sizeof_i32v(p.second.heap_type().code())
                  : 0);  // ref. index
   }
   return size;

src/wasm/value-type.h

@@ -45,39 +45,67 @@ class Simd128;
 
 class HeapType {
  public:
-  static const uint32_t kFunc = kV8MaxWasmTypes;  // shorthand: c
-  static const uint32_t kExtern = kFunc + 1;      // shorthand: e
-  static const uint32_t kEq = kFunc + 2;          // shorthand: q
-  static const uint32_t kExn = kFunc + 3;         // shorthand: x
-  static const uint32_t kI31 = kFunc + 4;         // shorthand: j
+  enum Representation : uint32_t {
+    kFunc = kV8MaxWasmTypes,  // shorthand: c
+    kExtern,                  // shorthand: e
+    kEq,                      // shorthand: q
+    kExn,                     // shorthand: x
+    kI31,                     // shorthand: j
+    // This code is used to represent failures in the parsing of heap types and
+    // does not correspond to a wasm heap type.
+    kBottom
+  };
   // Internal use only; defined in the public section to make it easy to
   // check that they are defined correctly:
-  static const uint32_t kFirstSentinel = kFunc;
-  static const uint32_t kLastSentinel = kI31;
+  static constexpr Representation kFirstSentinel = kFunc;
+  static constexpr Representation kLastSentinel = kBottom;
+
+  static constexpr HeapType from_code(uint8_t code) {
+    switch (code) {
+      case ValueTypeCode::kLocalFuncRef:
+        return HeapType(kFunc);
+      case ValueTypeCode::kLocalExternRef:
+        return HeapType(kExtern);
+      case ValueTypeCode::kLocalEqRef:
+        return HeapType(kEq);
+      case ValueTypeCode::kLocalExnRef:
+        return HeapType(kExn);
+      case ValueTypeCode::kLocalI31Ref:
+        return HeapType(kI31);
+      default:
+        return HeapType(kBottom);
+    }
+  }
 
-  explicit constexpr HeapType(uint32_t type) : type_(type) {
+  explicit constexpr HeapType(Representation repr) : representation_(repr) {
     CONSTEXPR_DCHECK(is_valid());
   }
+  explicit constexpr HeapType(uint32_t repr)
+      : HeapType(static_cast<Representation>(repr)) {}
 
   constexpr bool operator==(HeapType other) const {
-    return type_ == other.type_;
+    return representation_ == other.representation_;
   }
   constexpr bool operator!=(HeapType other) const {
-    return type_ != other.type_;
+    return representation_ != other.representation_;
   }
 
-  constexpr uint32_t type() { return type_; }
-  constexpr uint32_t ref_index() {
+  constexpr uint32_t representation() const { return representation_; }
+  constexpr uint32_t ref_index() const {
     CONSTEXPR_DCHECK(is_index());
-    return type_;
+    return representation_;
+  }
+
+  constexpr bool is_generic() const {
+    return representation_ >= kFirstSentinel;
   }
 
-  constexpr bool is_generic() { return type_ >= kFirstSentinel; }
+  constexpr bool is_index() const { return !is_generic(); }
 
-  constexpr bool is_index() { return !is_generic(); }
+  constexpr bool is_bottom() const { return representation_ == kBottom; }
 
-  std::string name() {
-    switch (type_) {
+  std::string name() const {
+    switch (representation_) {
       case kFunc:
         return std::string("func");
       case kExtern:
@@ -89,31 +117,34 @@ class HeapType {
       case kI31:
         return std::string("i31");
       default:
-        return std::to_string(type_);
+        return std::to_string(representation_);
     }
   }
 
-  constexpr uint32_t code() {
-    switch (type_) {
+  constexpr int32_t code() const {
+    // kLocal* codes represent the first byte of the LEB128 encoding. To get the
+    // int32 represented by a code, we need to sign-extend it from 7 to 32 bits.
+    int32_t mask = 0xFFFFFF80;
+    switch (representation_) {
       case kFunc:
-        return kLocalFuncRef;
+        return mask | kLocalFuncRef;
       case kExn:
-        return kLocalExnRef;
+        return mask | kLocalExnRef;
       case kExtern:
-        return kLocalExternRef;
+        return mask | kLocalExternRef;
       case kEq:
-        return kLocalEqRef;
+        return mask | kLocalEqRef;
       case kI31:
-        return kLocalI31Ref;
+        return mask | kLocalI31Ref;
       default:
-        return type_;
+        return static_cast<int32_t>(representation_);
     }
   }
 
  private:
   friend class ValueType;
-  uint32_t type_;
-  constexpr bool is_valid() const { return type_ <= kLastSentinel; }
+  Representation representation_;
+  constexpr bool is_valid() const { return representation_ <= kLastSentinel; }
 };
 enum Nullability : bool { kNonNullable, kNullable };
 
@@ -134,7 +165,8 @@ class ValueType {
   constexpr bool is_nullable() const { return kind() == kOptRef; }
 
   constexpr bool is_reference_to(uint32_t htype) const {
-    return (kind() == kRef || kind() == kOptRef) && heap() == htype;
+    return (kind() == kRef || kind() == kOptRef) &&
+           heap_representation() == htype;
   }
 
   constexpr bool is_defaultable() const {
@@ -157,25 +189,27 @@ class ValueType {
     return ValueType(KindField::encode(kind));
   }
   static constexpr ValueType Ref(uint32_t heap_type, Nullability nullability) {
-    CONSTEXPR_DCHECK(HeapType(heap_type).is_valid());
+    CONSTEXPR_DCHECK(heap_type != HeapType::kBottom &&
+                     HeapType(heap_type).is_valid());
     return ValueType(
         KindField::encode(nullability == kNullable ? kOptRef : kRef) |
         HeapTypeField::encode(heap_type));
   }
   static constexpr ValueType Ref(HeapType heap_type, Nullability nullability) {
-    return Ref(heap_type.type(), nullability);
+    return Ref(heap_type.representation(), nullability);
   }
 
   static constexpr ValueType Rtt(uint32_t heap_type,
                                  uint8_t inheritance_depth) {
-    CONSTEXPR_DCHECK(HeapType(heap_type).is_valid());
+    CONSTEXPR_DCHECK(heap_type != HeapType::kBottom &&
+                     HeapType(heap_type).is_valid());
     return ValueType(KindField::encode(kRtt) |
                      HeapTypeField::encode(heap_type) |
                      DepthField::encode(inheritance_depth));
   }
   static constexpr ValueType Rtt(HeapType heap_type,
                                  uint8_t inheritance_depth) {
-    return Rtt(heap_type.type(), inheritance_depth);
+    return Rtt(heap_type.representation(), inheritance_depth);
   }
 
   static constexpr ValueType FromRawBitField(uint32_t bit_field) {
@@ -183,11 +217,13 @@ class ValueType {
   }
 
   constexpr Kind kind() const { return KindField::decode(bit_field_); }
-  constexpr uint32_t heap() const {
+  constexpr uint32_t heap_representation() const {
     CONSTEXPR_DCHECK(is_reference_type());
     return HeapTypeField::decode(bit_field_);
   }
-  constexpr HeapType heap_type() const { return HeapType(heap()); }
+  constexpr HeapType heap_type() const {
+    return HeapType(heap_representation());
+  }
   constexpr uint8_t depth() const {
     CONSTEXPR_DCHECK(has_depth());
     return DepthField::decode(bit_field_);
@@ -252,7 +288,7 @@ class ValueType {
     CONSTEXPR_DCHECK(kind() != kBottom);
     switch (kind()) {
       case kOptRef:
-        switch (heap()) {
+        switch (heap_representation()) {
           case HeapType::kFunc:
             return kLocalFuncRef;
           case HeapType::kExtern:
@@ -265,7 +301,7 @@ class ValueType {
             return kLocalOptRef;
         }
       case kRef:
-        if (heap() == HeapType::kI31) return kLocalI31Ref;
+        if (heap_representation() == HeapType::kI31) return kLocalI31Ref;
         return kLocalRef;
       case kStmt:
         return kLocalVoid;
@@ -283,9 +319,10 @@ class ValueType {
   }
 
   constexpr bool encoding_needs_heap_type() const {
-    return (kind() == kRef && heap() != HeapType::kI31) || kind() == kRtt ||
-           (kind() == kOptRef &&
-            (!heap_type().is_generic() || heap() == HeapType::kI31));
+    return (kind() == kRef && heap_representation() != HeapType::kI31) ||
+           kind() == kRtt ||
+           (kind() == kOptRef && (!heap_type().is_generic() ||
+                                  heap_representation() == HeapType::kI31));
   }
 
   static ValueType For(MachineType type) {
@@ -323,14 +360,15 @@ class ValueType {
     std::ostringstream buf;
     switch (kind()) {
       case kRef:
-        if (heap() == HeapType::kI31) {
+        if (heap_representation() == HeapType::kI31) {
           buf << "i31ref";
         } else {
           buf << "(ref " << heap_type().name() << ")";
         }
         break;
       case kOptRef:
-        if (heap_type().is_generic()) {
+        if (heap_type().is_generic() &&
+            heap_representation() != HeapType::kI31) {
           // We prefer the shorthand to be backwards-compatible with previous
           // proposals.
           buf << heap_type().name() << "ref";

src/wasm/wasm-features.h

@@ -39,6 +39,15 @@ class WasmFeatures : public base::EnumSet<WasmFeature> {
   FOREACH_WASM_FEATURE(DECL_FEATURE_GETTER)
 #undef DECL_FEATURE_GETTER
 
+  static constexpr const char* name_for_feature(WasmFeature feature) {
+    switch (feature) {
+#define NAME(feat, ...)              \
+  case WasmFeature::kFeature_##feat: \
+    return #feat;
+      FOREACH_WASM_FEATURE(NAME)
+    }
+#undef NAME
+  }
   static inline constexpr WasmFeatures All();
   static inline constexpr WasmFeatures None();
   static inline constexpr WasmFeatures ForAsmjs();

src/wasm/wasm-js.cc

@@ -1337,7 +1337,7 @@ void WebAssemblyGlobal(const v8::FunctionCallbackInfo<v8::Value>& args) {
     }
     case i::wasm::ValueType::kRef:
     case i::wasm::ValueType::kOptRef: {
-      switch (type.heap()) {
+      switch (type.heap_representation()) {
         case i::wasm::HeapType::kExtern:
         case i::wasm::HeapType::kExn: {
           if (args.Length() < 2) {
@@ -1834,7 +1834,7 @@ void WebAssemblyGlobalGetValueCommon(
       break;
     case i::wasm::ValueType::kRef:
     case i::wasm::ValueType::kOptRef:
-      switch (receiver->type().heap()) {
+      switch (receiver->type().heap_representation()) {
         case i::wasm::HeapType::kExtern:
         case i::wasm::HeapType::kFunc:
         case i::wasm::HeapType::kExn:
@@ -1923,7 +1923,7 @@ void WebAssemblyGlobalSetValue(
       break;
     case i::wasm::ValueType::kRef:
     case i::wasm::ValueType::kOptRef:
-      switch (receiver->type().heap()) {
+      switch (receiver->type().heap_representation()) {
         case i::wasm::HeapType::kExtern:
         case i::wasm::HeapType::kExn:
           receiver->SetExternRef(Utils::OpenHandle(*args[0]));

src/wasm/wasm-module-builder.cc

@@ -418,7 +418,7 @@ void WriteValueType(ZoneBuffer* buffer, const ValueType& type) {
     buffer->write_u32v(type.depth());
   }
   if (type.encoding_needs_heap_type()) {
-    buffer->write_u32v(type.heap_type().code());
+    buffer->write_i32v(type.heap_type().code());
   }
 }
 
@@ -569,7 +569,7 @@ void WasmModuleBuilder::WriteTo(ZoneBuffer* buffer) const {
           break;
         case WasmInitExpr::kRefNullConst:
           buffer->write_u8(kExprRefNull);
-          buffer->write_u32v(global.type.heap_type().code());
+          buffer->write_i32v(global.type.heap_type().code());
           break;
         case WasmInitExpr::kRefFuncConst:
           UNIMPLEMENTED();

src/wasm/wasm-objects.cc

@@ -286,7 +286,7 @@ Handle<WasmTableObject> WasmTableObject::New(Isolate* isolate,
   table_obj->set_entries(*backing_store);
   table_obj->set_current_length(initial);
   table_obj->set_maximum_length(*max);
-  table_obj->set_raw_type(static_cast<int>(type.heap()));
+  table_obj->set_raw_type(static_cast<int>(type.heap_representation()));
 
   table_obj->set_dispatch_tables(ReadOnlyRoots(isolate).empty_fixed_array());
   if (entries != nullptr) {

src/wasm/wasm-subtyping.cc

@@ -145,15 +145,15 @@ V8_NOINLINE V8_EXPORT_PRIVATE bool IsSubtypeOfImpl(ValueType subtype,
 
   HeapType sub_heap = subtype.heap_type();
   HeapType super_heap = supertype.heap_type();
-  DCHECK(!module->has_signature(sub_heap.type()) &&
-         !module->has_signature(super_heap.type()));
+  DCHECK(!module->has_signature(sub_heap.representation()) &&
+         !module->has_signature(super_heap.representation()));
 
   if (sub_heap == super_heap) {
     return true;
   }
   // eqref is a supertype of all reference types except funcref.
-  if (super_heap.type() == HeapType::kEq) {
-    return sub_heap.type() != HeapType::kFunc;
+  if (super_heap.representation() == HeapType::kEq) {
+    return sub_heap.representation() != HeapType::kFunc;
   }
   // At the moment, generic heap types are not subtyping-related otherwise.
   if (sub_heap.is_generic() || super_heap.is_generic()) {

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

@@ -945,6 +945,7 @@ TEST(MemoryWithOOBEmptyDataSegment) {
 TEST(GcStructIdsPass) {
   {
     EXPERIMENTAL_FLAG_SCOPE(gc);
+    EXPERIMENTAL_FLAG_SCOPE(typed_funcref);
     EXPERIMENTAL_FLAG_SCOPE(reftypes);
     Isolate* isolate = CcTest::InitIsolateOnce();
     HandleScope scope(isolate);
@@ -994,6 +995,7 @@ TEST(GcStructIdsPass) {
 TEST(GcTypeIdsUndefinedIndex) {
   {
     EXPERIMENTAL_FLAG_SCOPE(gc);
+    EXPERIMENTAL_FLAG_SCOPE(typed_funcref);
     EXPERIMENTAL_FLAG_SCOPE(reftypes);
     Isolate* isolate = CcTest::InitIsolateOnce();
     HandleScope scope(isolate);
@@ -1027,6 +1029,7 @@ TEST(GcTypeIdsUndefinedIndex) {
 TEST(GcTypeIdsIllegalIndex) {
   {
     EXPERIMENTAL_FLAG_SCOPE(gc);
+    EXPERIMENTAL_FLAG_SCOPE(typed_funcref);
     EXPERIMENTAL_FLAG_SCOPE(reftypes);
     Isolate* isolate = CcTest::InitIsolateOnce();
     HandleScope scope(isolate);
@@ -1064,6 +1067,7 @@ TEST(GcTypeIdsIllegalIndex) {
 TEST(GcTypeIdsFunSigIllegalIndex) {
   {
     EXPERIMENTAL_FLAG_SCOPE(gc);
+    EXPERIMENTAL_FLAG_SCOPE(typed_funcref);
     EXPERIMENTAL_FLAG_SCOPE(reftypes);
     Isolate* isolate = CcTest::InitIsolateOnce();
     HandleScope scope(isolate);
@@ -1076,7 +1080,7 @@ TEST(GcTypeIdsFunSigIllegalIndex) {
         kTypeSectionCode,       // --
         U32V_1(7),              // Section size
         U32V_1(1),              // type count
-        kWasmFunctionTypeCode,  // index 0 = int32 -> int32
+        kWasmFunctionTypeCode,  // index 0 = int32 -> (ref null 0)
         U32V_1(1),              // param count
         kLocalI32,              // param 0
         U32V_1(1),              // returns count

test/common/wasm/wasm-interpreter.cc

@@ -2792,7 +2792,7 @@ class WasmInterpreterInternals {
         }
         case ValueType::kRef:
         case ValueType::kOptRef: {
-          switch (sig->GetParam(i).heap()) {
+          switch (sig->GetParam(i).heap_representation()) {
             case HeapType::kExtern:
             case HeapType::kExn:
             case HeapType::kFunc: {
@@ -2909,7 +2909,7 @@ class WasmInterpreterInternals {
         }
         case ValueType::kRef:
         case ValueType::kOptRef: {
-          switch (sig->GetParam(i).heap()) {
+          switch (sig->GetParam(i).heap_representation()) {
             case HeapType::kExtern:
             case HeapType::kExn:
             case HeapType::kFunc: {

test/fuzzer/wasm-fuzzer-common.cc

@@ -90,7 +90,7 @@ const char* ValueTypeToConstantName(ValueType type) {
     case ValueType::kF64:
       return "kWasmF64";
     case ValueType::kOptRef:
-      switch (type.heap()) {
+      switch (type.heap_representation()) {
         case HeapType::kExtern:
           return "kWasmExternRef";
         case HeapType::kFunc: